On Thu, Jul 26, 2007 at 03:31:21PM -0400, Jeff Garzik wrote:
> 1) you should propagate pci_enable_device return value to caller on error
ok. I'll return -ENODEV if I don't get a Scsi_Host back, though.
Unless you think it's worth going through the PTR_ERR/IS_ERR/ERR_PTR
hooplah. It's not like this is "what's scrogging my filesystem", it's
"my driver didn't load".
> 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.
> 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.
> 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 ;-)
> 5) call pci_set_drvdata(pdev, NULL) as with other PCI drivers
Why?
> 6) suggestions: tab alignment for struct member values; makes it far
> easier to read.
ok
> 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.
> >+ 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.
--
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
-
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
