On Mon, Dec 9, 2013 at 12:33 PM, Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote: > On Thu, Dec 05, 2013 at 05:19:55PM -0700, Bjorn Helgaas wrote: >> pci_setup_bridge_io() accessed PCI_IO_BASE and PCI_IO_LIMIT using dword >> (32-bit) reads and writes, which also access the Secondary Status register. >> Since the Secondary Status register is in the upper 16 bits of the dword, >> and we preserved those upper 16 bits, this had the effect of clearing any >> of the write-1-to-clear bits that happened to be set in the Secondary >> Status register. > > This is a good catch! > >> - pci_write_config_dword(bridge, PCI_IO_BASE, l); >> + pci_write_config_word(bridge, PCI_IO_BASE, l); > > But this is a problem :( > > tegra and mvebu at least do not have HW to do non-32 bit writes, so > their implementation of pci_write_config_word does the RMW internally > and will still have this same bug. > > I think you have to keep the 32 bit write here, but zero the > write-one-to-clear bits :( Is there actually some spec requirement about the access sizes that must be supported by the hardware? If there is something, I'll gladly keep the 32-bit access, but if it's only a tegra/mvebu-specific restriction, then I think it needs to be handled in the PCI config accessors for that hardware. This spec statement (PCI (not PCIe) r3.0, sec 3.2.2.3.4) makes it sound like tegra/mvebu is non-conforming and should deal with this specially: "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." I think the tegra/mvebu accessors should be able to use the address and access size to figure out what we're trying to do. If we're doing a WORD write to PCI_IO_BASE, it can turn that into: 32-bit read, clear the upper 16 bits (secondary status), insert the PCI_IO_BASE part, 32-bit write. If we're doing a 16-bit write to PCI_SEC_STATUS, it can turn that into: 32-bit read, insert upper 16 bits (secondary status), leave lower 16 bits alone, 32-bit write. 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
