[PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes

[PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes

[Bjorn Helgaas]

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(-)

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:

Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes

[Russell King - ARM Linux]

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.

Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes

[Shawn Lin]

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

Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes

[Thierry Reding]

[-- 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 --]

Re: [PATCH] PCI: Warn on possible RW1C corruption for sub-32 bit config writes

[Bjorn Helgaas]

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