[PATCH 00/22] PCI: Unify PCI error response checking

[PATCH 00/22] PCI: Unify PCI error response checking

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the 
CPU read, so most hardware fabricates ~0 data.

This patch series adds PCI_ERROR_RESPONSE definition and other helper
defintion SET_PCI_ERROR_RESPONSE and RESPONSE_IS_PCI_ERROR and uses it
where appropriate to make these checks consistent and easier to find.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Patch 1:
  - Adds the PCI_ERROR_RESPONSE and other related defintions
  - All other patches are dependent on this patch. This patch needs to
    be applied first, before the others

Patch 2 - 13
  - Uses SET_PCI_ERROR_RESPONSE() when device is not found

Patch 14 - 19
  - Uses RESPONSE_IS_PCI_ERROR() to check the reads from hardware

Patch 20 - 22
  - Edits the comments to include PCI_ERROR_RESPONSE alsong with
    0xFFFFFFFF, so that it becomes easier to grep for faulty hardware
    reads.

Thanks,
Naveen

Naveen Naidu (22):
  [PATCH 1/22] PCI: Add PCI_ERROR_RESPONSE and it's related defintions
  [PATCH 2/22] PCI: Unify PCI error response checking
  [PATCH 3/22] PCI: thunder: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 4/22] PCI: iproc: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 5/22] PCI: mediatek: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 6/22] PCI: exynos: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 7/22] PCI: histb: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 8/22] PCI: kirin: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 9/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 10/22] PCI: mvebu: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 11/22] PCI: altera: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 12/22] PCI: rcar: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 13/22] PCI: rockchip: Use SET_PCI_ERROR_RESPONSE() when device not found
  [PATCH 14/22] PCI/ERR: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
  [PATCH 15/22] PCI: vmd: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
  [PATCH 16/22] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
  [PATCH 17/22] PCI/DPC: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
  [PATCH 18/22] PCI/PME: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
  [PATCH 19/22] PCI: cpqphp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
  [PATCH 20/22] PCI: keystone: Use PCI_ERROR_RESPONSE to specify hardware error
  [PATCH 21/22] PCI: hv: Use PCI_ERROR_RESPONSE to specify hardware read error
  [PATCH 22/22] PCI: xgene: Use PCI_ERROR_RESPONSE to specify hardware error

 drivers/pci/access.c                        | 22 ++++++++++-----------
 drivers/pci/controller/dwc/pci-exynos.c     |  2 +-
 drivers/pci/controller/dwc/pci-keystone.c   |  4 ++--
 drivers/pci/controller/dwc/pcie-histb.c     |  2 +-
 drivers/pci/controller/dwc/pcie-kirin.c     |  2 +-
 drivers/pci/controller/pci-aardvark.c       |  8 ++++----
 drivers/pci/controller/pci-hyperv.c         |  2 +-
 drivers/pci/controller/pci-mvebu.c          |  4 ++--
 drivers/pci/controller/pci-thunder-ecam.c   | 20 +++++++++----------
 drivers/pci/controller/pci-thunder-pem.c    |  2 +-
 drivers/pci/controller/pci-xgene.c          |  8 ++++----
 drivers/pci/controller/pcie-altera.c        |  2 +-
 drivers/pci/controller/pcie-iproc.c         |  2 +-
 drivers/pci/controller/pcie-mediatek.c      |  4 ++--
 drivers/pci/controller/pcie-rcar-host.c     |  2 +-
 drivers/pci/controller/pcie-rockchip-host.c |  2 +-
 drivers/pci/controller/vmd.c                |  2 +-
 drivers/pci/hotplug/cpqphp_ctrl.c           |  4 ++--
 drivers/pci/hotplug/pciehp_hpc.c            | 10 +++++-----
 drivers/pci/pci.c                           | 10 +++++-----
 drivers/pci/pcie/dpc.c                      |  4 ++--
 drivers/pci/pcie/pme.c                      |  4 ++--
 drivers/pci/probe.c                         | 10 +++++-----
 include/linux/pci.h                         |  9 +++++++++
 24 files changed, 75 insertions(+), 66 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/22] PCI: Add PCI_ERROR_RESPONSE and it's related defintions

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Add a PCI_ERROR_RESPONSE definition for that and use it where
appropriate to make these checks consistent and easier to find.

