On Sun, 20 Feb 2000, Jeff Garzik wrote:
> Very good spotting. I have read the code for pci_[un]register_driver
> several times, but this did not occur to me. However I do not agree
> with the patch as presented...
>
> This is what I have in 8139too now, before your bugfix...
>
> /* module stays resident iff CONFIG_HOTPLUG and compiled into kernel */
> #if !defined(CONFIG_HOTPLUG) || defined(MODULE)
> return (rc > 0) ? 0 : (rc == 0 ? -ENODEV : rc);
> #else
> return (rc >= 0) ? 0 : rc;
> #endif
>
> Adding your bugfix to this junk will make things even uglier, so I think
> that encapsulating all this behavior into a macro of some sort would be
> good...
[I'm not familiar with an 8139too version more recent than what is in
2.3.46. Is rc still the return value of pci_register_driver()? Then you
can simplify things as rc will always be >= 0.]
Anyway, a generic solution looks like this:
---/* module stays resident iff CONFIG_HOTPLUG and compiled into kernel */ #if !defined(CONFIG_HOTPLUG) || defined(MODULE) rc = (rc > 0) ? 0 : (rc == 0 ? -ENODEV : rc); #else rc = (rc >= 0) ? 0 : rc; #endif
if (rc < 0) /* Cleanup if we encountered an error... */ pci_unregister_driver (&rtl8139_pci_driver);
return rc;
---
Simple, cleaner and much more capable than a macro approach. Think of a driver that calls some more functions that might fail in its init routine. The driver writer has to remember the call to pci_unregister_driver() anyway in the error path. So I don't think it's wise to hide this call for one failure case only. Let's just use the clean logic in current drivers so future driver writers have an easy to understand scheme to adopt.
Regards,
Daniel.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed Feb 23 2000 - 21:00:26 EST