[PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Pali Rohár]

Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
handling of CRS response and when CRSSVE flag was not enabled it marked CRS
response as failed transaction (due to simplicity).

But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
for PIO config response and implementation of re-issuing config requests
according to PCIe base specification is therefore simple.

This change implements re-issuing of config requests when response is CRS.
And to prevent infinite loop set upper bound to around PIO_RETRY_CNT value,
after which is transaction marked as failed and 0xFFFFFFFF is returned like
before.

Implementation is done by returning appropriate error codes from function
advk_pcie_check_pio_status(). On CRS is returned -EAGAIN and caller then
reissue transaction up to the PIO_RETRY_CNT count. As advk_pcie_wait_pio()
function waits some cycles, return number of these cycles and add them to
the retry count. So the total time for config request would be only linear
O(PIO_RETRY_CNT) and not quadratic O(PIO_RETRY_CNT^2) in the worst case.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
---
 drivers/pci/controller/pci-aardvark.c | 36 ++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index abc93225ba20..99f244190eae 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -470,6 +470,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
 	u32 reg;
 	unsigned int status;
 	char *strcomp_status, *str_posted;
+	int ret;
 
 	reg = advk_readl(pcie, PIO_STAT);
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
@@ -494,6 +495,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
 	case PIO_COMPLETION_STATUS_OK:
 		if (reg & PIO_ERR_STATUS) {
 			strcomp_status = "COMP_ERR";
+			ret = -EFAULT;
 			break;
 		}
 		/* Get the read result */
@@ -501,9 +503,11 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
 			*val = advk_readl(pcie, PIO_RD_DATA);
 		/* No error */
 		strcomp_status = NULL;
+		ret = 0;
 		break;
 	case PIO_COMPLETION_STATUS_UR:
 		strcomp_status = "UR";
+		ret = -EOPNOTSUPP;
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
 		if (allow_crs && val) {
@@ -521,6 +525,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
 			 */
 			*val = CFG_RD_CRS_VAL;
 			strcomp_status = NULL;
+			ret = 0;
 			break;
 		}
 		/* PCIe r4.0, sec 2.3.2, says:
@@ -536,21 +541,24 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
 		 * Request and taking appropriate action, e.g., complete the
 		 * Request to the host as a failed transaction.
 		 *
-		 * To simplify implementation do not re-issue the Configuration
-		 * Request and complete the Request as a failed transaction.
+		 * So return -EAGAIN and caller (pci-aardvark.c driver) will
+		 * re-issue request again up to the PIO_RETRY_CNT retries.
 		 */
 		strcomp_status = "CRS";
+		ret = -EAGAIN;
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";
+		ret = -ECANCELED;
 		break;
 	default:
 		strcomp_status = "Unknown";
+		ret = -EINVAL;
 		break;
 	}
 
 	if (!strcomp_status)
-		return 0;
+		return ret;
 
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
@@ -560,7 +568,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
 
-	return -EFAULT;
+	return ret;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)
@@ -568,13 +576,13 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
 	struct device *dev = &pcie->pdev->dev;
 	int i;
 
-	for (i = 0; i < PIO_RETRY_CNT; i++) {
+	for (i = 1; i <= PIO_RETRY_CNT; i++) {
 		u32 start, isr;
 
 		start = advk_readl(pcie, PIO_START);
 		isr = advk_readl(pcie, PIO_ISR);
 		if (!start && isr)
-			return 0;
+			return i;
 		udelay(PIO_RETRY_DELAY);
 	}
 
@@ -764,6 +772,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 			     int where, int size, u32 *val)
 {
 	struct advk_pcie *pcie = bus->sysdata;
+	int retry_count;
 	bool allow_crs;
 	u32 reg;
 	int ret;
@@ -816,6 +825,9 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	/* Program the data strobe */
 	advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
 
+	retry_count = 0;
+
+retry:
 	/* Clear PIO DONE ISR and start the transfer */
 	advk_writel(pcie, 1, PIO_ISR);
 	advk_writel(pcie, 1, PIO_START);
@@ -834,8 +846,12 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 		return PCIBIOS_SET_FAILED;
 	}
 
+	retry_count += ret;
+
 	/* Check PIO status and get the read result */
 	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
+	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
+		goto retry;
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
@@ -855,6 +871,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	struct advk_pcie *pcie = bus->sysdata;
 	u32 reg;
 	u32 data_strobe = 0x0;
+	int retry_count;
 	int offset;
 	int ret;
 
@@ -896,6 +913,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	/* Program the data strobe */
 	advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
 
+	retry_count = 0;
+
+retry:
 	/* Clear PIO DONE ISR and start the transfer */
 	advk_writel(pcie, 1, PIO_ISR);
 	advk_writel(pcie, 1, PIO_START);
@@ -904,7 +924,11 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
+	retry_count += ret;
+
 	ret = advk_pcie_check_pio_status(pcie, false, NULL);
+	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
+		goto retry;
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-- 
2.20.1

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Pali Rohár]

On Monday 23 August 2021 14:02:14 Pali Rohár wrote:
> Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> response as failed transaction (due to simplicity).
> 
> But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> for PIO config response and implementation of re-issuing config requests
> according to PCIe base specification is therefore simple.
> 
> This change implements re-issuing of config requests when response is CRS.
> And to prevent infinite loop set upper bound to around PIO_RETRY_CNT value,
> after which is transaction marked as failed and 0xFFFFFFFF is returned like
> before.
> 
> Implementation is done by returning appropriate error codes from function
> advk_pcie_check_pio_status(). On CRS is returned -EAGAIN and caller then
> reissue transaction up to the PIO_RETRY_CNT count. As advk_pcie_wait_pio()
> function waits some cycles, return number of these cycles and add them to
> the retry count. So the total time for config request would be only linear
> O(PIO_RETRY_CNT) and not quadratic O(PIO_RETRY_CNT^2) in the worst case.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")

