usb: replace __setup("nousb") with __module_param_call

usb: replace __setup("nousb") with __module_param_call

[Pete Zaitcev]

Fedora users complain that passing "nousbstorage" to the installer causes
the rest of the USB support to disappear. The installer uses kernel command
line as a way to pass options through Syslinux. The problem stems from the
use of strncmp() in obsolete_checksetup().

I used __module_param_call() instead of module_param because I wanted to
preserve the old syntax in grub.conf, and it's the only macro which allows
to remove the prefix.

The fix is tested to accept the option "nousb" correctly now.

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

---

--- linux-2.6.14/drivers/usb/core/usb.c	2005-10-28 19:12:01.000000000 -0700
+++ linux-2.6.14-lem/drivers/usb/core/usb.c	2005-12-20 10:53:21.000000000 -0800
@@ -54,7 +54,6 @@
 const char *usbcore_name = "usbcore";
 
 static int nousb;	/* Disable USB when built into kernel image */
-			/* Not honored on modular build */
 
 static DECLARE_RWSEM(usb_all_devices_rwsem);
 
@@ -1455,18 +1454,8 @@
 	.resume =	usb_generic_resume,
 };
 
-#ifndef MODULE
-
-static int __init usb_setup_disable(char *str)
-{
-	nousb = 1;
-	return 1;
-}
-
 /* format to disable USB on kernel command line is: nousb */
-__setup("nousb", usb_setup_disable);
-
-#endif
+__module_param_call("", nousb, param_set_bool, param_get_bool, &nousb, 0444);
 
 /*
  * for external read access to <nousb>

Re: usb: replace __setup("nousb") with __module_param_call

[Dmitry Torokhov]

On Tuesday 20 December 2005 17:15, Pete Zaitcev wrote:
> Fedora users complain that passing "nousbstorage" to the installer causes
> the rest of the USB support to disappear. The installer uses kernel command
> line as a way to pass options through Syslinux. The problem stems from the
> use of strncmp() in obsolete_checksetup().
>

I wonder if that strncmp() should be changed into something like
this (untested):

--- work.orig/init/main.c
+++ work/init/main.c
@@ -167,7 +167,7 @@ static int __init obsolete_checksetup(ch
 	p = __setup_start;
 	do {
 		int n = strlen(p->str);
-		if (!strncmp(line, p->str, n)) {
+		if (!strncmp(line, p->str, n) && !isalnum(line[n])) {
 			if (p->early) {
 				/* Already done in parse_early_param?  (Needs
 				 * exact match on param part) */


-- 
Dmitry

Re: usb: replace __setup("nousb") with __module_param_call

[Pete Zaitcev]

On Thu, 22 Dec 2005 01:10:52 -0500, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> On Tuesday 20 December 2005 17:15, Pete Zaitcev wrote:

> > Fedora users complain that passing "nousbstorage" to the installer causes
> > the rest of the USB support to disappear. The installer uses kernel command
> > line as a way to pass options through Syslinux. The problem stems from the
> > use of strncmp() in obsolete_checksetup().

