PCI: aardvark: PIO fixes

drivers/pci/controller/pci-aardvark.c +30 -5

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {
+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;

drivers/pci/controller/pci-aardvark.c +9 -0

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";

@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else

@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)

@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 

drivers/pci/controller/pci-aardvark.c +23 -5

+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */

@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)

@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }

Signature · 1 line

2.20.1

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

Quoted 48 lines

drivers/pci/controller/pci-aardvark.c +34 -5

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {
+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;
+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "UR";
+		}

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 518

Quoted 6 lines

drivers/pci/controller/pci-aardvark.c +3 -1

 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		strcomp_status = "CRS";
+		if (val) {
+			/* For reading, CRS is not an error status */
+			*val = CFG_RD_CRS_VAL;

Need Bjorn's input on this.

I don't think this is what is expected from
from a root complex according to the PCI specifications (depending on whether CSR software visibility is supported or not).

Here we are fabricating a CRS completion value for all PCI config read transactions that are hitting a CRS completion status (and that's not the expected behaviour according to the PCI specifications and I don't think that's correct).

Quoted 29 lines

drivers/pci/controller/pci-aardvark.c +9 -0

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";

@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else

@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)

@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 

drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 756

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -2

 	if (advk_pcie_pio_is_running(pcie)) {
-		*val = 0xffffffff;
-		return PCIBIOS_SET_FAILED;
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again insted of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;

Mmmm..here we are faking a CRS completion value to coerce the kernel into believing a CRS completion was received (which is not necessarily true) ?

if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

Lorenzo

Quoted 53 lines

drivers/pci/controller/pci-aardvark.c +23 -5

+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */

@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)

@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }

Signature · 1 line

2.20.1

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

Quoted 36 lines

drivers/pci/controller/pci-aardvark.c +24 -4

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 509

Quoted 8 lines

drivers/pci/controller/pci-aardvark.c +6 -1