Hello! Could you review this patch?

> ---
>  drivers/pci/controller/pci-aardvark.c | 36 ++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index abc93225ba20..99f244190eae 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -470,6 +470,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  	u32 reg;
>  	unsigned int status;
>  	char *strcomp_status, *str_posted;
> +	int ret;
>  
>  	reg = advk_readl(pcie, PIO_STAT);
>  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> @@ -494,6 +495,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  	case PIO_COMPLETION_STATUS_OK:
>  		if (reg & PIO_ERR_STATUS) {
>  			strcomp_status = "COMP_ERR";
> +			ret = -EFAULT;
>  			break;
>  		}
>  		/* Get the read result */
> @@ -501,9 +503,11 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  			*val = advk_readl(pcie, PIO_RD_DATA);
>  		/* No error */
>  		strcomp_status = NULL;
> +		ret = 0;
>  		break;
>  	case PIO_COMPLETION_STATUS_UR:
>  		strcomp_status = "UR";
> +		ret = -EOPNOTSUPP;
>  		break;
>  	case PIO_COMPLETION_STATUS_CRS:
>  		if (allow_crs && val) {
> @@ -521,6 +525,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  			 */
>  			*val = CFG_RD_CRS_VAL;
>  			strcomp_status = NULL;
> +			ret = 0;
>  			break;
>  		}
>  		/* PCIe r4.0, sec 2.3.2, says:
> @@ -536,21 +541,24 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  		 * Request and taking appropriate action, e.g., complete the
>  		 * Request to the host as a failed transaction.
>  		 *
> -		 * To simplify implementation do not re-issue the Configuration
> -		 * Request and complete the Request as a failed transaction.
> +		 * So return -EAGAIN and caller (pci-aardvark.c driver) will
> +		 * re-issue request again up to the PIO_RETRY_CNT retries.
>  		 */
>  		strcomp_status = "CRS";
> +		ret = -EAGAIN;
>  		break;
>  	case PIO_COMPLETION_STATUS_CA:
>  		strcomp_status = "CA";
> +		ret = -ECANCELED;
>  		break;
>  	default:
>  		strcomp_status = "Unknown";
> +		ret = -EINVAL;
>  		break;
>  	}
>  
>  	if (!strcomp_status)
> -		return 0;
> +		return ret;
>  
>  	if (reg & PIO_NON_POSTED_REQ)
>  		str_posted = "Non-posted";
> @@ -560,7 +568,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
>  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
>  
> -	return -EFAULT;
> +	return ret;
>  }
>  
>  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> @@ -568,13 +576,13 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
>  	struct device *dev = &pcie->pdev->dev;
>  	int i;
>  
> -	for (i = 0; i < PIO_RETRY_CNT; i++) {
> +	for (i = 1; i <= PIO_RETRY_CNT; i++) {
>  		u32 start, isr;
>  
>  		start = advk_readl(pcie, PIO_START);
>  		isr = advk_readl(pcie, PIO_ISR);
>  		if (!start && isr)
> -			return 0;
> +			return i;
>  		udelay(PIO_RETRY_DELAY);
>  	}
>  
> @@ -764,6 +772,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  			     int where, int size, u32 *val)
>  {
>  	struct advk_pcie *pcie = bus->sysdata;
> +	int retry_count;
>  	bool allow_crs;
>  	u32 reg;
>  	int ret;
> @@ -816,6 +825,9 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	/* Program the data strobe */
>  	advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
>  
> +	retry_count = 0;
> +
> +retry:
>  	/* Clear PIO DONE ISR and start the transfer */
>  	advk_writel(pcie, 1, PIO_ISR);
>  	advk_writel(pcie, 1, PIO_START);
> @@ -834,8 +846,12 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  		return PCIBIOS_SET_FAILED;
>  	}
>  
> +	retry_count += ret;
> +
>  	/* Check PIO status and get the read result */
>  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> +	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
> +		goto retry;
>  	if (ret < 0) {
>  		*val = 0xffffffff;
>  		return PCIBIOS_SET_FAILED;
> @@ -855,6 +871,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	struct advk_pcie *pcie = bus->sysdata;
>  	u32 reg;
>  	u32 data_strobe = 0x0;
> +	int retry_count;
>  	int offset;
>  	int ret;
>  
> @@ -896,6 +913,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	/* Program the data strobe */
>  	advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
>  
> +	retry_count = 0;
> +
> +retry:
>  	/* Clear PIO DONE ISR and start the transfer */
>  	advk_writel(pcie, 1, PIO_ISR);
>  	advk_writel(pcie, 1, PIO_START);
> @@ -904,7 +924,11 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	if (ret < 0)
>  		return PCIBIOS_SET_FAILED;
>  
> +	retry_count += ret;
> +
>  	ret = advk_pcie_check_pio_status(pcie, false, NULL);
> +	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
> +		goto retry;
>  	if (ret < 0)
>  		return PCIBIOS_SET_FAILED;
>  
> -- 
> 2.20.1
> 

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Bjorn Helgaas]

On Mon, Aug 23, 2021 at 02:02:14PM +0200, Pali Rohár wrote:
> Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> response as failed transaction (due to simplicity).
> 
> But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> for PIO config response and implementation of re-issuing config requests
> according to PCIe base specification is therefore simple.

I think the spec is confusingly worded.  It says (PCIe r5.0, sec
2.3.2) that when handling a Completion with CRS status for a config
request (paraphrasing slightly),

  If CRS Software Visibility is enabled, for config reads of Vendor
  ID, the Root Complex returns 0x0001 for Vendor ID.

  Otherwise ... the Root Complex must re-issue the Configuration
  Request as a new Request.