Also add helper definitions SET_PCI_ERROR_RESPONSE and
RESPONSE_IS_PCI_ERROR to make the code more readable.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 include/linux/pci.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index cd8aa6fce204..928c589bb5c4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -154,6 +154,15 @@ enum pci_interrupt_pin {
 /* The number of legacy PCI INTx interrupts */
 #define PCI_NUM_INTX	4
 
+/*
+ * Reading from a device that doesn't respond typically returns ~0.  A
+ * successful read from a device may also return ~0, so you need additional
+ * information to reliably identify errors.
+ */
+#define PCI_ERROR_RESPONSE			(~0ULL)
+#define SET_PCI_ERROR_RESPONSE(val)	(*val = ((typeof(*val)) PCI_ERROR_RESPONSE))
+#define RESPONSE_IS_PCI_ERROR(val)	(*val == ((typeof(*val)) PCI_ERROR_RESPONSE))
+
 /*
  * pci_power_t values must match the bits in the Capabilities PME_Support
  * and Control/Status PowerState fields in the Power Management capability.
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/22] PCI: thunder: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pci-thunder-ecam.c | 20 ++++++++++----------
 drivers/pci/controller/pci-thunder-pem.c  |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pci-thunder-ecam.c b/drivers/pci/controller/pci-thunder-ecam.c
index ffd84656544f..4c57ad2aa9aa 100644
--- a/drivers/pci/controller/pci-thunder-ecam.c
+++ b/drivers/pci/controller/pci-thunder-ecam.c
@@ -42,7 +42,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 	if (where_a == 0x4) {
 		addr = bus->ops->map_bus(bus, devfn, bar); /* BAR 0 */
 		if (!addr) {
-			*val = ~0;
+			SET_PCI_ERROR_RESPONSE(val);
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		v = readl(addr);
@@ -57,7 +57,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 
 		addr = bus->ops->map_bus(bus, devfn, bar); /* BAR 0 */
 		if (!addr) {
-			*val = ~0;
+			SET_PCI_ERROR_RESPONSE(val);
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		barl_orig = readl(addr + 0);
@@ -73,7 +73,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 	if (where_a == 0xc) {
 		addr = bus->ops->map_bus(bus, devfn, bar + 4); /* BAR 1 */
 		if (!addr) {
-			*val = ~0;
+			SET_PCI_ERROR_RESPONSE(val);
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		v = readl(addr); /* EA entry-3. Base-H */
@@ -105,7 +105,7 @@ static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, where_a);
 	if (!addr) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -136,7 +136,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, 0xc);
 	if (!addr) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -147,7 +147,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, 8);
 	if (!addr) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -177,7 +177,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, 0);
 	if (!addr) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -197,7 +197,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 		addr = bus->ops->map_bus(bus, devfn, 0x70);
 		if (!addr) {
-			*val = ~0;
+			SET_PCI_ERROR_RESPONSE(val);
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		/* E_CAP */
@@ -212,7 +212,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 		if (where_a == 0xb0) {
 			addr = bus->ops->map_bus(bus, devfn, where_a);
 			if (!addr) {
-				*val = ~0;
+				SET_PCI_ERROR_RESPONSE(val);
 				return PCIBIOS_DEVICE_NOT_FOUND;
 			}
 			v = readl(addr);
@@ -269,7 +269,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 		if (where_a == 0x70) {
 			addr = bus->ops->map_bus(bus, devfn, where_a);
 			if (!addr) {
-				*val = ~0;
+				SET_PCI_ERROR_RESPONSE(val);
 				return PCIBIOS_DEVICE_NOT_FOUND;
 			}
 			v = readl(addr);
diff --git a/drivers/pci/controller/pci-thunder-pem.c b/drivers/pci/controller/pci-thunder-pem.c
index 0660b9da204f..a463a1b61f23 100644
--- a/drivers/pci/controller/pci-thunder-pem.c
+++ b/drivers/pci/controller/pci-thunder-pem.c
@@ -42,7 +42,7 @@ static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
 	struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;
 
 	if (devfn != 0 || where >= 2048) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/22] PCI: iproc: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
hardware read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pcie-iproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index 30ac5fbefbbf..f182dc2cdb0c 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -660,7 +660,7 @@ static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
 
 	addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
 	if (!addr) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/22] PCI: mediatek: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Compile tested only.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pcie-mediatek.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 2f3f974977a3..aa744ccd1a2a 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -369,13 +369,13 @@ static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	port = mtk_pcie_find_port(bus, devfn);
 	if (!port) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
 	ret = mtk_pcie_hw_rd_cfg(port, bn, devfn, where, size, val);
 	if (ret)
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 
 	return ret;
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/22] PCI: exynos: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Compile tested only.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/dwc/pci-exynos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index c24dab383654..1fdbef184532 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -217,7 +217,7 @@ static int exynos_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
 	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
 
 	if (PCI_SLOT(devfn)) {
-		*val = ~0;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Compile tested only.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pci-aardvark.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 596ebcfcc82d..dc2f820ef55f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	int ret;
 
 	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 			*val = CFG_RD_CRS_VAL;
 			return PCIBIOS_SUCCESSFUL;
 		}
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_SET_FAILED;
 	}
 
