[V3,2/8] drivers: boot_constraint: Add boot_constraints_disable kernel parameter

Message ID 4bec0c3e58f76e06e5305c5ddbab859e2a87935e.1501578037.git.viresh.kumar@linaro.org
State New
Headers show
Series
  • drivers: Boot Constraints core
Related show

Commit Message

Viresh Kumar Aug. 1, 2017, 9:23 a.m.
Users must be given an option to discard any constraints set by
bootloaders. For example, consider that a constraint is set for the LCD
controller's supply and the LCD driver isn't loaded by the kernel. If
the user doesn't need to use the LCD device, then he shouldn't be forced
to honour the constraint.

We can also think about finer control of such constraints with help of
some sysfs files, but a kernel parameter is fine to begin with.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 drivers/base/boot_constraints/core.c            | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

-- 
2.13.0.71.gd7076ec9c9cb

Comments

Greg Kroah-Hartman Aug. 29, 2017, 6:37 a.m. | #1
On Tue, Aug 01, 2017 at 02:53:43PM +0530, Viresh Kumar wrote:
> Users must be given an option to discard any constraints set by

> bootloaders. For example, consider that a constraint is set for the LCD

> controller's supply and the LCD driver isn't loaded by the kernel. If

> the user doesn't need to use the LCD device, then he shouldn't be forced

> to honour the constraint.

> 

> We can also think about finer control of such constraints with help of

> some sysfs files, but a kernel parameter is fine to begin with.

> 

> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  Documentation/admin-guide/kernel-parameters.txt |  3 +++

>  drivers/base/boot_constraints/core.c            | 17 +++++++++++++++++

>  2 files changed, 20 insertions(+)

> 

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt

> index d9c171ce4190..0706d1b6004d 100644

> --- a/Documentation/admin-guide/kernel-parameters.txt

> +++ b/Documentation/admin-guide/kernel-parameters.txt

> @@ -426,6 +426,9 @@

>  			embedded devices based on command line input.

>  			See Documentation/block/cmdline-partition.txt

>  

> +	boot_constraints_disable

> +			Do not set any boot constraints for devices.


Shouldn't that be the default?  As really, that is what the situation is
today, why force everyone to always enable the disable value?  And
enabling a value to disable something is usually a sign of bad naming...

> +

>  	boot_delay=	Milliseconds to delay each printk during boot.

>  			Values larger than 10 seconds (10000) are changed to

>  			no delay (0).

> diff --git a/drivers/base/boot_constraints/core.c b/drivers/base/boot_constraints/core.c

> index 366a05d6d9ba..e0c33b2b216f 100644

> --- a/drivers/base/boot_constraints/core.c

> +++ b/drivers/base/boot_constraints/core.c

> @@ -24,6 +24,17 @@

>  static LIST_HEAD(constraint_devices);

>  static DEFINE_MUTEX(constraint_devices_mutex);

>  

> +static bool boot_constraints_disabled;


Again, this should only be an "enable" type of option, that kicks in if
you are using this type of bootloader/kernel interaction.  Don't force
someone to disable it.

thanks,

greg k-h
Jani Nikula Aug. 29, 2017, 6:48 a.m. | #2
On Tue, 29 Aug 2017, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, Aug 01, 2017 at 02:53:43PM +0530, Viresh Kumar wrote:

>> Users must be given an option to discard any constraints set by

>> bootloaders. For example, consider that a constraint is set for the LCD

>> controller's supply and the LCD driver isn't loaded by the kernel. If

>> the user doesn't need to use the LCD device, then he shouldn't be forced

>> to honour the constraint.

>> 

>> We can also think about finer control of such constraints with help of

>> some sysfs files, but a kernel parameter is fine to begin with.

>> 

>> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>

>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

>> ---

>>  Documentation/admin-guide/kernel-parameters.txt |  3 +++

>>  drivers/base/boot_constraints/core.c            | 17 +++++++++++++++++

>>  2 files changed, 20 insertions(+)

>> 

>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt

>> index d9c171ce4190..0706d1b6004d 100644

>> --- a/Documentation/admin-guide/kernel-parameters.txt

>> +++ b/Documentation/admin-guide/kernel-parameters.txt

>> @@ -426,6 +426,9 @@

>>  			embedded devices based on command line input.

>>  			See Documentation/block/cmdline-partition.txt

>>  

>> +	boot_constraints_disable

>> +			Do not set any boot constraints for devices.

>

> Shouldn't that be the default?  As really, that is what the situation is

> today, why force everyone to always enable the disable value?  And

> enabling a value to disable something is usually a sign of bad naming...

>

>> +

>>  	boot_delay=	Milliseconds to delay each printk during boot.

>>  			Values larger than 10 seconds (10000) are changed to

>>  			no delay (0).

>> diff --git a/drivers/base/boot_constraints/core.c b/drivers/base/boot_constraints/core.c

>> index 366a05d6d9ba..e0c33b2b216f 100644

>> --- a/drivers/base/boot_constraints/core.c

>> +++ b/drivers/base/boot_constraints/core.c

>> @@ -24,6 +24,17 @@

>>  static LIST_HEAD(constraint_devices);

