* Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes
2016-10-31 21:39 [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes Bjorn Helgaas
@ 2016-11-01 9:34 ` Russell King - ARM Linux
2016-11-01 9:59 ` Shawn Lin
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Russell King - ARM Linux @ 2016-11-01 9:34 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Rob Herring, Heiko Stuebner, Gabriele Paoloni,
Jon Mason, Ray Jui, Shawn Lin, Wenrui Li, Scott Branden,
Zhou Wang, Tanmay Inamdar, Greg Ungerer, Thierry Reding
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>
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes
2016-10-31 21:39 [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes Bjorn Helgaas
2016-11-01 9:34 ` Russell King - ARM Linux
@ 2016-11-01 9:59 ` Shawn Lin
2016-11-02 17:45 ` Thierry Reding
2016-11-10 17:27 ` Bjorn Helgaas
3 siblings, 0 replies; 5+ messages in thread
From: Shawn Lin @ 2016-11-01 9:59 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci
Cc: shawn.lin, Rob Herring, Heiko Stuebner, Gabriele Paoloni,
Jon Mason, Ray Jui, Wenrui Li, Scott Branden, Russell King,
Zhou Wang, Tanmay Inamdar, Greg Ungerer, Thierry Reding
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:
>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes
2016-10-31 21:39 [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes Bjorn Helgaas
2016-11-01 9:34 ` Russell King - ARM Linux
2016-11-01 9:59 ` Shawn Lin
@ 2016-11-02 17:45 ` Thierry Reding
2016-11-10 17:27 ` Bjorn Helgaas
3 siblings, 0 replies; 5+ messages in thread
From: Thierry Reding @ 2016-11-02 17:45 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Rob Herring, Heiko Stuebner, Gabriele Paoloni,
Jon Mason, Ray Jui, Shawn Lin, Wenrui Li, Scott Branden,
Russell King, Zhou Wang, Tanmay Inamdar, Greg Ungerer
[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]
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.
-----------------------------------------------------------------------------------
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes
2016-10-31 21:39 [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes Bjorn Helgaas
` (2 preceding siblings ...)
2016-11-02 17:45 ` Thierry Reding
@ 2016-11-10 17:27 ` Bjorn Helgaas
3 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2016-11-10 17:27 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Rob Herring, Heiko Stuebner, Gabriele Paoloni,
Jon Mason, Ray Jui, Shawn Lin, Wenrui Li, Scott Branden,
Russell King, Zhou Wang, Tanmay Inamdar, Greg Ungerer,
Thierry Reding
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
^ permalink raw reply [flat|nested] 5+ messages in thread