@@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 			*val = CFG_RD_CRS_VAL;
 			return PCIBIOS_SUCCESSFUL;
 		}
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_SET_FAILED;
 	}
 
 	/* Check PIO status and get the read result */
 	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
 	if (ret < 0) {
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_SET_FAILED;
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Pali Rohár]

On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:
> An MMIO read from a PCI device that doesn't exist or doesn't respond
> causes a PCI error.  There's no real data to return to satisfy the
> CPU read, so most hardware fabricates ~0 data.
> 
> Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
> read occurs.
> 
> This helps unify PCI error response checking and make error check
> consistent and easier to find.
> 
> Compile tested only.
> 
> Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> ---
>  drivers/pci/controller/pci-aardvark.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 596ebcfcc82d..dc2f820ef55f 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	int ret;
>  
>  	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> -		*val = 0xffffffff;
> +		SET_PCI_ERROR_RESPONSE(val);

Hello! Now I'm looking at this macro, and should not it depends on
"size" argument? If doing 8-bit or 16-bit read operation then should not
it rather sets only low 8 bits or low 16 bits to ones?

>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
>  
> @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  			*val = CFG_RD_CRS_VAL;
>  			return PCIBIOS_SUCCESSFUL;
>  		}
> -		*val = 0xffffffff;
> +		SET_PCI_ERROR_RESPONSE(val);
>  		return PCIBIOS_SET_FAILED;
>  	}
>  
> @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  			*val = CFG_RD_CRS_VAL;
>  			return PCIBIOS_SUCCESSFUL;
>  		}
> -		*val = 0xffffffff;
> +		SET_PCI_ERROR_RESPONSE(val);
>  		return PCIBIOS_SET_FAILED;
>  	}
>  
>  	/* Check PIO status and get the read result */
>  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
>  	if (ret < 0) {
> -		*val = 0xffffffff;
> +		SET_PCI_ERROR_RESPONSE(val);
>  		return PCIBIOS_SET_FAILED;
>  	}
>  
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

On 11/10, Pali Rohár wrote:
> On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:
> > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > causes a PCI error.  There's no real data to return to satisfy the
> > CPU read, so most hardware fabricates ~0 data.
> > 
> > Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
> > read occurs.
> > 
> > This helps unify PCI error response checking and make error check
> > consistent and easier to find.
> > 
> > Compile tested only.
> > 
> > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 596ebcfcc82d..dc2f820ef55f 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  	int ret;
> >  
> >  	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> > -		*val = 0xffffffff;
> > +		SET_PCI_ERROR_RESPONSE(val);
> 
> Hello! Now I'm looking at this macro, and should not it depends on
> "size" argument? If doing 8-bit or 16-bit read operation then should not
> it rather sets only low 8 bits or low 16 bits to ones?
>

