Linux Kernel Mailing List wrote:
> ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, [email protected]
>
> [PATCH] cpqarray update
>
> Yes, we should. I usually ask Mike Miller in our group to send my
> patches since he's been doing that and more known in the community, but
> since you already got a hold of it ...yes, please :)
>
> CHANGELOG:
>
> * Fix for eisa card not detecting partitions properly
> * Use pci_module_init instead of pci_register_device to handle hotplug
> scenario and unregister if the driver can't find pci controller
> * Add BLKSSZGET ioctl
[...]
> @@ -616,7 +623,7 @@
>
> /* detect controllers */
> printk(DRIVER_NAME "\n");
> - pci_register_driver(&cpqarray_pci_driver);
> + pci_module_init(&cpqarray_pci_driver);
> cpqarray_eisa_detect();
>
> for(i=0; i< MAX_CTLR; i++) {
You need to check the return value of pci_module_init() for errors.
Jeff
On Jan 26, 2004, at 2:15 PM, Jeff Garzik wrote:
> Linux Kernel Mailing List wrote:
>> ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, [email protected]
>> @@ -616,7 +623,7 @@
>> /* detect controllers */
>> printk(DRIVER_NAME "\n");
>> - pci_register_driver(&cpqarray_pci_driver);
>> + pci_module_init(&cpqarray_pci_driver);
>> cpqarray_eisa_detect();
>> for(i=0; i< MAX_CTLR; i++) {
>
> You need to check the return value of pci_module_init() for errors.
I'm defining a new bus and had copied pci_module_init() to
vio_module_init(). Here's what Greg KH had to say about that:
> Eeek! I want to fix that code in pci_module_init() so it doesn't do
> this at all. Please don't copy that horrible function. Just register
> the driver with a call to vio_register_driver() and drop the whole
> vio_module_init() completly. I'll be doing that for pci soon, and
> there's no reason you want to duplicate this broken logic (you always
> want your module probe to succeed, for lots of reasons...)
So there's no need for the quoted patch hunk at all.
--
Hollis Blanchard
IBM Linux Technology Center
On Wed, Jan 28, 2004 at 09:46:06AM -0600, Hollis Blanchard wrote:
> On Jan 26, 2004, at 2:15 PM, Jeff Garzik wrote:
>
> >Linux Kernel Mailing List wrote:
> >>ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, [email protected]
> >>@@ -616,7 +623,7 @@
> >> /* detect controllers */
> >> printk(DRIVER_NAME "\n");
> >>- pci_register_driver(&cpqarray_pci_driver);
> >>+ pci_module_init(&cpqarray_pci_driver);
> >> cpqarray_eisa_detect();
> >> for(i=0; i< MAX_CTLR; i++) {
> >
> >You need to check the return value of pci_module_init() for errors.
>
> I'm defining a new bus and had copied pci_module_init() to
> vio_module_init(). Here's what Greg KH had to say about that:
> >Eeek! I want to fix that code in pci_module_init() so it doesn't do
> >this at all. Please don't copy that horrible function. Just register
> >the driver with a call to vio_register_driver() and drop the whole
> >vio_module_init() completly. I'll be doing that for pci soon, and
> >there's no reason you want to duplicate this broken logic (you always
> >want your module probe to succeed, for lots of reasons...)
>
> So there's no need for the quoted patch hunk at all.
Well, changing it back to pci_register_driver() and actually checking
the return value would be a good idea :)
thanks,
greg k-h
Jeff Garzik wrote:
> Actually I disagree with GregKH on this.
>
> The register/unregister functions need to be returning error codes,
> _not_ the count of interfaces registered. It is trivial to count the
> registered interfaces in the driver itself, but IMO far more important
> to propagate fatal errors back to the original caller.
Nevermind, this got fixed. I'm still worried about the '1' return
value, though, for zero controllers found.
Jeff
Hollis Blanchard wrote:
> I'm defining a new bus and had copied pci_module_init() to
> vio_module_init(). Here's what Greg KH had to say about that:
>
>> Eeek! I want to fix that code in pci_module_init() so it doesn't do
>> this at all. Please don't copy that horrible function. Just register
>> the driver with a call to vio_register_driver() and drop the whole
>> vio_module_init() completly. I'll be doing that for pci soon, and
>> there's no reason you want to duplicate this broken logic (you always
>> want your module probe to succeed, for lots of reasons...)
Actually I disagree with GregKH on this.
The register/unregister functions need to be returning error codes,
_not_ the count of interfaces registered. It is trivial to count the
registered interfaces in the driver itself, but IMO far more important
to propagate fatal errors back to the original caller.
This is what pci_module_init() does... returns an error if an error
occurred, zero if not. Further, use of pci_module_init() makes drivers
future-proof for a time when the API can better return fatal errors that
occur during the registration process.
As it stands now, pci_register_driver() -can- call functions which can
fail internally (see what driver_register calls...), unrelated to the
driver, and the driver will never ever know this.
That is an ugly flaw in most current foo_register_driver() APIs. Errors
are silently lost :(
Jeff
On Wed, Jan 28, 2004 at 06:44:32PM -0500, Jeff Garzik wrote:
> Jeff Garzik wrote:
> >Actually I disagree with GregKH on this.
> >
> >The register/unregister functions need to be returning error codes,
> >_not_ the count of interfaces registered. It is trivial to count the
> >registered interfaces in the driver itself, but IMO far more important
> >to propagate fatal errors back to the original caller.
>
> Nevermind, this got fixed. I'm still worried about the '1' return
> value, though, for zero controllers found.
Yeah, I don't really like it either, but figured it was a 2.7 task to
clean it up properly.
thanks,
greg k-h