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

Stefan Roese
2021-08-11T08:29:24Z

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

Applied to u-boot-marvell/master

Thanks,
Stefan

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

Viele Grüße,
Stefan

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