Hello o/, Thank you for the review.

Yes! you are right that it should indeed depend on the "size" argument.
And that is what the SET_PCI_ERROR_RESPONSE macro does. The macro is
defined as:

  #define PCI_ERROR_RESPONSE           (~0ULL)
  #define SET_PCI_ERROR_RESPONSE(val)  (*val = ((typeof(*val))PCI_ERROR_RESPONSE))

The macro was part of "Patch 1/22" and is present here [1]. Apologies if
I added the receipient incorrectly.

[1]:
https://lore.kernel.org/linux-pci/d8e423386aad3d78bca575a7521b138508638e3b.1633972263.git.naveennaidu479@gmail.com/T/#m37295a0dcfe0d7e0f67efce3633efd7b891949c4

IIUC, the typeof(*val) helps in setting the value according to the size
of the argument.

Please let me know if my understanding is wrong.

> >  		return PCIBIOS_DEVICE_NOT_FOUND;
> >  	}
> >  
> > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  			*val = CFG_RD_CRS_VAL;
> >  			return PCIBIOS_SUCCESSFUL;
> >  		}
> > -		*val = 0xffffffff;
> > +		SET_PCI_ERROR_RESPONSE(val);
> >  		return PCIBIOS_SET_FAILED;
> >  	}
> >  
> > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  			*val = CFG_RD_CRS_VAL;
> >  			return PCIBIOS_SUCCESSFUL;
> >  		}
> > -		*val = 0xffffffff;
> > +		SET_PCI_ERROR_RESPONSE(val);
> >  		return PCIBIOS_SET_FAILED;
> >  	}
> >  
> >  	/* Check PIO status and get the read result */
> >  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> >  	if (ret < 0) {
> > -		*val = 0xffffffff;
> > +		SET_PCI_ERROR_RESPONSE(val);
> >  		return PCIBIOS_SET_FAILED;
> >  	}
> >  
> > -- 
> > 2.25.1
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Pali Rohár]

On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
> On 11/10, Pali Rohár wrote:
> > On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:
> > > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > > causes a PCI error.  There's no real data to return to satisfy the
> > > CPU read, so most hardware fabricates ~0 data.
> > > 
> > > Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
> > > read occurs.
> > > 
> > > This helps unify PCI error response checking and make error check
> > > consistent and easier to find.
> > > 
> > > Compile tested only.
> > > 
> > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index 596ebcfcc82d..dc2f820ef55f 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > >  	int ret;
> > >  
> > >  	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> > > -		*val = 0xffffffff;
> > > +		SET_PCI_ERROR_RESPONSE(val);
> > 
> > Hello! Now I'm looking at this macro, and should not it depends on
> > "size" argument? If doing 8-bit or 16-bit read operation then should not
> > it rather sets only low 8 bits or low 16 bits to ones?
> >
> 
> Hello o/, Thank you for the review.
> 
> Yes! you are right that it should indeed depend on the "size" argument.
> And that is what the SET_PCI_ERROR_RESPONSE macro does. The macro is
> defined as:
> 
>   #define PCI_ERROR_RESPONSE           (~0ULL)
>   #define SET_PCI_ERROR_RESPONSE(val)  (*val = ((typeof(*val))PCI_ERROR_RESPONSE))
> 
> The macro was part of "Patch 1/22" and is present here [1]. Apologies if
> I added the receipient incorrectly.
> 
> [1]:
> https://lore.kernel.org/linux-pci/d8e423386aad3d78bca575a7521b138508638e3b.1633972263.git.naveennaidu479@gmail.com/T/#m37295a0dcfe0d7e0f67efce3633efd7b891949c4
> 
> IIUC, the typeof(*val) helps in setting the value according to the size
> of the argument.
> 
> Please let me know if my understanding is wrong.

Hello! I mean "size" function argument which is passed as variable.

Function itself is declared as:

static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val);

