On Saturday 22 November 2008 05:04:03 Greg KH wrote:
> On Fri, Nov 21, 2008 at 09:28:51PM +1030, Rusty Russell wrote:
> > Greg, here's the complete patch I have now:
> >
> > Subject: USB: Use core_param.
> >
> > Found this when I changed args to __module_param_call. We now have
> > core_param for exactly this.
> >
> > This reverts to the 2005 (pre- aafbf24a) behaviour where "nousb" was
> > not a module parameter, just a kernel command line parameter. That's
> > more sensible anyway.
>
...
> No, we need to keep that module parameter please, some distros and users
> rely on it.
Fair enough. Patch below does this as moduleparam.h suggests.
It still means that the paremeter appears in
/sys/module/kernel/parameters/nousb OR
/sys/module/usbcore/parameters/nousb.
FYI, if Pete had discovered this __setup issue today, the correct fix would
be:
1) core_param(nousb) for backwards compat.
2) module_param(disable) for modern users who want module/in-built symmetry
(ie. boot cmdline "usbcore.disable", and "modprobe usbcore disable")
USB: Don't use __module_param_call
Found this when I changed args to __module_param_call. We now have
core_param for exactly this, but Greg assures me "nousb" is used as a
module parameter, using the method suggested in moduleparam.h will
have to do.
Signed-off-by: Rusty Russell <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Pete Zaitcev <[email protected]>
---
drivers/usb/core/usb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -962,9 +962,6 @@ void usb_buffer_unmap_sg(const struct us
}
EXPORT_SYMBOL_GPL(usb_buffer_unmap_sg);
-/* format to disable USB on kernel command line is: nousb */
-__module_param_call("", nousb, param_set_bool, param_get_bool, &nousb, 0444);
-
/*
* for external read access to <nousb>
*/
@@ -1052,6 +1049,11 @@ static void __exit usb_exit(void)
ksuspend_usb_cleanup();
}
+/* To disable USB, kernel command line is 'nousb' not 'usbcore.nousb' */
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX
+module_param(nousb, bool, 0444);
+
subsys_initcall(usb_init);
module_exit(usb_exit);
MODULE_LICENSE("GPL");
On Sat, Nov 22, 2008 at 01:01:06PM +1030, Rusty Russell wrote:
> On Saturday 22 November 2008 05:04:03 Greg KH wrote:
> > On Fri, Nov 21, 2008 at 09:28:51PM +1030, Rusty Russell wrote:
> > > Greg, here's the complete patch I have now:
> > >
> > > Subject: USB: Use core_param.
> > >
> > > Found this when I changed args to __module_param_call. We now have
> > > core_param for exactly this.
> > >
> > > This reverts to the 2005 (pre- aafbf24a) behaviour where "nousb" was
> > > not a module parameter, just a kernel command line parameter. That's
> > > more sensible anyway.
> >
> ...
> > No, we need to keep that module parameter please, some distros and users
> > rely on it.
>
> Fair enough. Patch below does this as moduleparam.h suggests.
>
> It still means that the paremeter appears in
> /sys/module/kernel/parameters/nousb OR
> /sys/module/usbcore/parameters/nousb.
What's the "OR" part? What determines where it goes?
> FYI, if Pete had discovered this __setup issue today, the correct fix would
> be:
> 1) core_param(nousb) for backwards compat.
> 2) module_param(disable) for modern users who want module/in-built symmetry
> (ie. boot cmdline "usbcore.disable", and "modprobe usbcore disable")
>
>
> USB: Don't use __module_param_call
>
> Found this when I changed args to __module_param_call. We now have
> core_param for exactly this, but Greg assures me "nousb" is used as a
> module parameter, using the method suggested in moduleparam.h will
> have to do.
Is there a real reason why we need to change this at all?
> +/* To disable USB, kernel command line is 'nousb' not 'usbcore.nousb' */
> +#undef MODULE_PARAM_PREFIX
> +#define MODULE_PARAM_PREFIX
> +module_param(nousb, bool, 0444);
That undef seems hacky beyond belief. How would one know to do this?
thanks,
greg k-h
On Fri, 21 Nov 2008 22:41:32 -0800, Greg KH <[email protected]> wrote:
> > FYI, if Pete had discovered this __setup issue today, the correct fix would
> > be:
> > 1) core_param(nousb) for backwards compat.
> > 2) module_param(disable) for modern users who want module/in-built symmetry
> > (ie. boot cmdline "usbcore.disable", and "modprobe usbcore disable")
> Is there a real reason why we need to change this at all?
As far as I know, in Fedora "nousb" is never used as a module parameter.
It came about because interrupt tables are sometimes bad and once
a request_irq() is done in an HCD, there's an interrupt storm. So,
the "nousb" is mostly used in the SysLinux's command line, rarely
in the real command line, and never in /etc/modprobe.conf.
I'm adding Jeremy to cc: for confirmation.
I think it would be good to try and drop "nousb" option in Fedora 11.
The conflict with "nousbstorage" would resolve itself too if we do.
-- Pete
On Saturday 22 November 2008 17:11:32 Greg KH wrote:
> On Sat, Nov 22, 2008 at 01:01:06PM +1030, Rusty Russell wrote:
> > Fair enough. Patch below does this as moduleparam.h suggests.
> >
> > It still means that the paremeter appears in
> > /sys/module/kernel/parameters/nousb OR
> > /sys/module/usbcore/parameters/nousb.
>
> What's the "OR" part? What determines where it goes?
Whether usbcore is a module or not, of course.
> > +/* To disable USB, kernel command line is 'nousb' not 'usbcore.nousb' */
> > +#undef MODULE_PARAM_PREFIX
> > +#define MODULE_PARAM_PREFIX
> > +module_param(nousb, bool, 0444);
>
> That undef seems hacky beyond belief.
*Exactly*. And while you're not the first person to do this, you're the one
one to use it for code which can be a module :(
> How would one know to do this?
The same way Pete found __module_param_call, by reading the header:
/* You can override this manually, but generally this should match the
module name. */
#ifdef MODULE
#define MODULE_PARAM_PREFIX /* empty */
#else
#define MODULE_PARAM_PREFIX KBUILD_MODNAME "."
#endif
Cheers,
Rusty.
On Sunday 23 November 2008 03:59:52 Pete Zaitcev wrote:
> On Fri, 21 Nov 2008 22:41:32 -0800, Greg KH <[email protected]> wrote:
> > > FYI, if Pete had discovered this __setup issue today, the correct fix
> > > would be:
> > > 1) core_param(nousb) for backwards compat.
> > > 2) module_param(disable) for modern users who want module/in-built
> > > symmetry (ie. boot cmdline "usbcore.disable", and "modprobe usbcore
> > > disable")
> >
> > Is there a real reason why we need to change this at all?
You're the only __module_param_call user outside moduleparam.h, and so an
unrelated patch broke USB. :(
> As far as I know, in Fedora "nousb" is never used as a module parameter.
> It came about because interrupt tables are sometimes bad and once
> a request_irq() is done in an HCD, there's an interrupt storm. So,
> the "nousb" is mostly used in the SysLinux's command line, rarely
> in the real command line, and never in /etc/modprobe.conf.
> I'm adding Jeremy to cc: for confirmation.
>
> I think it would be good to try and drop "nousb" option in Fedora 11.
> The conflict with "nousbstorage" would resolve itself too if we do.
My original patch simply turned it into a core_param (ie. not valid as a
module parameter). If this had existed in 2005, it would have been the right
answer.
If there's any chance of it actually breaking, let's leave it as a module
parameter too (which is what the patch I sent does).
Thanks,
Rusty.
On Fri, 21 Nov 2008 22:41:32 -0800, Greg KH <[email protected]> wrote:
> > > > This reverts to the 2005 (pre- aafbf24a) behaviour where "nousb" was
> > > > not a module parameter, just a kernel command line parameter. That's
> > > > more sensible anyway.
By the way, I floated the idea to remove "nousb" altogether on fedora-devel.
Alan Cox replied that he has an actual box that requires it,
because of a bug in SMM BIOS... Imagine my surprise. I think we're
stuck with "nousb" for a little while still.
-- Pete