BUT:

  A Root Complex implementation may choose to limit the number of
  Configuration Request/ CRS Completion Status loops before
  determining that something is wrong with the target of the Request
  and taking appropriate action, e.g., complete the Request to the
  host as a failed transaction.

So I think zero is a perfectly valid number of retries, and I'm pretty
sure there are RCs that never retry.

Is there a benefit to doing retry like this in the driver?  Can we not
simply rely on retries at a higher level?

> This change implements re-issuing of config requests when response is CRS.
> And to prevent infinite loop set upper bound to around PIO_RETRY_CNT value,
> after which is transaction marked as failed and 0xFFFFFFFF is returned like
> before.
> 
> Implementation is done by returning appropriate error codes from function
> advk_pcie_check_pio_status(). On CRS is returned -EAGAIN and caller then
> reissue transaction up to the PIO_RETRY_CNT count. As advk_pcie_wait_pio()
> function waits some cycles, return number of these cycles and add them to
> the retry count. So the total time for config request would be only linear
> O(PIO_RETRY_CNT) and not quadratic O(PIO_RETRY_CNT^2) in the worst case.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
> ---
>  drivers/pci/controller/pci-aardvark.c | 36 ++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index abc93225ba20..99f244190eae 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -470,6 +470,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  	u32 reg;
>  	unsigned int status;
>  	char *strcomp_status, *str_posted;
> +	int ret;
>  
>  	reg = advk_readl(pcie, PIO_STAT);
>  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> @@ -494,6 +495,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  	case PIO_COMPLETION_STATUS_OK:
>  		if (reg & PIO_ERR_STATUS) {
>  			strcomp_status = "COMP_ERR";
> +			ret = -EFAULT;
>  			break;
>  		}
>  		/* Get the read result */
> @@ -501,9 +503,11 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  			*val = advk_readl(pcie, PIO_RD_DATA);
>  		/* No error */
>  		strcomp_status = NULL;
> +		ret = 0;
>  		break;
>  	case PIO_COMPLETION_STATUS_UR:
>  		strcomp_status = "UR";
> +		ret = -EOPNOTSUPP;
>  		break;
>  	case PIO_COMPLETION_STATUS_CRS:
>  		if (allow_crs && val) {
> @@ -521,6 +525,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  			 */
>  			*val = CFG_RD_CRS_VAL;
>  			strcomp_status = NULL;
> +			ret = 0;
>  			break;
>  		}
>  		/* PCIe r4.0, sec 2.3.2, says:
> @@ -536,21 +541,24 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  		 * Request and taking appropriate action, e.g., complete the
>  		 * Request to the host as a failed transaction.
>  		 *
> -		 * To simplify implementation do not re-issue the Configuration
> -		 * Request and complete the Request as a failed transaction.
> +		 * So return -EAGAIN and caller (pci-aardvark.c driver) will
> +		 * re-issue request again up to the PIO_RETRY_CNT retries.
>  		 */
>  		strcomp_status = "CRS";
> +		ret = -EAGAIN;
>  		break;
>  	case PIO_COMPLETION_STATUS_CA:
>  		strcomp_status = "CA";
> +		ret = -ECANCELED;
>  		break;
>  	default:
>  		strcomp_status = "Unknown";
> +		ret = -EINVAL;
>  		break;
>  	}
>  
>  	if (!strcomp_status)
> -		return 0;
> +		return ret;
>  
>  	if (reg & PIO_NON_POSTED_REQ)
>  		str_posted = "Non-posted";
> @@ -560,7 +568,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
>  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
>  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
>  
> -	return -EFAULT;
> +	return ret;
>  }
>  
>  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> @@ -568,13 +576,13 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
>  	struct device *dev = &pcie->pdev->dev;
>  	int i;
>  
> -	for (i = 0; i < PIO_RETRY_CNT; i++) {
> +	for (i = 1; i <= PIO_RETRY_CNT; i++) {
>  		u32 start, isr;
>  
>  		start = advk_readl(pcie, PIO_START);
>  		isr = advk_readl(pcie, PIO_ISR);
>  		if (!start && isr)
> -			return 0;
> +			return i;
>  		udelay(PIO_RETRY_DELAY);
>  	}
>  
> @@ -764,6 +772,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  			     int where, int size, u32 *val)
>  {
>  	struct advk_pcie *pcie = bus->sysdata;
> +	int retry_count;
>  	bool allow_crs;
>  	u32 reg;
>  	int ret;
> @@ -816,6 +825,9 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	/* Program the data strobe */
>  	advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
>  
> +	retry_count = 0;
> +
> +retry:
>  	/* Clear PIO DONE ISR and start the transfer */
>  	advk_writel(pcie, 1, PIO_ISR);
>  	advk_writel(pcie, 1, PIO_START);
> @@ -834,8 +846,12 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  		return PCIBIOS_SET_FAILED;
>  	}
>  
> +	retry_count += ret;
> +
>  	/* Check PIO status and get the read result */
>  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> +	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
> +		goto retry;
>  	if (ret < 0) {
>  		*val = 0xffffffff;
>  		return PCIBIOS_SET_FAILED;
> @@ -855,6 +871,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	struct advk_pcie *pcie = bus->sysdata;
>  	u32 reg;
>  	u32 data_strobe = 0x0;
> +	int retry_count;
>  	int offset;
>  	int ret;
>  
> @@ -896,6 +913,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	/* Program the data strobe */
>  	advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
>  
> +	retry_count = 0;
> +
> +retry:
>  	/* Clear PIO DONE ISR and start the transfer */
>  	advk_writel(pcie, 1, PIO_ISR);
>  	advk_writel(pcie, 1, PIO_START);
> @@ -904,7 +924,11 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	if (ret < 0)
>  		return PCIBIOS_SET_FAILED;
>  
> +	retry_count += ret;
> +
>  	ret = advk_pcie_check_pio_status(pcie, false, NULL);
> +	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
> +		goto retry;
>  	if (ret < 0)
>  		return PCIBIOS_SET_FAILED;
>  
> -- 
> 2.20.1
> 

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Pali Rohár]

