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

[Jeff Garzik <jgarzik@pobox.com>]

viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Oct 20, 2004 at 10:37:10PM -0400, Jeff Garzik wrote:
> 
>>viro@parcelfarce.linux.theplanet.co.uk 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

Sure:  libata is a library, so all functions are helpers.  It's just one 
more helper function.


> case.  What you have is different, though - it's a single function that does
> entire ->remove() for all (AFAICS) SATA drivers.

That's intentional, see below.


> 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...

Ah the perils of an undocumented API :)  You're misunderstanding 
->port_stop.

That's a bug in ahci:  port_stop should never touch the hardware. 
port_stop is only for releasing per-driver resources like kmalloc or DMA 
memory.

Note...  another thing to keep in mind is that all libata drivers use 
ata_pci_remove_one() because that makes it possible to smooth over the 
differences between 2.4 and 2.6 scsi 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.
[...]
> 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...

I don't see any problems, given what I just wrote above.

Just the annoyance of individually mapping and unmapping 4 or 5 PCI 
BARs, and mixing 4 ranges of ISA legacy ioports for good measure.


Now...  addressing the overall theme of your message...  eventually 
libata wants to move to a strict port_{start,stop}, host_{start,stop} 
mechanism where the driver does more of the heavy lifting [by providing 
hooks that call libata helpers, rather than a helper calling hooks as 
ata_pci_remove_one does now].

But to get there will take _many_ iterations, since two things get in 
the way there:
* 2.4 compat
* the necessity to issue several ATA commands before we can respond to 
-any- SCSI commands

	Jeff