+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {
+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 518

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -1

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "UR";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		strcomp_status = "CRS";
+		if (val) {
+			/* For reading, CRS is not an error status */
+			*val = CFG_RD_CRS_VAL;

Need Bjorn's input on this. I don't think this is what is expected from from a root complex according to the PCI specifications (depending on whether CSR software visibility is supported or not).

Here we are fabricating a CRS completion value for all PCI config read transactions that are hitting a CRS completion status (and that's not the expected behaviour according to the PCI specifications and I don't think that's correct).

Quoted 29 lines

drivers/pci/controller/pci-aardvark.c +9 -0

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";

@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else

@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)

@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 

drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 756

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -2

 	if (advk_pcie_pio_is_running(pcie)) {
-		*val = 0xffffffff;
-		return PCIBIOS_SET_FAILED;
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again insted of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;

Mmmm..here we are faking a CRS completion value to coerce the kernel into believing a CRS completion was received (which is not necessarily true) ?

if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

Lorenzo

Quoted 53 lines

drivers/pci/controller/pci-aardvark.c +23 -5

+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */

@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)

@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }

Signature · 1 line

2.20.1

Quoted 46 lines · Lorenzo Pieralisi

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

Quoted 42 lines

drivers/pci/controller/pci-aardvark.c +28 -5

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 509

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c +2 -0

+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;

I think the comment is incorrect. Unsupported Request *is* an error status. But most platforms log it and fabricate ~0 data (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're doing here. So I think the code is fine, but the "not an error status" comment is wrong.

Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the hardware should be setting the "Unsupported Request Detected" bit in the Device Status register when this occurs.

Quoted 15 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 518

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -1

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "UR";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		strcomp_status = "CRS";
+		if (val) {
+			/* For reading, CRS is not an error status */
+			*val = CFG_RD_CRS_VAL;

Need Bjorn's input on this. I don't think this is what is expected from from a root complex according to the PCI specifications (depending on whether CSR software visibility is supported or not).

Quoted 4 lines

Here we are fabricating a CRS completion value for all PCI config read transactions that are hitting a CRS completion status (and that's not the expected behaviour according to the PCI specifications and I don't think that's correct).

Right. I think any config access (read or write) can be completed with a CRS completion (sec 2.3.1).

Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is enabled and a config read that includes both bytes of the Vendor ID receives a CRS completion, we must return 0x0001 for the Vendor ID and 0xff for any additional bytes. Note that a config read of only the two Vendor ID bytes is legal and should receive 0x0001 data.

But if CRS SV is disabled, I think config reads that receive CRS completions should fail the normal way, i.e., fabricate ~0 data.

Quoted 101 lines

Quoted 29 lines

drivers/pci/controller/pci-aardvark.c +9 -0

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";

@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else

@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)

@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 

drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 756

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -2

 	if (advk_pcie_pio_is_running(pcie)) {
-		*val = 0xffffffff;
-		return PCIBIOS_SET_FAILED;
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again insted of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;

Mmmm..here we are faking a CRS completion value to coerce the kernel into believing a CRS completion was received (which is not necessarily true) ?

if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

Lorenzo

Quoted 53 lines

drivers/pci/controller/pci-aardvark.c +23 -5

+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */

@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)

@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }

Signature · 1 line

2.20.1

Quoted 47 lines · On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:

Quoted 46 lines · Lorenzo Pieralisi

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

Quoted 42 lines

drivers/pci/controller/pci-aardvark.c +28 -5

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {

Quoted 8 lines

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 509

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c +2 -0

+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;

I think the comment is incorrect. Unsupported Request *is* an error status. But most platforms log it and fabricate ~0 data (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're doing here. So I think the code is fine, but the "not an error status" comment is wrong.

Ok, and what we should driver set as return value for pci_ops.read callback in this case?

Quoted 3 lines

Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the hardware should be setting the "Unsupported Request Detected" bit in the Device Status register when this occurs.

Yes there is register in kernel's emulated PCIe bridge which at bit 19
has: Unsupported Request Detected - The core sets this bit to 1 when an unsupported request is received. Write this bit to 1 to clear.

Quoted 23 lines

Quoted 15 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 518

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -1

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "UR";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		strcomp_status = "CRS";
+		if (val) {
+			/* For reading, CRS is not an error status */
+			*val = CFG_RD_CRS_VAL;

Need Bjorn's input on this. I don't think this is what is expected from from a root complex according to the PCI specifications (depending on whether CSR software visibility is supported or not).

Quoted 4 lines

Here we are fabricating a CRS completion value for all PCI config read transactions that are hitting a CRS completion status (and that's not the expected behaviour according to the PCI specifications and I don't think that's correct).

Right. I think any config access (read or write) can be completed with a CRS completion (sec 2.3.1).

Quoted 8 lines

Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is enabled and a config read that includes both bytes of the Vendor ID receives a CRS completion, we must return 0x0001 for the Vendor ID and 0xff for any additional bytes. Note that a config read of only the two Vendor ID bytes is legal and should receive 0x0001 data.

But if CRS SV is disabled, I think config reads that receive CRS completions should fail the normal way, i.e., fabricate ~0 data.

In PCIe base 2.0 is:

For other Configuration Requests, or when CRS Software Visibility is not enabled, the Root Complex will generally re-issue the Configuration Request until it completes with a status other than CRS as described in Section 2.3.2.

So what should pci-aardvark driver in this case do? Return ~0 or re-send this config read request (and how many times)?

Also this relates to previous discussion about PCI_EXP_RTCTL_CRSSVE:
https://lore.kernel.org/linux-pci/20210507152542.sd54lk7bk56qapf3@pali/

Quoted 101 lines

Quoted 101 lines

Quoted 29 lines

drivers/pci/controller/pci-aardvark.c +9 -0

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";

@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else

@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)

@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 

drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 756

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -2

 	if (advk_pcie_pio_is_running(pcie)) {
-		*val = 0xffffffff;
-		return PCIBIOS_SET_FAILED;
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again insted of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;

Mmmm..here we are faking a CRS completion value to coerce the kernel into believing a CRS completion was received (which is not necessarily true) ?

if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

Lorenzo

Quoted 53 lines

drivers/pci/controller/pci-aardvark.c +23 -5

+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */

@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)

@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }

Signature · 1 line

2.20.1

Quoted 57 lines · Pali Rohár

Quoted 55 lines · On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:

Quoted 40 lines · Lorenzo Pieralisi

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

Quoted 36 lines

drivers/pci/controller/pci-aardvark.c +24 -4

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);

Quoted 8 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 509

Quoted 8 lines

drivers/pci/controller/pci-aardvark.c +6 -1

+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {
+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;

I think the comment is incorrect. Unsupported Request *is* an error status. But most platforms log it and fabricate ~0 data (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're doing here. So I think the code is fine, but the "not an error status" comment is wrong.

Quoted 2 lines

Ok, and what we should driver set as return value for pci_ops.read callback in this case?

On most platforms, pci_ops.read() does not check for failure, so it returns PCIBIOS_SUCCESSFUL in this case.

Quoted 7 lines

Quoted 3 lines

Per the flowchart in PCIe r5.0, sec 6.2.5., fig 6-2, I think the hardware should be setting the "Unsupported Request Detected" bit in the Device Status register when this occurs.

Yes there is register in kernel's emulated PCIe bridge which at bit 19
has: Unsupported Request Detected - The core sets this bit to 1 when an unsupported request is received. Write this bit to 1 to clear.

I guess this bit 19 is the same as bit 3 in the 16-bit Device Status register? Bit 19 if you look at Device Control + Device Status as a single 32-bit register?

Quoted 32 lines

Quoted 23 lines

Quoted 15 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 518

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -1

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "UR";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CRS:
-		strcomp_status = "CRS";
+		if (val) {
+			/* For reading, CRS is not an error status */
+			*val = CFG_RD_CRS_VAL;

Need Bjorn's input on this. I don't think this is what is expected from from a root complex according to the PCI specifications (depending on whether CSR software visibility is supported or not).

Quoted 4 lines

Here we are fabricating a CRS completion value for all PCI config read transactions that are hitting a CRS completion status (and that's not the expected behaviour according to the PCI specifications and I don't think that's correct).

Right. I think any config access (read or write) can be completed with a CRS completion (sec 2.3.1).

Quoted 8 lines

Per sec 2.3.2, when CRS SV (in Root Control register, sec 7.5.3.12) is enabled and a config read that includes both bytes of the Vendor ID receives a CRS completion, we must return 0x0001 for the Vendor ID and 0xff for any additional bytes. Note that a config read of only the two Vendor ID bytes is legal and should receive 0x0001 data.

But if CRS SV is disabled, I think config reads that receive CRS completions should fail the normal way, i.e., fabricate ~0 data.

Quoted 6 lines

In PCIe base 2.0 is:

For other Configuration Requests, or when CRS Software Visibility is not enabled, the Root Complex will generally re-issue the Configuration Request until it completes with a status other than CRS as described in Section 2.3.2.

That text is from the "Configuration Request Retry Status" implementation note in PCIe r2.0, sec 2.3.1, "Request Handling Rules", and PCIe r5.0 contains the same text.

PCIe r4.0, sec 2.3.2, says (r5.0 contains the same text but with a formatting error that changes the meaning):

Root Complex handling of a Completion with Configuration Request Retry Status for a Configuration Request is implementation specific, except for the period following system reset (see Section 6.6). For Root Complexes that support CRS Software Visibility, the following rules apply:

* If CRS Software Visibility is not enabled, the Root Complex must re-issue the Configuration Request as a new Request.

* If CRS Software Visibility is enabled (see below):

- For a Configuration Read Request that includes both bytes of the Vendor ID field of a device Function's Configuration Space Header, the Root Complex must complete the Request to the host by returning a read-data value of 0001h for the Vendor ID field and all ‘1’s for any additional bytes included in the request. This read-data value has been reserved specifically for this use by the PCI-SIG and does not correspond to any assigned Vendor ID.

- For a Configuration Write Request or for any other Configuration Read Request, the Root Complex must re-issue the Configuration Request as a new Request.

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.

Quoted 2 lines

So what should pci-aardvark driver in this case do? Return ~0 or re-send this config read request (and how many times)?

That's a good question. I don't know what other hardware implementations do, but I doubt they retry forever. Since the spec doesn't specify a number of retries, I think you can choose to do none and immediately return ~0.

Quoted 104 lines

Also this relates to previous discussion about PCI_EXP_RTCTL_CRSSVE:
https://lore.kernel.org/linux-pci/20210507152542.sd54lk7bk56qapf3@pali/

Quoted 101 lines

Quoted 101 lines

Quoted 29 lines

drivers/pci/controller/pci-aardvark.c +9 -0

+			strcomp_status = NULL;
+		} else {
+			strcomp_status = "CRS";
+		}
 		break;
 	case PIO_COMPLETION_STATUS_CA:
 		strcomp_status = "CA";

@@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 		break;
 	}
 
+	if (!strcomp_status)
+		return 0;
+
 	if (reg & PIO_NON_POSTED_REQ)
 		str_posted = "Non-posted";
 	else

@@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 
 	dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
 		str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
+
+	return -EFAULT;
 }
 
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)

@@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 						 size, val);
 

drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 756

Quoted 10 lines

drivers/pci/controller/pci-aardvark.c +7 -2

 	if (advk_pcie_pio_is_running(pcie)) {
-		*val = 0xffffffff;
-		return PCIBIOS_SET_FAILED;
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again insted of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;

Mmmm..here we are faking a CRS completion value to coerce the kernel into believing a CRS completion was received (which is not necessarily true) ?

if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?

Lorenzo

Quoted 53 lines

drivers/pci/controller/pci-aardvark.c +23 -5

+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
 	}
 
 	/* Program the control register */

@@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, 1, PIO_START);
 
 	ret = advk_pcie_wait_pio(pcie);
+	if (ret < 0) {
+		/*
+		 * For PCI_VENDOR_ID register, return Completion Retry Status
+		 * so caller tries to issue the request again instead of failing
+		 */
+		if (where == PCI_VENDOR_ID) {
+			*val = CFG_RD_CRS_VAL;
+			return PCIBIOS_SUCCESSFUL;
+		} else {
+			*val = 0xffffffff;
+			return PCIBIOS_SET_FAILED;
+		}
+	}
+
+	/* Check PIO status and get the read result */
+	ret = advk_pcie_check_pio_status(pcie, val);
 	if (ret < 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_SET_FAILED;
 	}
 
-	advk_pcie_check_pio_status(pcie);
-
-	/* Get the read result */
-	*val = advk_readl(pcie, PIO_RD_DATA);
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
 	else if (size == 2)

@@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (ret < 0)
 		return PCIBIOS_SET_FAILED;
 
-	advk_pcie_check_pio_status(pcie);
+	ret = advk_pcie_check_pio_status(pcie, NULL);
+	if (ret < 0)
+		return PCIBIOS_SET_FAILED;
 
 	return PCIBIOS_SUCCESSFUL;
 }