>>  static DEFINE_MUTEX(constraint_devices_mutex);

>>  

>> +static bool boot_constraints_disabled;

>

> Again, this should only be an "enable" type of option, that kicks in if

> you are using this type of bootloader/kernel interaction.  Don't force

> someone to disable it.


I might add that "disable" type options lead to annoying double
negatives. Regardless of the default, I'd generally prefer "enable" type
options that you enable/disable as needed.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
Viresh Kumar Aug. 29, 2017, 10:02 a.m. | #3
On 29-08-17, 08:37, Greg Kroah-Hartman wrote:
> On Tue, Aug 01, 2017 at 02:53:43PM +0530, Viresh Kumar wrote:

> > +	boot_constraints_disable

> > +			Do not set any boot constraints for devices.

> 

> Shouldn't that be the default?  As really, that is what the situation is

> today, why force everyone to always enable the disable value?  And

> enabling a value to disable something is usually a sign of bad naming...


I will explain once again how it is getting used and then will do whatever you
suggest.

- Platforms that don't need boot constraints should not enable the CONFIG in the
  first place. Though we use the same kernel image on multiple hardware types
  many times.
- If a platform doesn't have a platform-specific driver that adds constraints at
  boot, then the boot constraint core wouldn't get into picture at all and it is
  as good as being disabled.
- And the above boot-argument (boot_constraints_disable) is used ONLY in the
  case where the platform driver is adding boot constraints at runtime.

So, the boot-constraints are disabled by default for everyone even if the
configuration is enabled. And that's why I named it the way it is right now.

Do you still feel that it needs to be renamed? 

-- 
viresh
Greg Kroah-Hartman Aug. 29, 2017, 11:56 a.m. | #4
On Tue, Aug 29, 2017 at 12:02:41PM +0200, Viresh Kumar wrote:
> On 29-08-17, 08:37, Greg Kroah-Hartman wrote:

> > On Tue, Aug 01, 2017 at 02:53:43PM +0530, Viresh Kumar wrote:

> > > +	boot_constraints_disable

> > > +			Do not set any boot constraints for devices.

> > 

> > Shouldn't that be the default?  As really, that is what the situation is

> > today, why force everyone to always enable the disable value?  And

> > enabling a value to disable something is usually a sign of bad naming...

> 

> I will explain once again how it is getting used and then will do whatever you

> suggest.

> 

> - Platforms that don't need boot constraints should not enable the CONFIG in the

>   first place. Though we use the same kernel image on multiple hardware types

>   many times.


Right, which means this is useless as an option, don't ever rely on it
:)

> - If a platform doesn't have a platform-specific driver that adds constraints at

>   boot, then the boot constraint core wouldn't get into picture at all and it is

>   as good as being disabled.


Possibly, but see above for the goal of one kernel image, many different
devices.

> - And the above boot-argument (boot_constraints_disable) is used ONLY in the

>   case where the platform driver is adding boot constraints at runtime.

> 

> So, the boot-constraints are disabled by default for everyone even if the

> configuration is enabled. And that's why I named it the way it is right now.

> 

> Do you still feel that it needs to be renamed? 


Well, negative options are ackward (although usb_disable is an
option...)  It still feels wrong, and I worry about the above
single-image goal...

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d9c171ce4190..0706d1b6004d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -426,6 +426,9 @@ 
 			embedded devices based on command line input.
 			See Documentation/block/cmdline-partition.txt
 
+	boot_constraints_disable
+			Do not set any boot constraints for devices.
+
 	boot_delay=	Milliseconds to delay each printk during boot.
 			Values larger than 10 seconds (10000) are changed to
 			no delay (0).
diff --git a/drivers/base/boot_constraints/core.c b/drivers/base/boot_constraints/core.c
index 366a05d6d9ba..e0c33b2b216f 100644
--- a/drivers/base/boot_constraints/core.c
+++ b/drivers/base/boot_constraints/core.c
@@ -24,6 +24,17 @@ 
 static LIST_HEAD(constraint_devices);
 static DEFINE_MUTEX(constraint_devices_mutex);
 
+static bool boot_constraints_disabled;
+
+static int __init constraints_disable(char *str)
+{
+	boot_constraints_disabled = true;
+	pr_debug("disabled\n");
+
+	return 0;
+}
+early_param("boot_constraints_disable", constraints_disable);
+
 /* Boot constraints core */
 
 static struct constraint_dev *constraint_device_find(struct device *dev)
@@ -126,6 +137,9 @@  int dev_boot_constraint_add(struct device *dev,
 	struct constraint *constraint;
 	int ret;
 
+	if (boot_constraints_disabled)
+		return -ENODEV;
+
 	mutex_lock(&constraint_devices_mutex);
 
 	/* Find or add the cdev type first */
@@ -184,6 +198,9 @@  void dev_boot_constraints_remove(struct device *dev)
 	struct constraint_dev *cdev;
 	struct constraint *constraint, *temp;
 
+	if (boot_constraints_disabled)
+		return;
+
 	mutex_lock(&constraint_devices_mutex);
 
 	cdev = constraint_device_find(dev);