Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI

[Chris Metcalf <cmetcalf@tilera.com>]

On 12/1/2011 5:47 PM, Arnd Bergmann wrote:
> On Thursday 01 December 2011 12:59:19 Chris Metcalf wrote:
>> The tile architecture does not support IOPORT, but it does support
>> PCI and IOMEM.  We were not specifying NO_IOPORT, though, because it
>> was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table)
>> that are needed to not get provided.  Moving the #ifdef fixes this
>> so that those functions and some related ones are now always provided
>> if CONFIG_PCI is true.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> Hmm, I'm sure that Al Viro intentionally did it the way it is, but I'm
> not sure exactly why that would be.
>
> Any driver using the functions you now expose for NO_IOPORT is probably
> prepared to handle both MMIO and PIO BARs on PCI, which you can consider
> broken in the case where a card actually requires PIO.
>
> On tile, it would be just as broken as other drivers that directly use
> inb/outb to do the port access, but on platforms that actually have
> PIO but are not able to map them (like arm/mach-rpc), you would now
> enable people to write silently broken drivers.
>
> However, assuming they check the return value of the *iomap, the
> drivers could still easily detect this case at runtime. Also, at
> least on tile, you already provide a pci_iomap function, which
> has the exact same problem as the devres functions.
>
> One area where your patch might actually break existing code is
> architectures that allow the devres functions to be built *and*
> have PCI support but don't provide a pci_iomap function, because
> they do not support mapping the PIO range into addresses. I'm
> not sure if that's actually still the case anywhere after Michael's
> recent patches, but it used to be the case on RPC and probably others.

Sounds sufficiently subtle that I'll plan to drop the patch unless someone
else (maybe Al Viro, now cc'ed) wants to weigh in.  For tile this just
means we'll continue not to set NO_IOPORT, at the cost of a few hundred
bytes of dead code; no big deal.

The specific issue I saw was calls to pcim_iomap_{regions,table} from
drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the
sata_sil24 in one of our platforms (TILEmpower).

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com