On Tuesday 14 September 2021 15:26:56 Bjorn Helgaas wrote:
> On Mon, Aug 23, 2021 at 02:02:14PM +0200, Pali Rohár wrote:
> > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > response as failed transaction (due to simplicity).
> > 
> > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > for PIO config response and implementation of re-issuing config requests
> > according to PCIe base specification is therefore simple.
> 
> I think the spec is confusingly worded.  It says (PCIe r5.0, sec
> 2.3.2) that when handling a Completion with CRS status for a config
> request (paraphrasing slightly),
> 
>   If CRS Software Visibility is enabled, for config reads of Vendor
>   ID, the Root Complex returns 0x0001 for Vendor ID.
> 
>   Otherwise ... the Root Complex must re-issue the Configuration
>   Request as a new Request.
> 
> BUT:
> 
>   A Root Complex implementation may choose to limit the number of
>   Configuration Request/ CRS Completion Status loops before
>   determining that something is wrong with the target of the Request
>   and taking appropriate action, e.g., complete the Request to the
>   host as a failed transaction.
> 
> So I think zero is a perfectly valid number of retries, and I'm pretty
> sure there are RCs that never retry.
> 
> Is there a benefit to doing retry like this in the driver?  Can we not
> simply rely on retries at a higher level?

I think that all drivers handle 0xFFFFFFFF read response as some kind of
fatal error. And because every PCI error is mapped to value 0xFFFFFFFF
it means that higher level has no chance to distinguish easily between
unsupported request and completion retry status.

And issue is there also with write requests. Is somebody checking return
value of pci_bus_write_config function?

I guess that zero retry count as you pointed is valid. But it is
something which we want?

I sent this patch because implementation of request retry was very
simple. Driver already waits for response, so adding another loop around
it does not increase code complexity.

> > This change implements re-issuing of config requests when response is CRS.
> > And to prevent infinite loop set upper bound to around PIO_RETRY_CNT value,
> > after which is transaction marked as failed and 0xFFFFFFFF is returned like
> > before.
> > 
> > Implementation is done by returning appropriate error codes from function
> > advk_pcie_check_pio_status(). On CRS is returned -EAGAIN and caller then
> > reissue transaction up to the PIO_RETRY_CNT count. As advk_pcie_wait_pio()
> > function waits some cycles, return number of these cycles and add them to
> > the retry count. So the total time for config request would be only linear
> > O(PIO_RETRY_CNT) and not quadratic O(PIO_RETRY_CNT^2) in the worst case.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 36 ++++++++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index abc93225ba20..99f244190eae 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -470,6 +470,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> >  	u32 reg;
> >  	unsigned int status;
> >  	char *strcomp_status, *str_posted;
> > +	int ret;
> >  
> >  	reg = advk_readl(pcie, PIO_STAT);
> >  	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> > @@ -494,6 +495,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> >  	case PIO_COMPLETION_STATUS_OK:
> >  		if (reg & PIO_ERR_STATUS) {
> >  			strcomp_status = "COMP_ERR";
> > +			ret = -EFAULT;
> >  			break;
> >  		}
> >  		/* Get the read result */
> > @@ -501,9 +503,11 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> >  			*val = advk_readl(pcie, PIO_RD_DATA);
> >  		/* No error */
> >  		strcomp_status = NULL;
> > +		ret = 0;
> >  		break;
> >  	case PIO_COMPLETION_STATUS_UR:
> >  		strcomp_status = "UR";
> > +		ret = -EOPNOTSUPP;
> >  		break;
> >  	case PIO_COMPLETION_STATUS_CRS:
> >  		if (allow_crs && val) {
> > @@ -521,6 +525,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> >  			 */
> >  			*val = CFG_RD_CRS_VAL;
> >  			strcomp_status = NULL;
> > +			ret = 0;
> >  			break;
> >  		}
> >  		/* PCIe r4.0, sec 2.3.2, says:
> > @@ -536,21 +541,24 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> >  		 * Request and taking appropriate action, e.g., complete the
> >  		 * Request to the host as a failed transaction.
> >  		 *
> > -		 * To simplify implementation do not re-issue the Configuration
> > -		 * Request and complete the Request as a failed transaction.
> > +		 * So return -EAGAIN and caller (pci-aardvark.c driver) will
> > +		 * re-issue request again up to the PIO_RETRY_CNT retries.
> >  		 */
> >  		strcomp_status = "CRS";
> > +		ret = -EAGAIN;
> >  		break;
> >  	case PIO_COMPLETION_STATUS_CA:
> >  		strcomp_status = "CA";
> > +		ret = -ECANCELED;
> >  		break;
> >  	default:
> >  		strcomp_status = "Unknown";
> > +		ret = -EINVAL;
> >  		break;
> >  	}
> >  
> >  	if (!strcomp_status)
> > -		return 0;
> > +		return ret;
> >  
> >  	if (reg & PIO_NON_POSTED_REQ)
> >  		str_posted = "Non-posted";
> > @@ -560,7 +568,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> >  	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> >  		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> >  
> > -	return -EFAULT;
> > +	return ret;
> >  }
> >  
> >  static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > @@ -568,13 +576,13 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> >  	struct device *dev = &pcie->pdev->dev;
> >  	int i;
> >  
> > -	for (i = 0; i < PIO_RETRY_CNT; i++) {
> > +	for (i = 1; i <= PIO_RETRY_CNT; i++) {
> >  		u32 start, isr;
> >  
> >  		start = advk_readl(pcie, PIO_START);
> >  		isr = advk_readl(pcie, PIO_ISR);
> >  		if (!start && isr)
> > -			return 0;
> > +			return i;
> >  		udelay(PIO_RETRY_DELAY);
> >  	}
> >  
> > @@ -764,6 +772,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  			     int where, int size, u32 *val)
> >  {
> >  	struct advk_pcie *pcie = bus->sysdata;
> > +	int retry_count;
> >  	bool allow_crs;
> >  	u32 reg;
> >  	int ret;
> > @@ -816,6 +825,9 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  	/* Program the data strobe */
> >  	advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
> >  
> > +	retry_count = 0;
> > +
> > +retry:
> >  	/* Clear PIO DONE ISR and start the transfer */
> >  	advk_writel(pcie, 1, PIO_ISR);
> >  	advk_writel(pcie, 1, PIO_START);
> > @@ -834,8 +846,12 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  		return PCIBIOS_SET_FAILED;
> >  	}
> >  
> > +	retry_count += ret;
> > +
> >  	/* Check PIO status and get the read result */
> >  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> > +	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
> > +		goto retry;
> >  	if (ret < 0) {
> >  		*val = 0xffffffff;
> >  		return PCIBIOS_SET_FAILED;
> > @@ -855,6 +871,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> >  	struct advk_pcie *pcie = bus->sysdata;
> >  	u32 reg;
> >  	u32 data_strobe = 0x0;
> > +	int retry_count;
> >  	int offset;
> >  	int ret;
> >  
> > @@ -896,6 +913,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> >  	/* Program the data strobe */
> >  	advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
> >  
> > +	retry_count = 0;
> > +
> > +retry:
> >  	/* Clear PIO DONE ISR and start the transfer */
> >  	advk_writel(pcie, 1, PIO_ISR);
> >  	advk_writel(pcie, 1, PIO_START);
> > @@ -904,7 +924,11 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> >  	if (ret < 0)
> >  		return PCIBIOS_SET_FAILED;
> >  
> > +	retry_count += ret;
> > +
> >  	ret = advk_pcie_check_pio_status(pcie, false, NULL);
> > +	if (ret == -EAGAIN && retry_count < PIO_RETRY_CNT)
> > +		goto retry;
> >  	if (ret < 0)
> >  		return PCIBIOS_SET_FAILED;
> >  
> > -- 
> > 2.20.1
> > 

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Bjorn Helgaas]

