Re: Linux v2.6.9... (compile stats)

[viro]

On Wed, Oct 20, 2004 at 10:37:10PM -0400, Jeff Garzik wrote:
> viro@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx wrote:
> >IDGI.  Why do you insist on releasing these guys in library code?  Even
> 
> Because there are two distinct and separate models of port mapping/usage:
> 
> 1) A bunch of separate IO address spaces (PIO).  The "mapping" is 
> currently done in ata_pci_init_native_mode() and ata_pci_init_legacy_mode()
> 
> 2) One single linear address space (MMIO).  The mapping is done in the 
> low-level driver.
> 
> #1 is in the library because the logic is duplicated _precisely_, across 
> multiple host controllers, according to a hardware specification.
> 
> Thus, if the mapping is done in the library core, so should the unmapping.

Not really.  You are making the case for having a helper that would unmap
for case 1 and having it in the library, just as we do for mapping in that
case.  What you have is different, though - it's a single function that does
entire ->remove() for all (AFAICS) SATA drivers.

And that's where the problem is - decision on releasing resource should belong
to the driver; sure, it can and should use library helper, just as it did
when it was grabbing these resources.

Note, BTW, that current ata_pci_remove_one() is begging for trouble - for
one thing, it does iounmap() before we get to ata_scsi_release(), i.e.
ata_host_remove(), i.e. ->port_stop().   And the first look at the drivers
that provide ->port_stop() shows that ahci_port_stop() does readl()/writel()
on the ->mmio_base.  Oops...

free_irq() also looks fishy, BTW.  How about moving all that group past the
point where you are done with individual ports and merging it (and any other
unmapping we might want to do) into a single callback?  Depending on whether
->host_stop() is really needed early we might use ->host_stop for that...

Comments?


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/