Signature · 1 line

2.20.1

Quoted 9 lines · On Tuesday 20 July 2021 11:34:51 Bjorn Helgaas wrote:

Quoted 2 lines · Pali Rohár

So what should pci-aardvark driver in this case do? Return ~0 or re-send this config read request (and how many times)?

That's a good question. I don't know what other hardware implementations do, but I doubt they retry forever. Since the spec doesn't specify a number of retries, I think you can choose to do none and immediately return ~0.

Ok, so I will change implementation to return ~0 without any retry. This is simple and easy implementation. Anyway, it would be nice to know what other real-hardware implementations of PCIe controllers are doing.

PCI_EXP_RTCTL_CRSSVE bit is only part of kernel emulated PCIe Bridge and has no real register in aardvark hw.

So for me it looks like that aardvark's pci_ops.read() should set read value to ~0 or 0xffff0001 based on what is stored in kernel emulated PCI_EXP_RTCTL_CRSSVE bit. Right?

Quoted 61 lines · On Tuesday 20 July 2021 11:34:51 Bjorn Helgaas wrote:

Quoted 59 lines · Pali Rohár

Quoted 47 lines · On Monday 19 July 2021 18:12:27 Bjorn Helgaas wrote:

Quoted 46 lines · Lorenzo Pieralisi