On Tue, Sep 14, 2021 at 10:46:59PM +0200, Pali Rohár wrote:
> On Tuesday 14 September 2021 15:26:56 Bjorn Helgaas wrote:
> > On Mon, Aug 23, 2021 at 02:02:14PM +0200, Pali Rohár wrote:
> > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > response as failed transaction (due to simplicity).
> > > 
> > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > for PIO config response and implementation of re-issuing config requests
> > > according to PCIe base specification is therefore simple.
> > 
> > I think the spec is confusingly worded.  It says (PCIe r5.0, sec
> > 2.3.2) that when handling a Completion with CRS status for a config
> > request (paraphrasing slightly),
> > 
> >   If CRS Software Visibility is enabled, for config reads of Vendor
> >   ID, the Root Complex returns 0x0001 for Vendor ID.
> > 
> >   Otherwise ... the Root Complex must re-issue the Configuration
> >   Request as a new Request.
> > 
> > BUT:
> > 
> >   A Root Complex implementation may choose to limit the number of
> >   Configuration Request/ CRS Completion Status loops before
> >   determining that something is wrong with the target of the Request
> >   and taking appropriate action, e.g., complete the Request to the
> >   host as a failed transaction.
> > 
> > So I think zero is a perfectly valid number of retries, and I'm pretty
> > sure there are RCs that never retry.
> > 
> > Is there a benefit to doing retry like this in the driver?  Can we not
> > simply rely on retries at a higher level?
> 
> I think that all drivers handle 0xFFFFFFFF read response as some kind of
> fatal error.

True.

> And because every PCI error is mapped to value 0xFFFFFFFF
> it means that higher level has no chance to distinguish easily between
> unsupported request and completion retry status.

Also true.  But we don't *want* higher-level code to distinguish
these.  The only place we should ever see CRS status is during
enumeration and after reset.  Those code paths should look for CRS
status and retry as needed.

It is illegal for a device to return CRS after it has returned a
successful completion unless an intervening reset has occurred, so
drivers and other code should never see it.

> And issue is there also with write requests. Is somebody checking return
> value of pci_bus_write_config function?

Similar case here.  The enumeration and wait-after-reset paths always
do *reads* until we get a successful completion, so I don't think we
ever issue a write that can get CRS.

> I guess that zero retry count as you pointed is valid. But it is
> something which we want?
> 
> I sent this patch because implementation of request retry was very
> simple. Driver already waits for response, so adding another loop around
> it does not increase code complexity.

"Adding a loop does not increase code complexity"?  Well, maybe not
MUCH, but it is a little, and the analysis behind it is fairly
significant.

Bjorn

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Pali Rohár]

