[PATCH (as387)] UHCI: check return code from pci_register_driver

[PATCH (as387)] UHCI: check return code from pci_register_driver

[Alan Stern]

Greg:

This is all your fault!  :-)

The patch below fixes the problem in which the UHCI driver doesn't
properly check the return code from pci_register_driver().

Alan Stern


On Fri, 1 Oct 2004, Andrew Morton wrote:

> Greg's latest tree, on x86_64:
> 
> Badness in remove_proc_entry at fs/proc/generic.c:688
> 
> Call Trace:<ffffffff8019cfb6>{remove_proc_entry+391} <ffffffff805a981f>{uhci_hcd_init+224} 
>        <ffffffff8010c26d>{init+475} <ffffffff8010ff17>{child_rip+8} 
>        <ffffffff8010c092>{init+0} <ffffffff8010ff0f>{child_rip+0} 
> 
> 
>                 WARN_ON(de->subdir);
> 
> which is a bit weird.  How did driver/uhci get itself a subdirectory?
> 
> Maybe it already existed, and uhci_hcd_init() tried to delete it anwyay?

On Sat, 2 Oct 2004, Reuben Farrelly wrote:

> slab error in kmem_cache_destroy(): cache `uhci_urb_priv': Can't free
> all objects
>   [<c0104ddc>] dump_stack+0x17/0x19
>   [<c013dfd5>] kmem_cache_destroy+0xea/0x15b
>   [<c03e17eb>] uhci_hcd_init+0xc8/0xff
>   [<c03ca89f>] do_initcalls+0x56/0xb3 
>   [<c01004f5>] init+0x81/0x189
>   [<c01022f1>] kernel_thread_helper+0x5/0xb
> drivers/usb/host/uhci-hcd.c: not all urb_priv's were freed!
> Badness in remove_proc_entry at fs/proc/generic.c:688
>   [<c0104ddc>] dump_stack+0x17/0x19
>   [<c017c196>] remove_proc_entry+0x129/0x133
>   [<c03e1810>] uhci_hcd_init+0xed/0xff
>   [<c03ca89f>] do_initcalls+0x56/0xb3 
>   [<c01004f5>] init+0x81/0x189
>   [<c01022f1>] kernel_thread_helper+0x5/0xb



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

===== drivers/usb/host/uhci-hcd.c 1.134 vs edited =====
--- 1.134/drivers/usb/host/uhci-hcd.c	2004-09-30 13:58:40 -04:00
+++ edited/drivers/usb/host/uhci-hcd.c	2004-10-04 10:37:21 -04:00
@@ -2412,7 +2412,7 @@
 		goto up_failed;
 
 	retval = pci_register_driver(&uhci_pci_driver);
-	if (retval)
+	if (retval < 0)
 		goto init_failed;
 
 	return 0;

Re: [PATCH (as387)] UHCI: check return code from pci_register_driver

[Reuben Farrelly]

Hi,

Alan Stern wrote:
> Greg:
> 
> This is all your fault!  :-)
> 
> The patch below fixes the problem in which the UHCI driver doesn't
> properly check the return code from pci_register_driver().
> 
> Alan Stern
> 
> 
> On Fri, 1 Oct 2004, Andrew Morton wrote:
> 
> 
>>Greg's latest tree, on x86_64:
>>
>>Badness in remove_proc_entry at fs/proc/generic.c:688
>>
>>Call Trace:<ffffffff8019cfb6>{remove_proc_entry+391} <ffffffff805a981f>{uhci_hcd_init+224} 
>>       <ffffffff8010c26d>{init+475} <ffffffff8010ff17>{child_rip+8} 
>>       <ffffffff8010c092>{init+0} <ffffffff8010ff0f>{child_rip+0} 
>>
>>
>>                WARN_ON(de->subdir);
>>
>>which is a bit weird.  How did driver/uhci get itself a subdirectory?
>>
>>Maybe it already existed, and uhci_hcd_init() tried to delete it anwyay?
> 
> 
> On Sat, 2 Oct 2004, Reuben Farrelly wrote:
> 
> 
>>slab error in kmem_cache_destroy(): cache `uhci_urb_priv': Can't free
>>all objects
>>  [<c0104ddc>] dump_stack+0x17/0x19
>>  [<c013dfd5>] kmem_cache_destroy+0xea/0x15b
>>  [<c03e17eb>] uhci_hcd_init+0xc8/0xff
>>  [<c03ca89f>] do_initcalls+0x56/0xb3 
>>  [<c01004f5>] init+0x81/0x189
>>  [<c01022f1>] kernel_thread_helper+0x5/0xb
>>drivers/usb/host/uhci-hcd.c: not all urb_priv's were freed!
>>Badness in remove_proc_entry at fs/proc/generic.c:688
>>  [<c0104ddc>] dump_stack+0x17/0x19
>>  [<c017c196>] remove_proc_entry+0x129/0x133
>>  [<c03e1810>] uhci_hcd_init+0xed/0xff
>>  [<c03ca89f>] do_initcalls+0x56/0xb3 
>>  [<c01004f5>] init+0x81/0x189
>>  [<c01022f1>] kernel_thread_helper+0x5/0xb
> 
> 
> 
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ===== drivers/usb/host/uhci-hcd.c 1.134 vs edited =====
> --- 1.134/drivers/usb/host/uhci-hcd.c	2004-09-30 13:58:40 -04:00
> +++ edited/drivers/usb/host/uhci-hcd.c	2004-10-04 10:37:21 -04:00
> @@ -2412,7 +2412,7 @@
>  		goto up_failed;
>  
>  	retval = pci_register_driver(&uhci_pci_driver);
> -	if (retval)
> +	if (retval < 0)
>  		goto init_failed;
>  
>  	return 0;