> I wonder if that strncmp() should be changed into something like
> this (untested):
> 
> --- work.orig/init/main.c
> +++ work/init/main.c
> @@ -167,7 +167,7 @@ static int __init obsolete_checksetup(ch
>  	p = __setup_start;
>  	do {
>  		int n = strlen(p->str);
> -		if (!strncmp(line, p->str, n)) {
> +		if (!strncmp(line, p->str, n) && !isalnum(line[n])) {
>  			if (p->early) {

Are you sure that your fix works well in case of __setup("foo=")?
It probably breaks all of those.

-- Pete

Re: usb: replace __setup("nousb") with __module_param_call

[Dmitry Torokhov]

On Thursday 22 December 2005 03:24, Pete Zaitcev wrote:
> On Thu, 22 Dec 2005 01:10:52 -0500, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > On Tuesday 20 December 2005 17:15, Pete Zaitcev wrote:
> 
> > > Fedora users complain that passing "nousbstorage" to the installer causes
> > > the rest of the USB support to disappear. The installer uses kernel command
> > > line as a way to pass options through Syslinux. The problem stems from the
> > > use of strncmp() in obsolete_checksetup().
> 
> > I wonder if that strncmp() should be changed into something like
> > this (untested):
> > 
> > --- work.orig/init/main.c
> > +++ work/init/main.c
> > @@ -167,7 +167,7 @@ static int __init obsolete_checksetup(ch
> >  	p = __setup_start;
> >  	do {
> >  		int n = strlen(p->str);
> > -		if (!strncmp(line, p->str, n)) {
> > +		if (!strncmp(line, p->str, n) && !isalnum(line[n])) {
> >  			if (p->early) {
> 
> Are you sure that your fix works well in case of __setup("foo=")?
> It probably breaks all of those.
> 

Yes, of course you are right. What do you think about the patch below?
I think it shoudl handle the case of one option being a prefix for
another.

-- 
Dmitry

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 init/main.c |   30 ++++++++++++++++--------------
 1 files changed, 16 insertions(+), 14 deletions(-)

Index: work/init/main.c
===================================================================
--- work.orig/init/main.c
+++ work/init/main.c
@@ -160,24 +160,22 @@ static const char *panic_later, *panic_p
 
 extern struct obs_kernel_param __setup_start[], __setup_end[];
 
-static int __init obsolete_checksetup(char *line)
+static int __init obsolete_checksetup(char *line, int len)
 {
 	struct obs_kernel_param *p;
 
 	p = __setup_start;
 	do {
-		int n = strlen(p->str);
-		if (!strncmp(line, p->str, n)) {
+		if (!strncmp(line, p->str, len) && len == strlen(p->str)) {
 			if (p->early) {
-				/* Already done in parse_early_param?  (Needs
-				 * exact match on param part) */
-				if (line[n] == '\0' || line[n] == '=')
-					return 1;
+				/* Already done in parse_early_param? */
+				return 1;
 			} else if (!p->setup_func) {
-				printk(KERN_WARNING "Parameter %s is obsolete,"
-				       " ignored\n", p->str);
+				printk(KERN_WARNING
+					"Parameter %s is obsolete, ignored\n",
+					p->str);
 				return 1;
-			} else if (p->setup_func(line + n))
+			} else if (p->setup_func(line + len))
 				return 1;
 		}
 		p++;
@@ -226,21 +224,25 @@ __setup("loglevel=", loglevel);
  */
 static int __init unknown_bootoption(char *param, char *val)
 {
+	int len = strlen(param);
+
 	/* Change NUL term back to "=", to make "param" the whole string. */
 	if (val) {
 		/* param=val or param="val"? */
-		if (val == param+strlen(param)+1)
+		if (val == param + len + 1) {
 			val[-1] = '=';
-		else if (val == param+strlen(param)+2) {
+			len++;
+		} else if (val == param + len + 2) {
 			val[-2] = '=';
-			memmove(val-1, val, strlen(val)+1);
+			memmove(val - 1, val, strlen(val) + 1);
 			val--;
+			len++;
 		} else
 			BUG();
 	}
 
 	/* Handle obsolete-style parameters */
-	if (obsolete_checksetup(param))
+	if (obsolete_checksetup(param, len))
 		return 0;
 
 	/*

Re: usb: replace __setup("nousb") with __module_param_call

[Pete Zaitcev]

On Tue, 3 Jan 2006 01:47:46 -0500, Dmitry Torokhov <dtor_core@ameritech.net> wrote:

> +static int __init obsolete_checksetup(char *line, int len)
> -		int n = strlen(p->str);
> -		if (!strncmp(line, p->str, n)) {
> +		if (!strncmp(line, p->str, len) && len == strlen(p->str)) {

This looks like it should work, with the disclaimer that I am not
infallible.

But even if it does, my patch saved reading, so I think it should be
applied as well.

-- Pete

Re: usb: replace __setup("nousb") with __module_param_call

[Dmitry Torokhov]

On 1/3/06, Pete Zaitcev <zaitcev@redhat.com> wrote:
> On Tue, 3 Jan 2006 01:47:46 -0500, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> > +static int __init obsolete_checksetup(char *line, int len)
> > -             int n = strlen(p->str);
> > -             if (!strncmp(line, p->str, n)) {
> > +             if (!strncmp(line, p->str, len) && len == strlen(p->str)) {
>
> This looks like it should work, with the disclaimer that I am not
> infallible.
>

;)

> But even if it does, my patch saved reading, so I think it should be
> applied as well.

What you mean by "saved reading"?

Btw, do we really need to export "nousb" in sysfs?

--
Dmitry

Re: usb: replace __setup("nousb") with __module_param_call

[Pete Zaitcev]

On Tue, 3 Jan 2006 09:46:26 -0500, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> > But even if it does, my patch saved reading, so I think it should be
> > applied as well.
> 
> What you mean by "saved reading"?

The diffstat was almost all dashes: 13 deletions, 1 addition.

> Btw, do we really need to export "nousb" in sysfs?

Nobody would die if we didn't, but there's nothing wrong with the idea
in general. At least you'd know that the parameter was actually parsed.
I wish usb-handoff was exported similarly, because there's absolutely
no way to tell if it worked or was quietly ignored. And I abhor printks
in normal or success cases, so I do not want such indication.

-- Pete

Re: [linux-usb-devel] Re: usb: replace __setup("nousb") with __module_param_call

[Alan Stern]

On Tue, 3 Jan 2006, Pete Zaitcev wrote:

> On Tue, 3 Jan 2006 09:46:26 -0500, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > > But even if it does, my patch saved reading, so I think it should be
> > > applied as well.
> > 
> > What you mean by "saved reading"?
> 
> The diffstat was almost all dashes: 13 deletions, 1 addition.
> 
> > Btw, do we really need to export "nousb" in sysfs?
> 
> Nobody would die if we didn't, but there's nothing wrong with the idea
> in general. At least you'd know that the parameter was actually parsed.
> I wish usb-handoff was exported similarly, because there's absolutely
> no way to tell if it worked or was quietly ignored. And I abhor printks
> in normal or success cases, so I do not want such indication.

usb-handoff no longer exists.  The kernel now takes USB host controllers
away from the BIOS as soon as they are discovered.

Alan Stern

Re: [linux-usb-devel] Re: usb: replace __setup("nousb") with __module_param_call

[Dmitry Torokhov]

On 1/3/06, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> usb-handoff no longer exists.  The kernel now takes USB host controllers
> away from the BIOS as soon as they are discovered.
>

Yes! YES! YEEEEES!

*Dmitry dances and rejoices*

--
Dmitry

Re: [linux-usb-devel] Re: usb: replace __setup("nousb") with __module_param_call

[Pete Zaitcev]

On Tue, 3 Jan 2006 15:34:27 -0500 (EST), Alan Stern <stern@rowland.harvard.edu> wrote:

> > I wish usb-handoff was exported similarly, because there's absolutely
> > no way to tell if it worked or was quietly ignored. And I abhor printks
> > in normal or success cases, so I do not want such indication.
> 
> usb-handoff no longer exists.  The kernel now takes USB host controllers
> away from the BIOS as soon as they are discovered.

This is why I wrote that I wish that it was exposed or exported.
It does not matter now, but it was a good example why the general
policy of exposure may be beneficial. English is very confusing.

-- Pete

Re: [linux-usb-devel] Re: usb: replace __setup("nousb") with __module_param_call

[Alan Stern]

On Tue, 3 Jan 2006, Dmitry Torokhov wrote:

> On 1/3/06, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > usb-handoff no longer exists.  The kernel now takes USB host controllers
> > away from the BIOS as soon as they are discovered.
> >
> 
> Yes! YES! YEEEEES!
> 
> *Dmitry dances and rejoices*

It may not be totally advantageous.  Sometimes people have trouble with
system installs, when for some reason the USB HID driver doesn't work.  
If you've got a USB keyboard now you're pretty well stuck, whereas in the
past you could specify "nousb" and the BIOS would continue to drive the
keyboard.

Alan Stern

Re: [linux-usb-devel] Re: usb: replace __setup("nousb") with __module_param_call

[Dmitry Torokhov]

On 1/3/06, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 3 Jan 2006, Dmitry Torokhov wrote:
>
> > On 1/3/06, Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > usb-handoff no longer exists.  The kernel now takes USB host controllers
> > > away from the BIOS as soon as they are discovered.
> > >
> >
> > Yes! YES! YEEEEES!
> >
> > *Dmitry dances and rejoices*
>
> It may not be totally advantageous.  Sometimes people have trouble with
> system installs, when for some reason the USB HID driver doesn't work.
> If you've got a USB keyboard now you're pretty well stuck, whereas in the
> past you could specify "nousb" and the BIOS would continue to drive the
> keyboard.
>

Ok, I'd settle with having "usb-handoff" option that defaults to 1. I
think if you look at number of problem reports that go away with
"usb-handoff" it is sensible default.

--
Dmitry