On Tuesday 14 September 2021 15:55:26 Bjorn Helgaas wrote:
> On Tue, Sep 14, 2021 at 10:46:59PM +0200, Pali Rohár wrote:
> > On Tuesday 14 September 2021 15:26:56 Bjorn Helgaas wrote:
> > > On Mon, Aug 23, 2021 at 02:02:14PM +0200, Pali Rohár wrote:
> > > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > > response as failed transaction (due to simplicity).
> > > > 
> > > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > > for PIO config response and implementation of re-issuing config requests
> > > > according to PCIe base specification is therefore simple.
> > > 
> > > I think the spec is confusingly worded.  It says (PCIe r5.0, sec
> > > 2.3.2) that when handling a Completion with CRS status for a config
> > > request (paraphrasing slightly),
> > > 
> > >   If CRS Software Visibility is enabled, for config reads of Vendor
> > >   ID, the Root Complex returns 0x0001 for Vendor ID.
> > > 
> > >   Otherwise ... the Root Complex must re-issue the Configuration
> > >   Request as a new Request.
> > > 
> > > BUT:
> > > 
> > >   A Root Complex implementation may choose to limit the number of
> > >   Configuration Request/ CRS Completion Status loops before
> > >   determining that something is wrong with the target of the Request
> > >   and taking appropriate action, e.g., complete the Request to the
> > >   host as a failed transaction.
> > > 
> > > So I think zero is a perfectly valid number of retries, and I'm pretty
> > > sure there are RCs that never retry.
> > > 
> > > Is there a benefit to doing retry like this in the driver?  Can we not
> > > simply rely on retries at a higher level?
> > 
> > I think that all drivers handle 0xFFFFFFFF read response as some kind of
> > fatal error.
> 
> True.
> 
> > And because every PCI error is mapped to value 0xFFFFFFFF
> > it means that higher level has no chance to distinguish easily between
> > unsupported request and completion retry status.
> 
> Also true.  But we don't *want* higher-level code to distinguish
> these.  The only place we should ever see CRS status is during
> enumeration and after reset.  Those code paths should look for CRS
> status and retry as needed.
> 
> It is illegal for a device to return CRS after it has returned a
> successful completion unless an intervening reset has occurred, so
> drivers and other code should never see it.
> 
> > And issue is there also with write requests. Is somebody checking return
> > value of pci_bus_write_config function?
> 
> Similar case here.  The enumeration and wait-after-reset paths always
> do *reads* until we get a successful completion, so I don't think we
> ever issue a write that can get CRS.

Yes, in normal conditions we should not see it.

But for testing purposes (that emulated bridge works fine) I'm using
setpci for changing some configuration.

And via setpci it is possible to turn off CRSSVE bit in which case then
Root Complex should re-issue request again.

I'm not sure how "legal" it is if userspace / setpci changes some of
these bits. At least on a hardware with a real Root Port device it
should be fully transparent. As hardware handles this re-issue and
kernel then would see (reissued) response.

Test case: Initialize device, then unbind it from sysfs, reset it (hot
reset or warm reset) and then rescan / reinit it again. Here device is
permitted to send CRS response.

We know that more PCIe cards are buggy and sometimes firmware on cards
crashes or resets card logic. Which may put card into initialization
state when it is again permitted to send CRS response.

> > I guess that zero retry count as you pointed is valid. But it is
> > something which we want?
> > 
> > I sent this patch because implementation of request retry was very
> > simple. Driver already waits for response, so adding another loop around
> > it does not increase code complexity.
> 
> "Adding a loop does not increase code complexity"?  Well, maybe not
> MUCH, but it is a little, and the analysis behind it is fairly
> significant.

I agree, it depends. For somebody it could be a little change which does
not harm and for somebody else it can be bigger. Maybe I'm biased here
as I patched pci-aardvark.c code more times and it could mean that patch
complexity for me is smaller as I know this code.

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Bjorn Helgaas]

On Wed, Sep 15, 2021 at 12:55:53PM +0200, Pali Rohár wrote:
> On Tuesday 14 September 2021 15:55:26 Bjorn Helgaas wrote:

> > It is illegal for a device to return CRS after it has returned a
> > successful completion unless an intervening reset has occurred, so
> > drivers and other code should never see it.
> > 
> > > And issue is there also with write requests. Is somebody checking return
> > > value of pci_bus_write_config function?
> > 
> > Similar case here.  The enumeration and wait-after-reset paths always
> > do *reads* until we get a successful completion, so I don't think we
> > ever issue a write that can get CRS.
> 
> Yes, in normal conditions we should not see it.
> 
> But for testing purposes (that emulated bridge works fine) I'm using
> setpci for changing some configuration.
> 
> And via setpci it is possible to turn off CRSSVE bit in which case then
> Root Complex should re-issue request again.
> 
> I'm not sure how "legal" it is if userspace / setpci changes some of
> these bits. At least on a hardware with a real Root Port device it
> should be fully transparent. As hardware handles this re-issue and
> kernel then would see (reissued) response.

If setpci changes bits like these, all bets are off.  We can't tell
what happened, so we can't rely on any configuration Linux did.  I
think we really should taint the kernel when this happens.

> Test case: Initialize device, then unbind it from sysfs, reset it (hot
> reset or warm reset) and then rescan / reinit it again. Here device is
> permitted to send CRS response.
> 
> We know that more PCIe cards are buggy and sometimes firmware on cards
> crashes or resets card logic. Which may put card into initialization
> state when it is again permitted to send CRS response.

Yep.  That's a buggy device and normally we would work around it with
a quirk.  This particular kind of bug would be hard to work around,
but a host bridge driver doesn't seem like the right place to do it
because we'd have to do it in *every* such driver.

Bjorn

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Pali Rohár]