And in "size" argument is stored number of bytes, kind of read operation
(read byte, read word, read dword). In *val is then stored read value.
For byte operation, just low 8 bits in *val variable are set.

Because *val is u32 it means that typeof(*val) is always 4 independently
of the "size" argument.

For example other project U-Boot has also pci-aardvark.c driver and
U-Boot has for (probably same) purpose pci_get_ff() macro, see:
https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367

I'm not saying if current approach to always sets 0xffffffff
(independently of "size" argument) is correct or not as I do not know
it too! I'm just giving example that this PCI code has very similar
implementation of other project (U-Boot) which sets number of ones based
on the size argument.

So probably something for other people to decide.

Anyway, I very like this your idea to have a macro which purpose is to
explicitly indicate error during config read operation! And to unify all
drivers to use same style for signalling config read error.

> > >  		return PCIBIOS_DEVICE_NOT_FOUND;
> > >  	}
> > >  
> > > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > >  			*val = CFG_RD_CRS_VAL;
> > >  			return PCIBIOS_SUCCESSFUL;
> > >  		}
> > > -		*val = 0xffffffff;
> > > +		SET_PCI_ERROR_RESPONSE(val);
> > >  		return PCIBIOS_SET_FAILED;
> > >  	}
> > >  
> > > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > >  			*val = CFG_RD_CRS_VAL;
> > >  			return PCIBIOS_SUCCESSFUL;
> > >  		}
> > > -		*val = 0xffffffff;
> > > +		SET_PCI_ERROR_RESPONSE(val);
> > >  		return PCIBIOS_SET_FAILED;
> > >  	}
> > >  
> > >  	/* Check PIO status and get the read result */
> > >  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> > >  	if (ret < 0) {
> > > -		*val = 0xffffffff;
> > > +		SET_PCI_ERROR_RESPONSE(val);
> > >  		return PCIBIOS_SET_FAILED;
> > >  	}
> > >  
> > > -- 
> > > 2.25.1
> > > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

On 11/10, Pali Rohár wrote:
> On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
> > On 11/10, Pali Rohár wrote:
> > > On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:
> > > > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > > > causes a PCI error.  There's no real data to return to satisfy the
> > > > CPU read, so most hardware fabricates ~0 data.
> > > > 
> > > > Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
> > > > read occurs.
> > > > 
> > > > This helps unify PCI error response checking and make error check
> > > > consistent and easier to find.
> > > > 
> > > > Compile tested only.
> > > > 
> > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index 596ebcfcc82d..dc2f820ef55f 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > >  	int ret;
> > > >  
> > > >  	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> > > > -		*val = 0xffffffff;
> > > > +		SET_PCI_ERROR_RESPONSE(val);
> > > 
> > > Hello! Now I'm looking at this macro, and should not it depends on
> > > "size" argument? If doing 8-bit or 16-bit read operation then should not
> > > it rather sets only low 8 bits or low 16 bits to ones?
> > >
> > 
> > Hello o/, Thank you for the review.
> > 
> > Yes! you are right that it should indeed depend on the "size" argument.
> > And that is what the SET_PCI_ERROR_RESPONSE macro does. The macro is
> > defined as:
> > 
> >   #define PCI_ERROR_RESPONSE           (~0ULL)
> >   #define SET_PCI_ERROR_RESPONSE(val)  (*val = ((typeof(*val))PCI_ERROR_RESPONSE))
> > 
> > The macro was part of "Patch 1/22" and is present here [1]. Apologies if
> > I added the receipient incorrectly.
> > 
> > [1]:
> > https://lore.kernel.org/linux-pci/d8e423386aad3d78bca575a7521b138508638e3b.1633972263.git.naveennaidu479@gmail.com/T/#m37295a0dcfe0d7e0f67efce3633efd7b891949c4
> > 
> > IIUC, the typeof(*val) helps in setting the value according to the size
> > of the argument.
> > 
> > Please let me know if my understanding is wrong.
> 
> Hello! I mean "size" function argument which is passed as variable.
>