Confirming fix, this solves the trace that I have been seeing upon bootup.

Reuben

Re: [PATCH (as387)] UHCI: check return code from pci_register_driver

[Greg KH]

On Mon, Oct 04, 2004 at 11:33:35AM -0400, Alan Stern wrote:
> Greg:
> 
> This is all your fault!  :-)
> 
> The patch below fixes the problem in which the UHCI driver doesn't
> properly check the return code from pci_register_driver().

Yeah, it's all my fault, what else is new...

Anyway, no, my change to the uhci (and ohci and ehci drivers) is ok,
it's just that pci_register_driver() is incorrect :)

Here's a fix for it, that lets the USB host controllers work properly.
Now PCI works like the other bus drivers.  As we had no idea of how many
devices bound to the driver, this function was just lying and returning
"1".  What a stinker.

I'll add this to my trees, and I've gone through and audited all callers
of this function to now work properly (there were some pretty strange
ideas of what to do on an error returned from this function...)

Alan, these error messages lead me to believe that the error recovery
code in the uhci driver doesn't quite work properly, as even if the
register of the pci driver fails, we shouldn't error out with this mess,
right?

thanks,

greg k-h


===== pci-driver.c 1.46 vs edited =====
--- 1.46/drivers/pci/pci-driver.c	2004-09-29 23:09:23 -07:00
+++ edited/pci-driver.c	2004-10-04 11:11:20 -07:00
@@ -396,13 +396,13 @@
  * @drv: the driver structure to register
  * 
  * Adds the driver structure to the list of registered drivers.
- * Returns a negative value on error. The driver remains registered
- * even if no device was claimed during registration.
+ * Returns a negative value on error, otherwise 0. 
+ * If no error occured, the driver remains registered even if 
+ * no device was claimed during registration.
  */
-int
-pci_register_driver(struct pci_driver *drv)
+int pci_register_driver(struct pci_driver *drv)
 {
-	int count = 0;
+	int error;
 
 	/* initialize common driver fields */
 	drv->driver.name = drv->name;
@@ -414,13 +414,12 @@
 	pci_init_dynids(&drv->dynids);
 
 	/* register with core */
-	count = driver_register(&drv->driver);
+	error = driver_register(&drv->driver);
 
-	if (count >= 0) {
+	if (!error)
 		pci_populate_driver_dir(drv);
-	}
 
-	return count ? count : 1;
+	return error;
 }
 
 /**