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

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

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Wed, 20 Oct 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
>>>   drivers/scsi/pcmcia: 3 warnings, 0 errors
>>>   drivers/scsi: 148 warnings, 0 errors
>>
>>Mostly dealt with, but I'm still messing with SATA parts.
> 
> 
> Jeff had SATA patches - it needs to use the new iomap interfaces, and then
> it's much cleaner. I tested that his patches worked for me several weeks
> ago, but nor all architectures had the iomap interface, so I assume Jeff
> wasn't very eager to push it out.
> 
> Anyway, Al, talk to Jeff. Jeff?


Current patch is at
http://www.kernel.org/pub/linux/kernel/people/jgarzik/libata/patch.iomap.bz2

I still merging stuff, so won't get around to it for another day or so :)

I certainly don't mind anyone stealing the task from me, but the effort 
is larger than the other iomap conversions.  The patch above hits all 
the easily-picked fruit, leaving the stuff that requires a modicum of 
effort:

* map/unmap N PCI bars (N >= 4, per controller)
* map/unmap 2 ISA I/O regions (0x170, 0x1f0)
* accurately handle the odd situation where IDE driver steals 0x170 
while libata steals 0x1f0 (or vice versa), a.k.a. the reason for 
quirk_intel_ide_combined() and the ____request_resource nastiness

Currently the code is set up to handle:
* N PIO ports
	or
* a single MMIO address that contains all the registers the driver needs
(mmio_base)

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

[viro]

On Wed, Oct 20, 2004 at 08:29:59PM -0400, Jeff Garzik wrote:
> Current patch is at
> http://www.kernel.org/pub/linux/kernel/people/jgarzik/libata/patch.iomap.bz2

Got it.
 
> I still merging stuff, so won't get around to it for another day or so :)
> 
> I certainly don't mind anyone stealing the task from me, but the effort 
> is larger than the other iomap conversions.  The patch above hits all 
> the easily-picked fruit, leaving the stuff that requires a modicum of 
> effort:

Hey, it's not that there wasn't enough work in other places...  I've
picked the patch above for -bird and will happily leave further sata
stuff to you, TYVM ;-)

Speaking of which, since -bk5 is out there now...  Could you drop the delta
between it and your current netdev-2.6 somewhere on anonftp?  AFAICS there
are 6 patches missing from the pile I've sent to you (3 out of them with
objections I've seen + 1 you've asked to postpone), but there's another
pile waiting to be sent (11 more)...

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

[viro]

On Wed, Oct 20, 2004 at 08:29:59PM -0400, Jeff Garzik wrote:
> I still merging stuff, so won't get around to it for another day or so :)
> 
> I certainly don't mind anyone stealing the task from me, but the effort 
> is larger than the other iomap conversions.  The patch above hits all 
> the easily-picked fruit, leaving the stuff that requires a modicum of 
> effort:
> 
> * map/unmap N PCI bars (N >= 4, per controller)
> * map/unmap 2 ISA I/O regions (0x170, 0x1f0)
> * accurately handle the odd situation where IDE driver steals 0x170 
> while libata steals 0x1f0 (or vice versa), a.k.a. the reason for 
> quirk_intel_ide_combined() and the ____request_resource nastiness
> 
> Currently the code is set up to handle:
> * N PIO ports
> 	or
> * a single MMIO address that contains all the registers the driver needs
> (mmio_base)

Hmm...  It misses a bunch of easy stuff, actually (tons of casts to void *
from what used to be unsigned long and is void __iomem * with your patch).

I don't see where you handle PIO stuff, though - no ioport_map() _or_
pci_iomap() in sight.  Note that ioport_map() is not equivalent to a cast -
we add a constant there.  How does ioread/iowrite work on the results?

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

[Jeff Garzik]

viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Oct 20, 2004 at 08:29:59PM -0400, Jeff Garzik wrote:
> 
>>I still merging stuff, so won't get around to it for another day or so :)
>>
>>I certainly don't mind anyone stealing the task from me, but the effort 
>>is larger than the other iomap conversions.  The patch above hits all 
>>the easily-picked fruit, leaving the stuff that requires a modicum of 
>>effort:
>>
>>* map/unmap N PCI bars (N >= 4, per controller)
>>* map/unmap 2 ISA I/O regions (0x170, 0x1f0)
>>* accurately handle the odd situation where IDE driver steals 0x170 
>>while libata steals 0x1f0 (or vice versa), a.k.a. the reason for 
>>quirk_intel_ide_combined() and the ____request_resource nastiness
>>
>>Currently the code is set up to handle:
>>* N PIO ports
>>	or
>>* a single MMIO address that contains all the registers the driver needs
>>(mmio_base)
> 
> 
> Hmm...  It misses a bunch of easy stuff, actually (tons of casts to void *
> from what used to be unsigned long and is void __iomem * with your patch).

feel free to send a delta :)


> I don't see where you handle PIO stuff, though - no ioport_map() _or_
> pci_iomap() in sight.

Correct, that part doesn't exist yet.  grep in the above quoted text for 
"* map/unap" for the to-do list.

The mapping of the PIO PCI BARs requires independently mapping at least 
5 (but varies from controller to controller) IO port ranges, and 
tracking those mappings in a coherent manner.

	Jeff

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

[viro]

On Wed, Oct 20, 2004 at 09:59:47PM -0400, Jeff Garzik wrote:
> >Hmm...  It misses a bunch of easy stuff, actually (tons of casts to void *
> >from what used to be unsigned long and is void __iomem * with your patch).
> 
> feel free to send a delta :)
 
Will do.

> >I don't see where you handle PIO stuff, though - no ioport_map() _or_
> >pci_iomap() in sight.
> 
> Correct, that part doesn't exist yet.  grep in the above quoted text for 
> "* map/unap" for the to-do list.
> 
> The mapping of the PIO PCI BARs requires independently mapping at least 
> 5 (but varies from controller to controller) IO port ranges, and 
> tracking those mappings in a coherent manner.

IDGI.  Why do you insist on releasing these guys in library code?  Even
with iomem case you are creating mappings in driver code, so the reverse
should also be done there...

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

[Jeff Garzik]

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.

	Jeff

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

[viro]

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

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

[Jeff Garzik]

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