Thank you for explaining! Now I understand what you mean :), Apologies
for not being not understanding this beforehand.

> Function itself is declared as:
> 
> static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val);
> 
> And in "size" argument is stored number of bytes, kind of read operation
> (read byte, read word, read dword). In *val is then stored read value.
> For byte operation, just low 8 bits in *val variable are set.
> 
> Because *val is u32 it means that typeof(*val) is always 4 independently
> of the "size" argument.
> 
> For example other project U-Boot has also pci-aardvark.c driver and
> U-Boot has for (probably same) purpose pci_get_ff() macro, see:
> https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367
> 
> I'm not saying if current approach to always sets 0xffffffff
> (independently of "size" argument) is correct or not as I do not know
> it too! I'm just giving example that this PCI code has very similar
> implementation of other project (U-Boot) which sets number of ones based
> on the size argument.
>

I am not sure too, if we would like to have something like pci_get_ff()
which sets the return mask based on the size.

If we were to have something like pci_get_ff(), I can think of one
problem, some of the functions such as pci_raw_set_power_state() which
checks for errors does not have a "size" argument. An excerpt from that
function is as follows:
  static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
  {
    pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
        if (pmcsr == (u16) ~0) {

For these functions we wont be able to use pci_get_ff(), I mean we could
definitely put the responsibility onto the programmers to write down the
correct size. But that might lead to mistakes, I guess?

Then for those cases, we might need to maintain both the
SET_PCI_ERROR_RESPONSE macro and the pci_get_ff() functions, which then
means that we might not have the same style for signalling config read
error.

I am pretty new to kernel development, so I am sure that whatever I said
above might be totally wrong. If so, please correct me :)

> So probably something for other people to decide.
> 
> Anyway, I very like this your idea to have a macro which purpose is to
> explicitly indicate error during config read operation! And to unify all
> drivers to use same style for signalling config read error.
> 

Thank you :D, I think I'll wait for other people to chime in here with
their opinions and then I'll redo the patch with whatever will be
decided.

Thank again for the detailed reply.

> > > >  		return PCIBIOS_DEVICE_NOT_FOUND;
> > > >  	}
> > > >  
> > > > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > >  			*val = CFG_RD_CRS_VAL;
> > > >  			return PCIBIOS_SUCCESSFUL;
> > > >  		}
> > > > -		*val = 0xffffffff;
> > > > +		SET_PCI_ERROR_RESPONSE(val);
> > > >  		return PCIBIOS_SET_FAILED;
> > > >  	}
> > > >  
> > > > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > >  			*val = CFG_RD_CRS_VAL;
> > > >  			return PCIBIOS_SUCCESSFUL;
> > > >  		}
> > > > -		*val = 0xffffffff;
> > > > +		SET_PCI_ERROR_RESPONSE(val);
> > > >  		return PCIBIOS_SET_FAILED;
> > > >  	}
> > > >  
> > > >  	/* Check PIO status and get the read result */
> > > >  	ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> > > >  	if (ret < 0) {
> > > > -		*val = 0xffffffff;
> > > > +		SET_PCI_ERROR_RESPONSE(val);
> > > >  		return PCIBIOS_SET_FAILED;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.25.1
> > > > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Bjorn Helgaas]

On Tue, Oct 12, 2021 at 09:29:28PM +0530, Naveen Naidu wrote:
> On 11/10, Pali Rohár wrote:
> > On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
> > > On 11/10, Pali Rohár wrote:
> > > > On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:
> > > > > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > > > > causes a PCI error.  There's no real data to return to satisfy the
> > > > > CPU read, so most hardware fabricates ~0 data.
> > > > > 
> > > > > Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
> > > > > read occurs.
> > > > > 
> > > > > This helps unify PCI error response checking and make error check
> > > > > consistent and easier to find.
> > > > > 
> > > > > Compile tested only.
> > > > > 
> > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> > > > > ---
> > > > >  drivers/pci/controller/pci-aardvark.c | 8 ++++----
> > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > index 596ebcfcc82d..dc2f820ef55f 100644
> > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > > >  	int ret;
> > > > >  
> > > > >  	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> > > > > -		*val = 0xffffffff;
> > > > > +		SET_PCI_ERROR_RESPONSE(val);
> > > > 
> > > > Hello! Now I'm looking at this macro, and should not it depends on
> > > > "size" argument? If doing 8-bit or 16-bit read operation then should not
> > > > it rather sets only low 8 bits or low 16 bits to ones?

