[PATCH] arm: a37xx: pci: Fix handling PIO config error responses

[PATCH] arm: a37xx: pci: Fix handling PIO config error responses

[Pali Rohár]

Returning fabricated CRS value (0xFFFF0001) by PCIe Root Complex to OS is
allowed only for 4-byte PCI_VENDOR_ID config read request and only when
CRSSVE bit in Root Port PCIe device is enabled. In all other error PCIe
Root Complex must return all-ones.

So implement this logic in pci-aardvark.c driver properly.

aardvark HW does not have Root Port PCIe device and U-Boot does not
implement emulation of this device. So expect that CRSSVE bit is set as
U-Boot can already handle CRS value for PCI_VENDOR_ID config read request.

More callers of pci_bus_read_config() function in U-Boot do not check for
return value, but check readback value. Therefore always fill readback
value in pcie_advk_read_config() function. On error fill all-ones of
correct size as it is required for PCIe Root Complex.

And also correctly propagates error from failed config write request to
return value of pcie_advk_write_config() function. Most U-Boot callers
ignores this return value, but it is a good idea to return correct value
from function.

These issues about return value of failed config read requests, including
special handling of CRS were reported by Lorenzo and Bjorn for Linux kernel
driver pci-aardvark together with quotes from PCIe r4.0 spec, see details:
https://lore.kernel.org/linux-pci/20210624213345.3617-1-pali@kernel.org/t/#u

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/pci-aardvark.c | 52 +++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
index 1b9bae7cca72..815b26162f15 100644
--- a/drivers/pci/pci-aardvark.c
+++ b/drivers/pci/pci-aardvark.c
@@ -177,7 +177,6 @@
 #define LINK_MAX_RETRIES			10
 #define LINK_WAIT_TIMEOUT			100000
 
-#define CFG_RD_UR_VAL			0xFFFFFFFF
 #define CFG_RD_CRS_VAL			0xFFFF0001
 
 /**
@@ -263,12 +262,12 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
  * pcie_advk_check_pio_status() - Validate PIO status and get the read result
  *
  * @pcie: Pointer to the PCI bus
- * @read: Read from or write to configuration space - true(read) false(write)
- * @read_val: Pointer to the read result, only valid when read is true
+ * @allow_crs: Only for read requests, if CRS response is allowed
+ * @read_val: Pointer to the read result
  *
  */
 static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
-				      bool read,
+				      bool allow_crs,
 				      uint *read_val)
 {
 	uint reg;
@@ -286,22 +285,16 @@ static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
 			break;
 		}
 		/* Get the read result */
-		if (read)
+		if (read_val)
 			*read_val = advk_readl(pcie, PIO_RD_DATA);
 		/* No error */
 		strcomp_status = NULL;
 		break;
 	case PIO_COMPLETION_STATUS_UR:
-		if (read) {
-			/* For reading, UR is not an error status. */
-			*read_val = CFG_RD_UR_VAL;
-			strcomp_status = NULL;
-		} else {
-			strcomp_status = "UR";
-		}
+		strcomp_status = "UR";
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		if (read) {
+		if (allow_crs && read_val) {
 			/* For reading, CRS is not an error status. */
 			*read_val = CFG_RD_CRS_VAL;
 			strcomp_status = NULL;
@@ -352,6 +345,7 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
 				 enum pci_size_t size)
 {
 	struct pcie_advk *pcie = dev_get_priv(bus);
+	bool allow_crs;
 	uint reg;
 	int ret;
 
@@ -364,13 +358,17 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
 		return 0;
 	}
 
+	allow_crs = (offset == PCI_VENDOR_ID) && (size == 4);
+
 	if (advk_readl(pcie, PIO_START)) {
 		dev_err(pcie->dev,
 			"Previous PIO read/write transfer is still running\n");
-		if (offset != PCI_VENDOR_ID)
-			return -EINVAL;
-		*valuep = CFG_RD_CRS_VAL;
-		return 0;
+		if (allow_crs) {
+			*valuep = CFG_RD_CRS_VAL;
+			return 0;
+		}
+		*valuep = pci_get_ff(size);
+		return -EINVAL;
 	}
 
 	/* Program the control register */
@@ -392,16 +390,20 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
 	advk_writel(pcie, 1, PIO_START);
 
 	if (!pcie_advk_wait_pio(pcie)) {
-		if (offset != PCI_VENDOR_ID)
-			return -EINVAL;
-		*valuep = CFG_RD_CRS_VAL;
-		return 0;
+		if (allow_crs) {
+			*valuep = CFG_RD_CRS_VAL;
+			return 0;
+		}
+		*valuep = pci_get_ff(size);
+		return -EINVAL;
 	}
 
 	/* Check PIO status and get the read result */
-	ret = pcie_advk_check_pio_status(pcie, true, &reg);
-	if (ret)
+	ret = pcie_advk_check_pio_status(pcie, allow_crs, &reg);
+	if (ret) {
+		*valuep = pci_get_ff(size);
 		return ret;
+	}
 
 	dev_dbg(pcie->dev, "(addr,size,val)=(0x%04x, %d, 0x%08x)\n",
 		offset, size, reg);
@@ -511,9 +513,7 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
 	}
 
 	/* Check PIO status */