On Thursday 16 September 2021 09:32:57 Bjorn Helgaas wrote:
> On Wed, Sep 15, 2021 at 12:55:53PM +0200, Pali Rohár wrote:
> > On Tuesday 14 September 2021 15:55:26 Bjorn Helgaas wrote:
> 
> > > It is illegal for a device to return CRS after it has returned a
> > > successful completion unless an intervening reset has occurred, so
> > > drivers and other code should never see it.
> > > 
> > > > And issue is there also with write requests. Is somebody checking return
> > > > value of pci_bus_write_config function?
> > > 
> > > Similar case here.  The enumeration and wait-after-reset paths always
> > > do *reads* until we get a successful completion, so I don't think we
> > > ever issue a write that can get CRS.
> > 
> > Yes, in normal conditions we should not see it.
> > 
> > But for testing purposes (that emulated bridge works fine) I'm using
> > setpci for changing some configuration.
> > 
> > And via setpci it is possible to turn off CRSSVE bit in which case then
> > Root Complex should re-issue request again.
> > 
> > I'm not sure how "legal" it is if userspace / setpci changes some of
> > these bits. At least on a hardware with a real Root Port device it
> > should be fully transparent. As hardware handles this re-issue and
> > kernel then would see (reissued) response.
> 
> If setpci changes bits like these, all bets are off.  We can't tell
> what happened, so we can't rely on any configuration Linux did.  I
> think we really should taint the kernel when this happens.

For testing purposes, setpci is still a very good tool.

> > Test case: Initialize device, then unbind it from sysfs, reset it (hot
> > reset or warm reset) and then rescan / reinit it again. Here device is
> > permitted to send CRS response.
> > 
> > We know that more PCIe cards are buggy and sometimes firmware on cards
> > crashes or resets card logic. Which may put card into initialization
> > state when it is again permitted to send CRS response.
> 
> Yep.  That's a buggy device and normally we would work around it with
> a quirk.  This particular kind of bug would be hard to work around,
> but a host bridge driver doesn't seem like the right place to do it
> because we'd have to do it in *every* such driver.

This described firmware crashing & card reset logic I saw in more wifi
cards. Sometimes even wifi drivers itself detects that card does not
respond and do some its own internal card reset (e.g. iwldvm on laptop).
So it very common situation.

But I have not seen that these cards on laptop issue CRS response. Maybe
because their firmware or PCIe logic bootup too fast (so there is a very
little window for CRS response) or because CRS response sent to OS did
not cause any issue.

So no particular workaround is needed for above described scenario.


But anyway, in case that in future there would be need for disabling CRS
feature in kernel (e.g. for doing some workaround for endpoint or
extended pcie switch) then this re-issuing of config request on CRS
response in pci-aardvark.c would be needed to have similar behavior like
real HW hen CRS is disabled.

And I like the idea if driver is "feature complete" and prepared also
for other _valid_ code paths. This is just my opinion.

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Bjorn Helgaas]

On Wed, Sep 22, 2021 at 12:17:36PM +0200, Pali Rohár wrote:
> On Thursday 16 September 2021 09:32:57 Bjorn Helgaas wrote:
> > On Wed, Sep 15, 2021 at 12:55:53PM +0200, Pali Rohár wrote:
> > ...

> > > I'm not sure how "legal" it is if userspace / setpci changes some of
> > > these bits. At least on a hardware with a real Root Port device it
> > > should be fully transparent. As hardware handles this re-issue and
> > > kernel then would see (reissued) response.
> > 
> > If setpci changes bits like these, all bets are off.  We can't tell
> > what happened, so we can't rely on any configuration Linux did.  I
> > think we really should taint the kernel when this happens.
> 
> For testing purposes, setpci is still a very good tool.

Yes, absolutely!

> > > Test case: Initialize device, then unbind it from sysfs, reset it (hot
> > > reset or warm reset) and then rescan / reinit it again. Here device is
> > > permitted to send CRS response.
> > > 
> > > We know that more PCIe cards are buggy and sometimes firmware on cards
> > > crashes or resets card logic. Which may put card into initialization
> > > state when it is again permitted to send CRS response.
> > 
> > Yep.  That's a buggy device and normally we would work around it with
> > a quirk.  This particular kind of bug would be hard to work around,
> > but a host bridge driver doesn't seem like the right place to do it
> > because we'd have to do it in *every* such driver.
> 
> This described firmware crashing & card reset logic I saw in more wifi
> cards. Sometimes even wifi drivers itself detects that card does not
> respond and do some its own internal card reset (e.g. iwldvm on laptop).
> So it very common situation.
> 
> But I have not seen that these cards on laptop issue CRS response. Maybe
> because their firmware or PCIe logic bootup too fast (so there is a very
> little window for CRS response) or because CRS response sent to OS did
> not cause any issue.

After a reset, we normally delay 100ms *before* issuing the first
config request to the device, e.g., [1].  I expect that in most cases
the device has completed its initialization in that 100ms and it never
responds with CRS status.

> So no particular workaround is needed for above described scenario.
> 
> 
> But anyway, in case that in future there would be need for disabling CRS
> feature in kernel (e.g. for doing some workaround for endpoint or
> extended pcie switch) then this re-issuing of config request on CRS
> response in pci-aardvark.c would be needed to have similar behavior like
> real HW hen CRS is disabled.

To be pedantic, there's no such thing as "disabling CRS".  CRS is a
required feature with no enable/disable bit.  There is only "enabling
or disabling CRS *Software Visibility*".

The config read of Vendor ID after a reset should be done by the PCI
core, not a device driver.  If we disable CRS SV, the only outcomes of
that read are:

  1) Valid Vendor ID data, or

  2) Failed transaction, typically reported as 0xffff data (and, I
     expect, an Unsupported Request or similar error logged)

In either case there may have been zero or more retries on PCIe.  The
PCI core needs to handle the failure case sensibly even though it has
no idea whether the read has been retried.

