On Mon, Dec 9, 2013 at 5:10 PM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Dec 09, 2013 at 02:58:06PM -0700, Bjorn Helgaas wrote:
>
>> >> "A function must not restrict the size of the access it supports in
>> >> Configuration Space. The configuration commands, like other commands,
>> >> allow data to be accessed using any combination of bytes (including a
>> >> byte, word, DWORD, or non-contiguous bytes) and multiple data phases
>> >> in a burst. The target is required to handle any combination of byte
>> >> enables."
>> >
>> > This verbage is talking about the target/responder, not the host. Any
>> > target must respond to all possible sizes for all possible config
>> > registers, including combinations that even x86 can't issue (like a BE
>> > pattern of b0110)
>>
>> Now I'm confused. From the PCI core's point of view,
>> pci_setup_bridge_io() is talking to a target, namely a PCI-to-PCI
>> bridge. PCI config space accessors are also for talking to targets.
>
> The fundamental issue is that the PCI host drivers I pointed out have
> hardware that cannot construct a PCI-E ConfigWr TLP (or PCI/PCI-X
> eqiuv) with any content other than a 32 bit DWORD write.
Yep.
> So even though pci_setup_bridge_io is talking to a target, and the
> target is required to handle all possible ConfigWr TLPs, the host
> driver (struct pci_ops) can only generate one possibility.
Yep.
> The problematic host drivers are all emulating non-32 bit ConfigWr
> support by doing RMW, and you have observed that RMW is not 100%
> correct when dealing with write 1 to clear bits.
Yep.
> Basically, your fix to pci_setup_bridge_io is fine - but your
> observation led me to realize that the HW drivers implementing RMW for
> 8 and 16 bit ops under their struct pci_ops.read function have exactly
> the same flaw you are fixing here - they will silently wipe out write
> 1 to clear bits.
Ah, OK. I thought you were saying that we couldn't change
pci_setup_bridge_io() to use 16-bit accesses because of this problem.
>> Some arches set up their host bridges so they appear in PCI config
>> space and have register layouts that look like PCI-to-PCI bridges. In
>> my opinion, those arches then have the responsibility of following all
>> the PCI-to-PCI bridge rules, including access size restrictions,
>> either directly in hardware or in their config accessors.
>
> Sure, but this isn't the problem :)
>
> IMHO, the pci core needs to help host drivers implement a correct
> pci_ops.write function for the case where the host hardware can only
> produce 32 bit ConfigWR TLPs. That seems to be common enough and
> subtle enough it shouldn't be inlined into every driver.
Sure, if somebody can come up with a reasonable way to share this sort
of code, that sounds like a good thing. Maybe extending struct
pci_ops is the way to do this, but I hope not, because it seems like
it will require quite a bit of specific knowledge about which
registers have RW1C bits. They're not all at constant offsets because
they can be in capabilities, too.
> Just to be clear, I am talking about implementations like this:
>
> static int cns3xxx_pci_write_config(struct pci_bus *bus, unsigned int devfn,
> int where, int size, u32 val)
> {
> u32 v;
> void __iomem *base;
> u32 mask = (0x1ull << (size * 8)) - 1;
> int shift = (where % 4) * 8;
>
> base = cns3xxx_pci_cfg_base(bus, devfn, where);
> if (!base)
> return PCIBIOS_SUCCESSFUL;
>
> v = __raw_readl(base);
>
> v &= ~(mask << shift);
> v |= (val & mask) << shift;
>
> __raw_writel(v, base);
>
> return PCIBIOS_SUCCESSFUL;
> }
>
> static struct pci_ops cns3xxx_pcie_ops = {
> .read = cns3xxx_pci_read_config,
> .write = cns3xxx_pci_write_config,
> };
>
> Which we now know is subtly broken as it will clear write 1 to clear
> bits when doing sub dword operations.
>
> I found 6 copies of this pattern in 5 mins of searching :(
Yes, I bet there are a bunch of them.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
