Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1722173 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 2/8] drivers: boot_constraint: Add  boot_constraints_disable kernel parameter Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-29 08:40 +0200
    Re: [PATCH V3 2/8] drivers: boot_constraint: Add boot_constraints_disable kernel parameter Jani Nikula <jani.nikula@linux.intel.com> - 2017-08-29 08:50 +0200
    Re: [PATCH V3 2/8] drivers: boot_constraint: Add  boot_constraints_disable kernel parameter Viresh Kumar <viresh.kumar@linaro.org> - 2017-08-29 12:10 +0200
      Re: [PATCH V3 2/8] drivers: boot_constraint: Add  boot_constraints_disable kernel parameter Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-29 14:00 +0200

#1722173 — Re: [PATCH V3 2/8] drivers: boot_constraint: Add boot_constraints_disable kernel parameter

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

[toc] | [next] | [standalone]

#1722184 — Re: [PATCH V3 2/8] drivers: boot_constraint: Add boot_constraints_disable kernel parameter

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

[toc] | [prev] | [next] | [standalone]

#1722293

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

[toc] | [prev] | [next] | [standalone]

#1722407

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...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web