> > Function itself is declared as:
> > 
> > static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val);
> > 
> > And in "size" argument is stored number of bytes, kind of read operation
> > (read byte, read word, read dword). In *val is then stored read value.
> > For byte operation, just low 8 bits in *val variable are set.
> > 
> > Because *val is u32 it means that typeof(*val) is always 4 independently
> > of the "size" argument.
> > 
> > For example other project U-Boot has also pci-aardvark.c driver and
> > U-Boot has for (probably same) purpose pci_get_ff() macro, see:
> > https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367
> > 
> > I'm not saying if current approach to always sets 0xffffffff
> > (independently of "size" argument) is correct or not as I do not know
> > it too! I'm just giving example that this PCI code has very similar
> > implementation of other project (U-Boot) which sets number of ones based
> > on the size argument.

I don't think there's an issue here.  advk_pcie_rd_conf() can set the
whole u32 to 0xffffffff regardless of the "size" value because
pci_bus_read_config_byte() and pci_bus_read_config_word() extract out
the part they need:

  res = bus->ops->read(bus, devfn, pos, len, &data);              \
  *value = (type)data;                                            \

where "type" is u8 or u16 (this is hard to grep for; it's in the
PCI_OP_READ() macro in drivers/pci/access.c).

> I am not sure too, if we would like to have something like pci_get_ff()
> which sets the return mask based on the size.

I'd like to see how pci_get_ff() works because this is potentially a
widespread, invasive change and it's always better to copy a good
existing design than to make up something new.

> > Anyway, I very like this your idea to have a macro which purpose is to
> > explicitly indicate error during config read operation! And to unify all
> > drivers to use same style for signalling config read error.

I definitely think there's a lot of value in making this consistent
*somehow*, so thanks for working on this!

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found

[Pali Rohár]

On Tuesday 12 October 2021 21:13:10 Bjorn Helgaas wrote:
> On Tue, Oct 12, 2021 at 09:29:28PM +0530, Naveen Naidu wrote:
> > On 11/10, Pali Rohár wrote:
> > > On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
> > > > On 11/10, Pali Rohár wrote:
> > > > > On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:
> > > > > > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > > > > > causes a PCI error.  There's no real data to return to satisfy the
> > > > > > CPU read, so most hardware fabricates ~0 data.
> > > > > > 
> > > > > > Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
> > > > > > read occurs.
> > > > > > 
> > > > > > This helps unify PCI error response checking and make error check
> > > > > > consistent and easier to find.
> > > > > > 
> > > > > > Compile tested only.
> > > > > > 
> > > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> > > > > > ---
> > > > > >  drivers/pci/controller/pci-aardvark.c | 8 ++++----
> > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > > index 596ebcfcc82d..dc2f820ef55f 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > > > > >  	int ret;
> > > > > >  
> > > > > >  	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> > > > > > -		*val = 0xffffffff;
> > > > > > +		SET_PCI_ERROR_RESPONSE(val);
> > > > > 
> > > > > Hello! Now I'm looking at this macro, and should not it depends on
> > > > > "size" argument? If doing 8-bit or 16-bit read operation then should not
> > > > > it rather sets only low 8 bits or low 16 bits to ones?
> 
> > > Function itself is declared as:
> > > 
> > > static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val);
> > > 
> > > And in "size" argument is stored number of bytes, kind of read operation
> > > (read byte, read word, read dword). In *val is then stored read value.
> > > For byte operation, just low 8 bits in *val variable are set.
> > > 
> > > Because *val is u32 it means that typeof(*val) is always 4 independently
> > > of the "size" argument.
> > > 
> > > For example other project U-Boot has also pci-aardvark.c driver and
> > > U-Boot has for (probably same) purpose pci_get_ff() macro, see:
> > > https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367
> > > 
> > > I'm not saying if current approach to always sets 0xffffffff
> > > (independently of "size" argument) is correct or not as I do not know
> > > it too! I'm just giving example that this PCI code has very similar
> > > implementation of other project (U-Boot) which sets number of ones based
> > > on the size argument.
> 
> I don't think there's an issue here.  advk_pcie_rd_conf() can set the
> whole u32 to 0xffffffff regardless of the "size" value because
> pci_bus_read_config_byte() and pci_bus_read_config_word() extract out
> the part they need:
> 
>   res = bus->ops->read(bus, devfn, pos, len, &data);              \
>   *value = (type)data;                                            \
> 
> where "type" is u8 or u16 (this is hard to grep for; it's in the
> PCI_OP_READ() macro in drivers/pci/access.c).