> And I like the idea if driver is "feature complete" and prepared also
> for other _valid_ code paths. This is just my opinion.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci.c?id=v5.14#n4651

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Pali Rohár]

On Wednesday 22 September 2021 11:48:03 Bjorn Helgaas wrote:
> On Wed, Sep 22, 2021 at 12:17:36PM +0200, Pali Rohár wrote:
> > On Thursday 16 September 2021 09:32:57 Bjorn Helgaas wrote:
> > > On Wed, Sep 15, 2021 at 12:55:53PM +0200, Pali Rohár wrote:
> > > ...
> 
> > > > I'm not sure how "legal" it is if userspace / setpci changes some of
> > > > these bits. At least on a hardware with a real Root Port device it
> > > > should be fully transparent. As hardware handles this re-issue and
> > > > kernel then would see (reissued) response.
> > > 
> > > If setpci changes bits like these, all bets are off.  We can't tell
> > > what happened, so we can't rely on any configuration Linux did.  I
> > > think we really should taint the kernel when this happens.
> > 
> > For testing purposes, setpci is still a very good tool.
> 
> Yes, absolutely!
> 
> > > > Test case: Initialize device, then unbind it from sysfs, reset it (hot
> > > > reset or warm reset) and then rescan / reinit it again. Here device is
> > > > permitted to send CRS response.
> > > > 
> > > > We know that more PCIe cards are buggy and sometimes firmware on cards
> > > > crashes or resets card logic. Which may put card into initialization
> > > > state when it is again permitted to send CRS response.
> > > 
> > > Yep.  That's a buggy device and normally we would work around it with
> > > a quirk.  This particular kind of bug would be hard to work around,
> > > but a host bridge driver doesn't seem like the right place to do it
> > > because we'd have to do it in *every* such driver.
> > 
> > This described firmware crashing & card reset logic I saw in more wifi
> > cards. Sometimes even wifi drivers itself detects that card does not
> > respond and do some its own internal card reset (e.g. iwldvm on laptop).
> > So it very common situation.
> > 
> > But I have not seen that these cards on laptop issue CRS response. Maybe
> > because their firmware or PCIe logic bootup too fast (so there is a very
> > little window for CRS response) or because CRS response sent to OS did
> > not cause any issue.
> 
> After a reset, we normally delay 100ms *before* issuing the first
> config request to the device, e.g., [1].  I expect that in most cases
> the device has completed its initialization in that 100ms and it never
> responds with CRS status.
> 
> > So no particular workaround is needed for above described scenario.
> > 
> > 
> > But anyway, in case that in future there would be need for disabling CRS
> > feature in kernel (e.g. for doing some workaround for endpoint or
> > extended pcie switch) then this re-issuing of config request on CRS
> > response in pci-aardvark.c would be needed to have similar behavior like
> > real HW hen CRS is disabled.
> 
> To be pedantic, there's no such thing as "disabling CRS".  CRS is a
> required feature with no enable/disable bit.  There is only "enabling
> or disabling CRS *Software Visibility*".

Of course, I mean that CRSSVE bit.

> The config read of Vendor ID after a reset should be done by the PCI
> core, not a device driver.

Of course. But in case of unexpected reset (which PCI code does not
detect), card driver at the same time could issue some config read/write
request.

> If we disable CRS SV, the only outcomes of
> that read are:
> 
>   1) Valid Vendor ID data, or
> 
>   2) Failed transaction, typically reported as 0xffff data (and, I
>      expect, an Unsupported Request or similar error logged)

Yes. And I think it should apply also for any other config register, not
just vendor id.

In case error reporting or AER functionality is not supported then there
would be no error logged. And PCI core / kernel does not have to know
that such thing happened.

Anyway, there is probably a candidate wifi card with "not ready" issue:
https://lore.kernel.org/linux-wireless/CAHp75Vd5iCLELx8s+Zvcj8ufd2bN6CK26soDMkZyC1CwMO2Qeg@mail.gmail.com/

> In either case there may have been zero or more retries on PCIe.  The
> PCI core needs to handle the failure case sensibly even though it has
> no idea whether the read has been retried.
> 
> > And I like the idea if driver is "feature complete" and prepared also
> > for other _valid_ code paths. This is just my opinion.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci.c?id=v5.14#n4651

Re: [PATCH] PCI: aardvark: Implement re-issuing config requests on CRS response

[Bjorn Helgaas]

On Sun, Sep 26, 2021 at 01:13:19PM +0200, Pali Rohár wrote:
> On Wednesday 22 September 2021 11:48:03 Bjorn Helgaas wrote:

> > The config read of Vendor ID after a reset should be done by the PCI
> > core, not a device driver.
> 
> Of course. But in case of unexpected reset (which PCI code does not
> detect), card driver at the same time could issue some config read/write
> request.

By "unexpected reset", you mean a reset performed autonomously by the
device, or a reset initiated by the driver without help from the PCI
core?  Either way, I think the PCI core is pretty much out of the
picture and the driver is on its own.

> > If we disable CRS SV, the only outcomes of
> > that read are:
> > 
> >   1) Valid Vendor ID data, or
> > 
> >   2) Failed transaction, typically reported as 0xffff data (and, I
> >      expect, an Unsupported Request or similar error logged)
> 
> Yes. And I think it should apply also for any other config register, not
> just vendor id.

Yes.

> In case error reporting or AER functionality is not supported then there
> would be no error logged. And PCI core / kernel does not have to know
> that such thing happened.

There *should* be at least the logging in Device Status for all PCIe
devices, though I'm not sure the PCI core handles that nicely.  I'm
looking at PCIe r5.0, sec 6.2.5: https://imgur.com/a/0yqygiM

Bjorn