Matthew Wilcox wrote:
On Thu, Jul 26, 2007 at 03:31:21PM -0400, Jeff Garzik wrote:
2) you dropped the check for pci_resource_start() returning zero (look
for 'iop == 0' in original code)
That's not actually something that could happen through that path.
Er, huh? It's a value returned from the PCI device. Of course it can
happen on that path. That's the first time you look at the PCI BAR values.
3) what happened to PCI BAR #1 ? shouldn't you move that call here too?
it gets ioremapped (look at 'pci_memory_address' in old code, right
next to pci_resource_start call)
That'll move later, once I'm on the iomap path. We don't have the
structure to stash the result in at this point.
If you follow other PCI drivers, you should check it in pci_driver::probe().
4) it is often wise to add sanity checks that ensure that PCI BAR #0 ==
IORESOURCE_IO and PCI BAR #1 == IORESOURCE_MEM
Why is that? There's no danger of advansys producing any more cards ;-)
Granted it is far less important than checking pci_resource_start()==0
and pci_resource_len()==0 -- which happens often enough to care about
it. Firmware or kernel PCI allocation routines or even PCI quirks can
affect this.
Generally this is not about new cards but about misconfiguration, or
even protection from users trying to see if adding a PCI ID works for them.
But overall, it's sane to check that the resource is memory before
passing it to ioremap, so that you catch the error at the correct time.
This is standard stuff that all PCI drivers should do.
5) call pci_set_drvdata(pdev, NULL) as with other PCI drivers
Why?
So that you don't leave a dangling pointer to an object that has already
been destroyed.
Again, this is standard PCI driver practice.
7) bug: don't unregister, if pci_register_driver() failed during init
I assumed that pci_unregister_driver handled that, for the same reason that
kfree(NULL) works. Otherwise I have to keep track of that in the driver
somewhere.
Well either verify your assumption or keep track of it :)
+ for (i = 0; i < asc_board_count; i++) {
+ struct Scsi_Host *host = asc_host[i];
+ if (!host)
+ continue;
+ scsi_remove_host(host);
+ advansys_release(host);
+ asc_host[i] = NULL;
this last line of code is rather pointless, isn't it?
No ... see advansys_interrupt. Yes, that needs to be cleaned up *too*,
but I can't fix everything at once. Eventually, the asc_host array will
disappear.
Look at scsi_module.c: it is sane practice to have two loops for code
like this: the first loop calls scsi_remove_host() for all hosts, and
the second loop does the rest of the cleanup. IMO this is more
conservative, as you know the SCSI layer will not be passing anything to
any port on your card, when you start shutting it down. Makes it harder
to trigger races and bugs.
Of course, when everything is converted to EISA/PCI hotplug API, you
cannot do this anymore.
But I find it sane practice for older buses, especially.
Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
