Re: pci_generic_config_write32() access size problem

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Thu, Oct 01, 2015 at 09:46:59AM +0200, Thierry Reding wrote:
> So it seems like it's failing to properly read the header type. That's
> retrieved from byte 0xe as an 8 bit access. That said, even with 32-bit
> accesses I haven't seen any problems, but that might just be because
> none of the devices I've tested with have R1WC bits in problematic
> places.

As I've explained, the registers which are most likely to be affected
in PCIe are:

- PCI status register
- PCIe device status register
- PCIe error status(es)

and possibly others.  Some of these registers are accessed by the PCIe
AER driver.

Here's the problem: how do you know whether the PCIe on your board is
operating correctly if your PCI host driver goes around writing '1's
to the RW1C bits in these status registers, thereby clearing the error
status(es)?  It may appear to be working correctly, but generating a
number of correctable errors.

Also, as I've pointed out, how do you know whether 8-bit or 16-bit
registers neighbouring the register you're accessing are safe to be
written, especially those in the space above 0x40, where vendors are
allowed to place their own vendor specific registers - this is the
bigger problem.

Yes, you can get away with saying "the cards I have in front of me
work today" but as long as the bus doesn't support 8-bit or 16-bit
accesses, you're at the mercy of these issues, because what you have
is an implementation which doesn't conform to the PCI specifications.

The point that I initially raised was that (IMHO) generic PCI code
should not be making it easy to make "mistakes" like always accessing
config space via 32-bit accesses, at least not without taking some
counter-measures against it - even at a basic level of detecting a
write to the PCI command register and zeroing the upper 16-bits of
the 32-bit quantity it's about to write back.  Maybe also looking
up the PCI device, and then its capabilities, and doing a similar
thing for the PCIe status registers containing RW1C bits.

Right now, it's all too easy to just plug the 32-bit accessors in
without thinking, and without even checking or testing whether the
PCI conformant accessors should be used.

It seems that the Marvell platforms have suffered from something like
this: the later hardware definitely can (it's specified to) but it's
not mentioned whether or not they're supported in older hardware - but
practical tests have shown that the 8-bit and 16-bit accesses are
supported there too.  However, the driver opted to just use the 32-bit
accessors anyway.

I'd hope that having the code more complex behind the 32-bit accessors
to at least try a little harder to avoid clearing these RW1C bits will
be enough to make people think twice about using them - especially if
they have to walk the list of PCI devices on the bus to try and find
the appropriate PCI device struct corresponding to the requested devfn.

Those who are dealing with non-conformant hardware then get a slight
performance impact when accessing PCI configuration space, which I think
is entirely justified given that it _is_ non-conformant hardware.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.