| Message ID | 20161031213902.6340.96123.stgit@bhelgaas-glaptop.roam.corp.google.com |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Oct 31, 2016 at 04:39:02PM -0500, Bjorn Helgaas wrote: > Hardware that supports only 32-bit config writes is not spec-compliant. > For example, if software performs a 16-bit write, we must do a 32-bit read, > merge in the 16 bits we intend to write, followed by a 32-bit write. If > the 16 bits we *don't* intend to write happen to have any RW1C (write-one- > to-clear) bits set, we just inadvertently cleared something we shouldn't > have. > > Add a rate-limited warning when we do sub-32 bit config writes. Remove > similar probe-time warnings from some of the affected host bridge drivers. Yay, I've been on about this for ages... Enthusiastically-Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
On 2016/11/1 5:39, Bjorn Helgaas wrote: > Hardware that supports only 32-bit config writes is not spec-compliant. > For example, if software performs a 16-bit write, we must do a 32-bit read, > merge in the 16 bits we intend to write, followed by a 32-bit write. If > the 16 bits we *don't* intend to write happen to have any RW1C (write-one- > to-clear) bits set, we just inadvertently cleared something we shouldn't > have. > > Add a rate-limited warning when we do sub-32 bit config writes. Remove > similar probe-time warnings from some of the affected host bridge drivers. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/access.c | 17 +++++++++++++++-- > drivers/pci/host/pcie-hisi.c | 2 -- > drivers/pci/host/pcie-rockchip.c | 3 --- > 3 files changed, 15 insertions(+), 7 deletions(-) > For pcie-rockchip, Acked-by: Shawn Lin <shawn.lin@rock-chips.com> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > index d11cdbb..e0cd124 100644 > --- a/drivers/pci/access.c > +++ b/drivers/pci/access.c > @@ -142,10 +142,23 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn, > if (size == 4) { > writel(val, addr); > return PCIBIOS_SUCCESSFUL; > - } else { > - mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); > } > > + /* > + * N.B. In general, hardware that supports only 32-bit writes on > + * PCI is not spec-compliant. For example, software may perform a > + * 16-bit write. If the hardware only supports 32-bit accesses, we > + * must do a 32-bit read, merge in the 16 bits we intend to write, > + * followed by a 32-bit write. If the 16 bits we *don't* intend to > + * write happen to have any RW1C (write-one-to-clear) bits set, we > + * just inadvertently cleared something we shouldn't have. > + */ > + > + dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n", > + size, pci_domain_nr(bus), bus->number, > + PCI_SLOT(devfn), PCI_FUNC(devfn), where); > + > + mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); > tmp = readl(addr) & mask; > tmp |= val << ((where & 0x3) * 8); > writel(tmp, addr); > diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c > index 56154c2..5b5901d 100644 > --- a/drivers/pci/host/pcie-hisi.c > +++ b/drivers/pci/host/pcie-hisi.c > @@ -194,8 +194,6 @@ static int hisi_pcie_probe(struct platform_device *pdev) > if (ret) > return ret; > > - dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); > - > return 0; > } > > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > index e0b22da..6419d8c 100644 > --- a/drivers/pci/host/pcie-rockchip.c > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -1187,9 +1187,6 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > pcie_bus_configure_settings(child); > > pci_bus_add_devices(bus); > - > - dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); > - > return err; > > err_vpcie: > > > >
On Mon, Oct 31, 2016 at 04:39:02PM -0500, Bjorn Helgaas wrote: > Hardware that supports only 32-bit config writes is not spec-compliant. > For example, if software performs a 16-bit write, we must do a 32-bit read, > merge in the 16 bits we intend to write, followed by a 32-bit write. If > the 16 bits we *don't* intend to write happen to have any RW1C (write-one- > to-clear) bits set, we just inadvertently cleared something we shouldn't > have. > > Add a rate-limited warning when we do sub-32 bit config writes. Remove > similar probe-time warnings from some of the affected host bridge drivers. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/access.c | 17 +++++++++++++++-- > drivers/pci/host/pcie-hisi.c | 2 -- > drivers/pci/host/pcie-rockchip.c | 3 --- > 3 files changed, 15 insertions(+), 7 deletions(-) Yeah, this is a good idea: Acked-by: Thierry Reding <treding@nvidia.com> ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
On Mon, Oct 31, 2016 at 04:39:02PM -0500, Bjorn Helgaas wrote: > Hardware that supports only 32-bit config writes is not spec-compliant. > For example, if software performs a 16-bit write, we must do a 32-bit read, > merge in the 16 bits we intend to write, followed by a 32-bit write. If > the 16 bits we *don't* intend to write happen to have any RW1C (write-one- > to-clear) bits set, we just inadvertently cleared something we shouldn't > have. > > Add a rate-limited warning when we do sub-32 bit config writes. Remove > similar probe-time warnings from some of the affected host bridge drivers. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Applied with acks from Russell, Shawn, and Thierry to pci/enumeration for v4.10. > --- > drivers/pci/access.c | 17 +++++++++++++++-- > drivers/pci/host/pcie-hisi.c | 2 -- > drivers/pci/host/pcie-rockchip.c | 3 --- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > index d11cdbb..e0cd124 100644 > --- a/drivers/pci/access.c > +++ b/drivers/pci/access.c > @@ -142,10 +142,23 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn, > if (size == 4) { > writel(val, addr); > return PCIBIOS_SUCCESSFUL; > - } else { > - mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); > } > > + /* > + * N.B. In general, hardware that supports only 32-bit writes on > + * PCI is not spec-compliant. For example, software may perform a > + * 16-bit write. If the hardware only supports 32-bit accesses, we > + * must do a 32-bit read, merge in the 16 bits we intend to write, > + * followed by a 32-bit write. If the 16 bits we *don't* intend to > + * write happen to have any RW1C (write-one-to-clear) bits set, we > + * just inadvertently cleared something we shouldn't have. > + */ > + > + dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n", > + size, pci_domain_nr(bus), bus->number, > + PCI_SLOT(devfn), PCI_FUNC(devfn), where); > + > + mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); > tmp = readl(addr) & mask; > tmp |= val << ((where & 0x3) * 8); > writel(tmp, addr); > diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c > index 56154c2..5b5901d 100644 > --- a/drivers/pci/host/pcie-hisi.c > +++ b/drivers/pci/host/pcie-hisi.c > @@ -194,8 +194,6 @@ static int hisi_pcie_probe(struct platform_device *pdev) > if (ret) > return ret; > > - dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); > - > return 0; > } > > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > index e0b22da..6419d8c 100644 > --- a/drivers/pci/host/pcie-rockchip.c > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -1187,9 +1187,6 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > pcie_bus_configure_settings(child); > > pci_bus_add_devices(bus); > - > - dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); > - > return err; > > err_vpcie: > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/access.c b/drivers/pci/access.c index d11cdbb..e0cd124 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -142,10 +142,23 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn, if (size == 4) { writel(val, addr); return PCIBIOS_SUCCESSFUL; - } else { - mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); } + /* + * N.B. In general, hardware that supports only 32-bit writes on + * PCI is not spec-compliant. For example, software may perform a + * 16-bit write. If the hardware only supports 32-bit accesses, we + * must do a 32-bit read, merge in the 16 bits we intend to write, + * followed by a 32-bit write. If the 16 bits we *don't* intend to + * write happen to have any RW1C (write-one-to-clear) bits set, we + * just inadvertently cleared something we shouldn't have. + */ + + dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n", + size, pci_domain_nr(bus), bus->number, + PCI_SLOT(devfn), PCI_FUNC(devfn), where); + + mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8)); tmp = readl(addr) & mask; tmp |= val << ((where & 0x3) * 8); writel(tmp, addr); diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c index 56154c2..5b5901d 100644 --- a/drivers/pci/host/pcie-hisi.c +++ b/drivers/pci/host/pcie-hisi.c @@ -194,8 +194,6 @@ static int hisi_pcie_probe(struct platform_device *pdev) if (ret) return ret; - dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); - return 0; } diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c index e0b22da..6419d8c 100644 --- a/drivers/pci/host/pcie-rockchip.c +++ b/drivers/pci/host/pcie-rockchip.c @@ -1187,9 +1187,6 @@ static int rockchip_pcie_probe(struct platform_device *pdev) pcie_bus_configure_settings(child); pci_bus_add_devices(bus); - - dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n"); - return err; err_vpcie:
Hardware that supports only 32-bit config writes is not spec-compliant. For example, if software performs a 16-bit write, we must do a 32-bit read, merge in the 16 bits we intend to write, followed by a 32-bit write. If the 16 bits we *don't* intend to write happen to have any RW1C (write-one- to-clear) bits set, we just inadvertently cleared something we shouldn't have. Add a rate-limited warning when we do sub-32 bit config writes. Remove similar probe-time warnings from some of the affected host bridge drivers. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/access.c | 17 +++++++++++++++-- drivers/pci/host/pcie-hisi.c | 2 -- drivers/pci/host/pcie-rockchip.c | 3 --- 3 files changed, 15 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html