-	pcie_advk_check_pio_status(pcie, false, &reg);
-
-	return 0;
+	return pcie_advk_check_pio_status(pcie, false, NULL);
 }
 
 /**
-- 
2.20.1

Re: [PATCH] arm: a37xx: pci: Fix handling PIO config error responses

[Stefan Roese]

On 09.08.21 09:53, Pali Rohár wrote:
> Returning fabricated CRS value (0xFFFF0001) by PCIe Root Complex to OS is
> allowed only for 4-byte PCI_VENDOR_ID config read request and only when
> CRSSVE bit in Root Port PCIe device is enabled. In all other error PCIe
> Root Complex must return all-ones.
> 
> So implement this logic in pci-aardvark.c driver properly.
> 
> aardvark HW does not have Root Port PCIe device and U-Boot does not
> implement emulation of this device. So expect that CRSSVE bit is set as
> U-Boot can already handle CRS value for PCI_VENDOR_ID config read request.
> 
> More callers of pci_bus_read_config() function in U-Boot do not check for
> return value, but check readback value. Therefore always fill readback
> value in pcie_advk_read_config() function. On error fill all-ones of
> correct size as it is required for PCIe Root Complex.
> 
> And also correctly propagates error from failed config write request to
> return value of pcie_advk_write_config() function. Most U-Boot callers
> ignores this return value, but it is a good idea to return correct value
> from function.
> 
> These issues about return value of failed config read requests, including
> special handling of CRS were reported by Lorenzo and Bjorn for Linux kernel
> driver pci-aardvark together with quotes from PCIe r4.0 spec, see details:
> https://lore.kernel.org/linux-pci/20210624213345.3617-1-pali@kernel.org/t/#u
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 52 +++++++++++++++++++-------------------
>   1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 1b9bae7cca72..815b26162f15 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -177,7 +177,6 @@
>   #define LINK_MAX_RETRIES			10
>   #define LINK_WAIT_TIMEOUT			100000
>   
> -#define CFG_RD_UR_VAL			0xFFFFFFFF
>   #define CFG_RD_CRS_VAL			0xFFFF0001
>   
>   /**
> @@ -263,12 +262,12 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>    * pcie_advk_check_pio_status() - Validate PIO status and get the read result
>    *
>    * @pcie: Pointer to the PCI bus
> - * @read: Read from or write to configuration space - true(read) false(write)
> - * @read_val: Pointer to the read result, only valid when read is true
> + * @allow_crs: Only for read requests, if CRS response is allowed
> + * @read_val: Pointer to the read result
>    *
>    */
>   static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
> -				      bool read,
> +				      bool allow_crs,
>   				      uint *read_val)
>   {
>   	uint reg;
> @@ -286,22 +285,16 @@ static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
>   			break;
>   		}
>   		/* Get the read result */
> -		if (read)
> +		if (read_val)
>   			*read_val = advk_readl(pcie, PIO_RD_DATA);
>   		/* No error */
>   		strcomp_status = NULL;
>   		break;
>   	case PIO_COMPLETION_STATUS_UR:
> -		if (read) {
> -			/* For reading, UR is not an error status. */
> -			*read_val = CFG_RD_UR_VAL;
> -			strcomp_status = NULL;
> -		} else {
> -			strcomp_status = "UR";
> -		}
> +		strcomp_status = "UR";
>   		break;
>   	case PIO_COMPLETION_STATUS_CRS:
> -		if (read) {
> +		if (allow_crs && read_val) {
>   			/* For reading, CRS is not an error status. */
>   			*read_val = CFG_RD_CRS_VAL;
>   			strcomp_status = NULL;
> @@ -352,6 +345,7 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   				 enum pci_size_t size)
>   {
>   	struct pcie_advk *pcie = dev_get_priv(bus);
> +	bool allow_crs;
>   	uint reg;
>   	int ret;
>   
> @@ -364,13 +358,17 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> +	allow_crs = (offset == PCI_VENDOR_ID) && (size == 4);
> +
>   	if (advk_readl(pcie, PIO_START)) {
>   		dev_err(pcie->dev,
>   			"Previous PIO read/write transfer is still running\n");
> -		if (offset != PCI_VENDOR_ID)
> -			return -EINVAL;
> -		*valuep = CFG_RD_CRS_VAL;
> -		return 0;
> +		if (allow_crs) {
> +			*valuep = CFG_RD_CRS_VAL;
> +			return 0;
> +		}
> +		*valuep = pci_get_ff(size);
> +		return -EINVAL;
>   	}
>   
>   	/* Program the control register */
> @@ -392,16 +390,20 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	advk_writel(pcie, 1, PIO_START);
>   
>   	if (!pcie_advk_wait_pio(pcie)) {
> -		if (offset != PCI_VENDOR_ID)
> -			return -EINVAL;
> -		*valuep = CFG_RD_CRS_VAL;
> -		return 0;
> +		if (allow_crs) {
> +			*valuep = CFG_RD_CRS_VAL;
> +			return 0;
> +		}
> +		*valuep = pci_get_ff(size);
> +		return -EINVAL;
>   	}
>   
>   	/* Check PIO status and get the read result */
> -	ret = pcie_advk_check_pio_status(pcie, true, &reg);
> -	if (ret)
> +	ret = pcie_advk_check_pio_status(pcie, allow_crs, &reg);
> +	if (ret) {
> +		*valuep = pci_get_ff(size);
>   		return ret;
> +	}
>   
>   	dev_dbg(pcie->dev, "(addr,size,val)=(0x%04x, %d, 0x%08x)\n",
>   		offset, size, reg);
> @@ -511,9 +513,7 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/* Check PIO status */
> -	pcie_advk_check_pio_status(pcie, false, &reg);
> -
> -	return 0;
> +	return pcie_advk_check_pio_status(pcie, false, NULL);
>   }
>   
>   /**
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] arm: a37xx: pci: Fix handling PIO config error responses

[Stefan Roese]

On 09.08.21 09:53, Pali Rohár wrote:
> Returning fabricated CRS value (0xFFFF0001) by PCIe Root Complex to OS is
> allowed only for 4-byte PCI_VENDOR_ID config read request and only when
> CRSSVE bit in Root Port PCIe device is enabled. In all other error PCIe
> Root Complex must return all-ones.
> 
> So implement this logic in pci-aardvark.c driver properly.
> 
> aardvark HW does not have Root Port PCIe device and U-Boot does not
> implement emulation of this device. So expect that CRSSVE bit is set as
> U-Boot can already handle CRS value for PCI_VENDOR_ID config read request.
> 
> More callers of pci_bus_read_config() function in U-Boot do not check for
> return value, but check readback value. Therefore always fill readback
> value in pcie_advk_read_config() function. On error fill all-ones of
> correct size as it is required for PCIe Root Complex.
> 
> And also correctly propagates error from failed config write request to
> return value of pcie_advk_write_config() function. Most U-Boot callers
> ignores this return value, but it is a good idea to return correct value
> from function.
> 
> These issues about return value of failed config read requests, including
> special handling of CRS were reported by Lorenzo and Bjorn for Linux kernel
> driver pci-aardvark together with quotes from PCIe r4.0 spec, see details:
> https://lore.kernel.org/linux-pci/20210624213345.3617-1-pali@kernel.org/t/#u
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 52 +++++++++++++++++++-------------------
>   1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 1b9bae7cca72..815b26162f15 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -177,7 +177,6 @@
>   #define LINK_MAX_RETRIES			10
>   #define LINK_WAIT_TIMEOUT			100000
>   
> -#define CFG_RD_UR_VAL			0xFFFFFFFF
>   #define CFG_RD_CRS_VAL			0xFFFF0001
>   
>   /**
> @@ -263,12 +262,12 @@ static int pcie_advk_wait_pio(struct pcie_advk *pcie)
>    * pcie_advk_check_pio_status() - Validate PIO status and get the read result
>    *
>    * @pcie: Pointer to the PCI bus
> - * @read: Read from or write to configuration space - true(read) false(write)
> - * @read_val: Pointer to the read result, only valid when read is true
> + * @allow_crs: Only for read requests, if CRS response is allowed
> + * @read_val: Pointer to the read result
>    *
>    */
>   static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
> -				      bool read,
> +				      bool allow_crs,
>   				      uint *read_val)
>   {
>   	uint reg;
> @@ -286,22 +285,16 @@ static int pcie_advk_check_pio_status(struct pcie_advk *pcie,
>   			break;
>   		}
>   		/* Get the read result */
> -		if (read)
> +		if (read_val)
>   			*read_val = advk_readl(pcie, PIO_RD_DATA);
>   		/* No error */
>   		strcomp_status = NULL;
>   		break;
>   	case PIO_COMPLETION_STATUS_UR:
> -		if (read) {
> -			/* For reading, UR is not an error status. */
> -			*read_val = CFG_RD_UR_VAL;
> -			strcomp_status = NULL;
> -		} else {
> -			strcomp_status = "UR";
> -		}
> +		strcomp_status = "UR";
>   		break;
>   	case PIO_COMPLETION_STATUS_CRS:
> -		if (read) {
> +		if (allow_crs && read_val) {
>   			/* For reading, CRS is not an error status. */
>   			*read_val = CFG_RD_CRS_VAL;
>   			strcomp_status = NULL;
> @@ -352,6 +345,7 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   				 enum pci_size_t size)
>   {
>   	struct pcie_advk *pcie = dev_get_priv(bus);
> +	bool allow_crs;
>   	uint reg;
>   	int ret;
>   
> @@ -364,13 +358,17 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   		return 0;
>   	}
>   
> +	allow_crs = (offset == PCI_VENDOR_ID) && (size == 4);
> +
>   	if (advk_readl(pcie, PIO_START)) {
>   		dev_err(pcie->dev,
>   			"Previous PIO read/write transfer is still running\n");
> -		if (offset != PCI_VENDOR_ID)
> -			return -EINVAL;
> -		*valuep = CFG_RD_CRS_VAL;
> -		return 0;
> +		if (allow_crs) {
> +			*valuep = CFG_RD_CRS_VAL;
> +			return 0;
> +		}
> +		*valuep = pci_get_ff(size);
> +		return -EINVAL;
>   	}
>   
>   	/* Program the control register */
> @@ -392,16 +390,20 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	advk_writel(pcie, 1, PIO_START);
>   
>   	if (!pcie_advk_wait_pio(pcie)) {
> -		if (offset != PCI_VENDOR_ID)
> -			return -EINVAL;
> -		*valuep = CFG_RD_CRS_VAL;
> -		return 0;
> +		if (allow_crs) {
> +			*valuep = CFG_RD_CRS_VAL;
> +			return 0;
> +		}
> +		*valuep = pci_get_ff(size);
> +		return -EINVAL;
>   	}
>   
>   	/* Check PIO status and get the read result */
> -	ret = pcie_advk_check_pio_status(pcie, true, &reg);
> -	if (ret)
> +	ret = pcie_advk_check_pio_status(pcie, allow_crs, &reg);
> +	if (ret) {
> +		*valuep = pci_get_ff(size);
>   		return ret;
> +	}
>   
>   	dev_dbg(pcie->dev, "(addr,size,val)=(0x%04x, %d, 0x%08x)\n",
>   		offset, size, reg);
> @@ -511,9 +513,7 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/* Check PIO status */
> -	pcie_advk_check_pio_status(pcie, false, &reg);
> -
> -	return 0;
> +	return pcie_advk_check_pio_status(pcie, false, NULL);
>   }
>   
>   /**
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de