Re: [PATCH] Don't call request_region() for 3C90x chips

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Jeff Garzik wrote:

>>>> It's generally not a good idea to call request_region() on an 
>>>> address returned by pci_iomap(), even less so on a MMIO address. And 
>>>> there was absolutely no point in claiming the region already claimed 
>>>> by the PCI core, especially with the same PCI generic owner's name. 
>>>> As this is the only case of the must_free_region flag being set, 
>>>> this flag may go away as well...

>>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>> I agree you have identified a bug, but this is not a solution.

>>> The current driver bug is that it calls request_region() potentially 
>>> on an MMIO address, but the solution is _not_ to completely avoid 
>>> reserving the resource.

>>    It's not even a MMIO/PIO address anymore after pci_iomap() -- it 
>> either went thru ioremap() or ioport_map() which both change the 
>> mapping from the physical to the virtual address (or some equivalent 
>> of it for I/O ports).

> Yes.  _Obviously_ you must reserve the resource passed to 
> pci_iomap/ioremap, not the cookie returned by such.

    What somewhat puzzled me is the words "Cardbus drivers already allocate 
for us" in the current driver's vortex_probe1(). What extra drivers, and why 
should they call request_region() for us? :-/

>>> The region registered with the PCI core, but _not_ claimed by anyone. 
>>> Someone still needs to either call pci_{request,release}_regions() or 
>>> request_[mem_]region() to indicate that the resource is reserved.

>>    Sigh, it seems I've missed that difference. So, I'll recast...

> IMO it would be easiest to do pci_{request,release}_regions() in the 
> PCI-only code.  I believe this matches up well with the existing 
> EISA-specific code, which also performs request_region().

    Ugh, I've looked at vortex_remove_one() and found another buglet: they're 
trying to reset the chip there... after calling pci_disable_device(). Guess 
whether they really reset anything. I wonder whether it's accpetable for this 
fix to be put in the same patch...

>     Jeff

WBR, Sergei