Ok! No problem if this is something which is not going to be changed.

> > I am not sure too, if we would like to have something like pci_get_ff()
> > which sets the return mask based on the size.
> 
> I'd like to see how pci_get_ff() works because this is potentially a
> widespread, invasive change and it's always better to copy a good
> existing design than to make up something new.

Here is U-Boot implementation of that function, it is pretty simple:
https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-uclass.c#L103-113

> > > Anyway, I very like this your idea to have a macro which purpose is to
> > > explicitly indicate error during config read operation! And to unify all
> > > drivers to use same style for signalling config read error.
> 
> I definitely think there's a lot of value in making this consistent
> *somehow*, so thanks for working on this!

+1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/22] PCI: mvebu: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pci-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index ed13e81cd691..51d61194a31a 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -654,7 +654,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 
 	port = mvebu_pcie_find_port(pcie, bus, devfn);
 	if (!port) {
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -664,7 +664,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 						 size, val);
 
 	if (!mvebu_pcie_link_up(port)) {
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 13/22] PCI: rockchip: Use SET_PCI_ERROR_RESPONSE() when device not found

[Naveen Naidu]

An MMIO read from a PCI device that doesn't exist or doesn't respond
causes a PCI error.  There's no real data to return to satisfy the
CPU read, so most hardware fabricates ~0 data.

Use SET_PCI_ERROR_RESPONSE() to set the error response, when a faulty
read occurs.

This helps unify PCI error response checking and make error check
consistent and easier to find.

Compile tested only.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index c52316d0bfd2..f5d718700d59 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -222,7 +222,7 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 	struct rockchip_pcie *rockchip = bus->sysdata;
 
 	if (!rockchip_pcie_valid_device(rockchip, bus, PCI_SLOT(devfn))) {
-		*val = 0xffffffff;
+		SET_PCI_ERROR_RESPONSE(val);
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 22/22] PCI: xgene: Use PCI_ERROR_RESPONSE to specify hardware error

[Naveen Naidu]

Include PCI_ERROR_RESPONSE along with 0xffffffff in the comment to
specify a hardware error. This makes finding where MMIO read error
occurs easier.

Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
---
 drivers/pci/controller/pci-xgene.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index e64536047b65..4b10794e1ba1 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -176,10 +176,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 	 * Retry Status (CRS) logic: when CRS Software Visibility is
 	 * enabled and we read the Vendor and Device ID of a non-existent
 	 * device, the controller fabricates return data of 0xFFFF0001
-	 * ("device exists but is not ready") instead of 0xFFFFFFFF
-	 * ("device does not exist").  This causes the PCI core to retry
-	 * the read until it times out.  Avoid this by not claiming to
-	 * support CRS SV.
+	 * ("device exists but is not ready") instead of
+	 * 0xFFFFFFFF (PCI_ERROR_RESPONSE) ("device does not exist"). This causes
+	 * the PCI core to retry the read until it times out.
+	 * Avoid this by not claiming to support CRS SV.
 	 */
 	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
 	    ((where & ~0x3) == XGENE_V1_PCI_EXP_CAP + PCI_EXP_RTCTL))
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel