| Message ID | 4bec0c3e58f76e06e5305c5ddbab859e2a87935e.1501578037.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: Boot Constraints core | expand |
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
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
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
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...
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);