| Message ID | 87hak8qfu5.fsf@rustcorp.com.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2013-03-19 at 13:02 +1030, Rusty Russell wrote: > Andy Lutomirski <luto@amacapital.net> writes: > > On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: > >> Andy Lutomirski <luto@amacapital.net> writes: > >>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: > >>>> Err, yes. Don't remove module parameters, they're part of the API. Do > >>>> you have a particular example? > >>> > >>> So things like i915.i915_enable_ppgtt, which is there to enable > >>> something experimental, needs to stay forever once the relevant > >>> feature becomes non-experimental and non-optional? This seems silly. > ... > >>> Having the module parameter go away while still allowing the module to > >>> load seems like a good solution (possibly with a warning in the logs > >>> so the user can eventually delete the parameter). > >> > >> Why not do that for *every* missing parameter then? Why have this weird > >> notation where the user must know that the parameter might one day go > >> away? > > > > Fair enough. What about the other approach, then? Always warn if an > > option doesn't match (built-in or otherwise) but load the module > > anyways. > > What does everyone think of this? Jon, Lucas, does this match your > experience? I'm not sure why I'm being cc'd on this, though I did recently remove a module parameter (sfc.rx_alloc_method). For what it's worth: > Subject: modules: don't fail to load on unknown parameters. > > Although parameters are supposed to be part of the kernel API, experimental > parameters are often removed. In addition, downgrading a kernel might cause > previously-working modules to fail to load. > > On balance, it's probably better to warn, and load the module anyway. I agree with this. > Reported-by: Andy Lutomirski <luto@amacapital.net> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> [...] This should also go to stable, so the downgrading issue doesn't continue to bite people. Ben.
Hi Rusty, On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: > Andy Lutomirski <luto@amacapital.net> writes: >> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> Andy Lutomirski <luto@amacapital.net> writes: >>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>>> Err, yes. Don't remove module parameters, they're part of the API. Do >>>>> you have a particular example? >>>> >>>> So things like i915.i915_enable_ppgtt, which is there to enable >>>> something experimental, needs to stay forever once the relevant >>>> feature becomes non-experimental and non-optional? This seems silly. > ... >>>> Having the module parameter go away while still allowing the module to >>>> load seems like a good solution (possibly with a warning in the logs >>>> so the user can eventually delete the parameter). >>> >>> Why not do that for *every* missing parameter then? Why have this weird >>> notation where the user must know that the parameter might one day go >>> away? >> >> Fair enough. What about the other approach, then? Always warn if an >> option doesn't match (built-in or otherwise) but load the module >> anyways. > > What does everyone think of this? Jon, Lucas, does this match your > experience? > > Thanks, > Rusty. > > Subject: modules: don't fail to load on unknown parameters. > > Although parameters are supposed to be part of the kernel API, experimental > parameters are often removed. In addition, downgrading a kernel might cause > previously-working modules to fail to load. I agree with this reasoning > > On balance, it's probably better to warn, and load the module anyway. However loading the module anyway would bring at least one drawback: if the user made a typo when passing the option the module would load anyway and he will probably not even look in the log, since there's was no errors from modprobe. For finit_module we could put a flag to trigger this behavior and propagate it to modprobe, but this is not possible with init_module(). I can't think in any other option right now... do you have any? Lucas De Marchi -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 19, 2013 at 8:26 PM, Lucas De Marchi <lucas.demarchi@profusion.mobi> wrote: > Hi Rusty, > > On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Andy Lutomirski <luto@amacapital.net> writes: >>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>> Andy Lutomirski <luto@amacapital.net> writes: >>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>>>> Err, yes. Don't remove module parameters, they're part of the API. Do >>>>>> you have a particular example? >>>>> >>>>> So things like i915.i915_enable_ppgtt, which is there to enable >>>>> something experimental, needs to stay forever once the relevant >>>>> feature becomes non-experimental and non-optional? This seems silly. >> ... >>>>> Having the module parameter go away while still allowing the module to >>>>> load seems like a good solution (possibly with a warning in the logs >>>>> so the user can eventually delete the parameter). >>>> >>>> Why not do that for *every* missing parameter then? Why have this weird >>>> notation where the user must know that the parameter might one day go >>>> away? >>> >>> Fair enough. What about the other approach, then? Always warn if an >>> option doesn't match (built-in or otherwise) but load the module >>> anyways. >> >> What does everyone think of this? Jon, Lucas, does this match your >> experience? >> >> Thanks, >> Rusty. >> >> Subject: modules: don't fail to load on unknown parameters. >> >> Although parameters are supposed to be part of the kernel API, experimental >> parameters are often removed. In addition, downgrading a kernel might cause >> previously-working modules to fail to load. > > I agree with this reasoning > >> >> On balance, it's probably better to warn, and load the module anyway. > > However loading the module anyway would bring at least one drawback: > if the user made a typo when passing the option the module would load > anyway and he will probably not even look in the log, since there's > was no errors from modprobe. > > For finit_module we could put a flag to trigger this behavior and > propagate it to modprobe, but this is not possible with init_module(). > I can't think in any other option right now... do you have any? Have a different finit_module return value for "successfully loaded, but there were warnings" perhaps? --Andy > > > Lucas De Marchi
On Tue, Mar 19, 2013 at 8:32 PM, Andy Lutomirski <luto@amacapital.net> wrote: > On Tue, Mar 19, 2013 at 8:26 PM, Lucas De Marchi > <lucas.demarchi@profusion.mobi> wrote: >> Hi Rusty, >> >> On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> Andy Lutomirski <luto@amacapital.net> writes: >>>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>>> Andy Lutomirski <luto@amacapital.net> writes: >>>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>>>>> Err, yes. Don't remove module parameters, they're part of the API. Do >>>>>>> you have a particular example? >>>>>> >>>>>> So things like i915.i915_enable_ppgtt, which is there to enable >>>>>> something experimental, needs to stay forever once the relevant >>>>>> feature becomes non-experimental and non-optional? This seems silly. >>> ... >>>>>> Having the module parameter go away while still allowing the module to >>>>>> load seems like a good solution (possibly with a warning in the logs >>>>>> so the user can eventually delete the parameter). >>>>> >>>>> Why not do that for *every* missing parameter then? Why have this weird >>>>> notation where the user must know that the parameter might one day go >>>>> away? >>>> >>>> Fair enough. What about the other approach, then? Always warn if an >>>> option doesn't match (built-in or otherwise) but load the module >>>> anyways. >>> >>> What does everyone think of this? Jon, Lucas, does this match your >>> experience? >>> >>> Thanks, >>> Rusty. >>> >>> Subject: modules: don't fail to load on unknown parameters. >>> >>> Although parameters are supposed to be part of the kernel API, experimental >>> parameters are often removed. In addition, downgrading a kernel might cause >>> previously-working modules to fail to load. >> >> I agree with this reasoning >> >>> >>> On balance, it's probably better to warn, and load the module anyway. >> >> However loading the module anyway would bring at least one drawback: >> if the user made a typo when passing the option the module would load >> anyway and he will probably not even look in the log, since there's >> was no errors from modprobe. >> >> For finit_module we could put a flag to trigger this behavior and >> propagate it to modprobe, but this is not possible with init_module(). >> I can't think in any other option right now... do you have any? > > Have a different finit_module return value for "successfully loaded, > but there were warnings" perhaps? Never mind. I was thinking that finit_module was new in 3.9. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ben Hutchings <bhutchings@solarflare.com> writes: > This should also go to stable, so the downgrading issue doesn't continue > to bite people. Andy was complaining about experimental params going away: I haven't heard a single complaint about the downgrading issue. I think it's a nice to have, which is why I mentioned it. I also CC'd the two maintainers most likely to know if it *is* a current problem. And note that this code has been this way for over ten years! No bug report, no cc:stable. What if people were relying on detecting module parameters by the load failing? I'd rather find out when they upgrade to a new kernel instead of poisoning the old ones, too. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lucas De Marchi <lucas.demarchi@profusion.mobi> writes: > Hi Rusty, > > On Mon, Mar 18, 2013 at 11:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Andy Lutomirski <luto@amacapital.net> writes: >>> On Sun, Mar 17, 2013 at 7:24 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>> Andy Lutomirski <luto@amacapital.net> writes: >>>>> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>>>> Err, yes. Don't remove module parameters, they're part of the API. Do >>>>>> you have a particular example? >>>>> >>>>> So things like i915.i915_enable_ppgtt, which is there to enable >>>>> something experimental, needs to stay forever once the relevant >>>>> feature becomes non-experimental and non-optional? This seems silly. >> ... >>>>> Having the module parameter go away while still allowing the module to >>>>> load seems like a good solution (possibly with a warning in the logs >>>>> so the user can eventually delete the parameter). >>>> >>>> Why not do that for *every* missing parameter then? Why have this weird >>>> notation where the user must know that the parameter might one day go >>>> away? >>> >>> Fair enough. What about the other approach, then? Always warn if an >>> option doesn't match (built-in or otherwise) but load the module >>> anyways. >> >> What does everyone think of this? Jon, Lucas, does this match your >> experience? >> >> Thanks, >> Rusty. >> >> Subject: modules: don't fail to load on unknown parameters. >> >> Although parameters are supposed to be part of the kernel API, experimental >> parameters are often removed. In addition, downgrading a kernel might cause >> previously-working modules to fail to load. > > I agree with this reasoning > >> >> On balance, it's probably better to warn, and load the module anyway. > > However loading the module anyway would bring at least one drawback: > if the user made a typo when passing the option the module would load > anyway and he will probably not even look in the log, since there's > was no errors from modprobe. > > For finit_module we could put a flag to trigger this behavior and > propagate it to modprobe, but this is not possible with init_module(). > I can't think in any other option right now... do you have any? No good ones :( MODULE_PARM_DESC isn't compulsory, so you can't rely on that to tell you about option names. Even if we had a flag, how would you know to set it? I guess you could try without then try with, and if it works the second time print a warning about typos. But it's still pretty ugly. We could implement such a flag with a fake "IGNORE_BAD_PARAMS" parameter, for example. That would fail nicely on older kernels, too. Hmmm.... Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/module.c b/kernel/module.c index 3c2c72d..46db10a 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3206,6 +3206,17 @@ out: return err; } +static int unknown_module_param_cb(char *param, char *val, const char *modname) +{ + /* Check for magic 'dyndbg' arg */ + int ret = ddebug_dyndbg_module_param_cb(param, val, modname); + if (ret != 0) { + printk(KERN_WARNING "%s: unknown parameter '%s' ignored\n", + modname, param); + } + return 0; +} + /* Allocate and load the module: note that size of section 0 is always zero, and we rely on this for optional sections. */ static int load_module(struct load_info *info, const char __user *uargs, @@ -3292,7 +3303,7 @@ static int load_module(struct load_info *info, const char __user *uargs, /* Module is ready to execute: parsing args may do that. */ err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, - -32768, 32767, &ddebug_dyndbg_module_param_cb); + -32768, 32767, unknown_module_param_cb); if (err < 0) goto bug_cleanup;