Re: [PATCH 05/17] libata: implement PCI init helpers for new LLD init model

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> This is definitely the wrong direction.
> 
> Ouch...  The whole patch?
> 
>> We don't want to keep crowding knowledge of multiple bus technologies 
>> into the same function.
> 
> Do you mean PCI-IDE (legacy/BMDMA), PCI-native and newer controllers 
> (including MSI stuff)?  Bus-wise they're all PCI.

ata_generic, ISA, VLB, and embedded controllers that use libata are not 
PCI.  libata-bmdma can drive older non-PCI controllers.

Its a bit confusing because "PCI IDE" is often used to refer to the 
legacy IDE shadow register interface.  I started to use "bmdma" to 
describe the interface, but it drives Alan crazy :)


>> ata_pci_request_irq() and other code above follows the same theme... 
>> but its an unmaintainable direction.
>>
>> This sort of stuff needs to be split up, not coalesced.
> 
> ata_pci_*() functions are PCI helpers and I wanted to move most of PCI 
> resource bookkeeping into helpers.  Less clutter in LLDs and less errors.

Putting legacy IDE resource allocation and ultra-modern (MSI) resource 
allocation in the same function is the wrong direction.

Legacy LLDDs should not be traversing code that has to worry about PCI 
MSI, and PCI MSI controllers should not be traversing code paths that 
deal with legacy IDE.

I'm looking forward to having ARM and PowerMac IDE drivers ported to 
libata soon, and we don't need to be adding special ARM/PPC bus handling 
stuff to these already-gigantic resource routines.


>> Another thing to think about:  IMO it makes sense to separate out the 
>> PCI IDE resource handling, because that set of technology is largely 
>> static.
>  >
>> Most newer controllers will only have a few resources, normally MMIO, 
>> and may even support MSI-X (multiple messages for different event 
>> types, rather than a single message for all events like MSI).
>>
>> So, I'd like to see some of this inside libata-bmdma.c to keep the 
>> core free of such nastiness.
> 
> I'm having a little bit difficult time following what you're thinking. 
> Whatever the final interface is, the goals would be similar to what's 
> listed in the head message of this patchset, and I think 
> alloc/init/attach model is a good way to achieve that thus killing 
> probe_ent.  Can you explain in more detail how the interface should look 
> like?

Killing probe_ent is a good thing, definitely.  Ideally we should have 
an interface for the LLDD like

	host_set = alloc_ata()
	[ fill in host_set ]
	rc = register_ata()
	...
	unregister_ata()
	free_ata()

Additionally, to take into account strange resource allocation stuff, 
BMDMA drivers should eventually do

	host_set = alloc_bmdma()
	[ fill in host_set ]
	rc = register_bmdma()
	...
	unregister_bmdma()
	free_bmdma()

That keeps all the legacy IDE crap out of the core completely.

Overall, your movement towards killing probe_ent and filling in host_set 
directly is a good one, as I illustrate above.  But lumping all the 
resource allocation and irq registration into a few big functions is the 
wrong direction.  Consider for example an LLDD that supports MSI-X in 
your design.  That's a good proof of concept.

	Jeff