On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:

[...]

Quoted 42 lines

drivers/pci/controller/pci-aardvark.c +28 -5

-static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
+static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
 {
 	struct device *dev = &pcie->pdev->dev;
 	u32 reg;

@@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
 		PIO_COMPLETION_STATUS_SHIFT;
 
-	if (!status)
-		return;
-
+	/*
+	 * According to HW spec, the PIO status check sequence as below:
+	 * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
+	 *    it still needs to check Error Status(bit11), only when this bit
+	 *    indicates no error happen, the operation is successful.
+	 * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
+	 *    means a PIO write error, and for PIO read it is successful with
+	 *    a read value of 0xFFFFFFFF.
+	 * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
+	 *    only means a PIO write error, and for PIO read it is successful
+	 *    with a read value of 0xFFFF0001.
+	 * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
+	 *    error for both PIO read and PIO write operation.
+	 * 5) other errors are indicated as 'unknown'.
+	 */
 	switch (status) {
+	case PIO_COMPLETION_STATUS_OK:
+		if (reg & PIO_ERR_STATUS) {
+			strcomp_status = "COMP_ERR";
+			break;
+		}
+		/* Get the read result */
+		if (val)
+			*val = advk_readl(pcie, PIO_RD_DATA);
+		/* No error */
+		strcomp_status = NULL;
+		break;
 	case PIO_COMPLETION_STATUS_UR:
-		strcomp_status = "UR";
+		if (val) {

Quoted 8 lines

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c static void advk_pcie_check_pio_status(struct advk_pcie *pcie) · line 509

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c +2 -0

+			/* For reading, UR is not an error status */
+			*val = CFG_RD_UR_VAL;

I think the comment is incorrect. Unsupported Request *is* an error status. But most platforms log it and fabricate ~0 data (CFG_RD_UR_VAL) to return to the CPU, and I think that's what you're doing here. So I think the code is fine, but the "not an error status" comment is wrong.

Ok, and what we should driver set as return value for pci_ops.read callback in this case?

Quoted 2 lines

On most platforms, pci_ops.read() does not check for failure, so it returns PCIBIOS_SUCCESSFUL in this case.

Ok. Most platforms do not check for failure. But it is generally correct? Probably more platforms do not provide error flag and only return value. But aardvark hw provides this kind error information, so should pci-aardvark's pci_ops.read() on error returns PCIBIOS_SUCCESSFUL on some other return value?

Per Lorenzo's request [1] I'm resending PCI aardvark patches from big patch series [2] which fixes just one small thing - aardvark PIO code.

[1] - https://lore.kernel.org/linux-pci/20210603151605.GA18917@lpieralisi/
[2] - https://lore.kernel.org/linux-pci/[email protected]/

Evan Wang (1):
PCI: aardvark: Fix checking for PIO status

[...]

This patch series fix PIO functions used for accessing PCI config space.

In v2 is fixed processing of CRS response which depends on emulation of CRSVIS and CRSSVE bits in config space of emulated PCIe bridge.

Patch "PCI: aardvark: Fix checking for PIO Non-posted Request" was dropped from v2 as it was already applied.

[...]