* [PATCH 01/22] PCI: Add PCI_ERROR_RESPONSE and it's related defintions
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
@ 2021-10-11 17:37 ` Naveen Naidu
2021-10-11 17:45 ` [PATCH 03/22] PCI: thunder: Use SET_PCI_ERROR_RESPONSE() when device not found Naveen Naidu
` (7 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 17:37 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
linux-arm-kernel, bcm-kernel-feedback-list, linux-mediatek,
linux-samsung-soc, linux-renesas-soc, linux-rockchip,
linuxppc-dev
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 03/22] PCI: thunder: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
2021-10-11 17:37 ` [PATCH 01/22] PCI: Add PCI_ERROR_RESPONSE and it's related defintions Naveen Naidu
@ 2021-10-11 17:45 ` Naveen Naidu
2021-10-11 17:46 ` [PATCH 04/22] PCI: iproc: " Naveen Naidu
` (6 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 17:45 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
Robert Richter, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 04/22] PCI: iproc: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
2021-10-11 17:37 ` [PATCH 01/22] PCI: Add PCI_ERROR_RESPONSE and it's related defintions Naveen Naidu
2021-10-11 17:45 ` [PATCH 03/22] PCI: thunder: Use SET_PCI_ERROR_RESPONSE() when device not found Naveen Naidu
@ 2021-10-11 17:46 ` Naveen Naidu
2021-10-11 17:51 ` [PATCH 05/22] PCI: mediatek: " Naveen Naidu
` (5 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 17:46 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
Ray Jui, Scott Branden,
maintainer:BROADCOM IPROC ARM ARCHITECTURE,
moderated list:BROADCOM IPROC ARM ARCHITECTURE
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 05/22] PCI: mediatek: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
` (2 preceding siblings ...)
2021-10-11 17:46 ` [PATCH 04/22] PCI: iproc: " Naveen Naidu
@ 2021-10-11 17:51 ` Naveen Naidu
2021-10-11 17:52 ` [PATCH 06/22] PCI: exynos: " Naveen Naidu
` (4 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 17:51 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
p.zabel, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Matthias Brugger,
open list:PCIE DRIVER FOR MEDIATEK,
moderated list:ARM/Mediatek SoC support
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 06/22] PCI: exynos: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
` (3 preceding siblings ...)
2021-10-11 17:51 ` [PATCH 05/22] PCI: mediatek: " Naveen Naidu
@ 2021-10-11 17:52 ` Naveen Naidu
2021-10-11 17:56 ` [PATCH 09/22] PCI: aardvark: " Naveen Naidu
` (3 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 17:52 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
Jingoo Han, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Krzysztof Kozlowski,
moderated list:PCI DRIVER FOR SAMSUNG EXYNOS,
open list:PCI DRIVER FOR SAMSUNG EXYNOS
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
` (4 preceding siblings ...)
2021-10-11 17:52 ` [PATCH 06/22] PCI: exynos: " Naveen Naidu
@ 2021-10-11 17:56 ` Naveen Naidu
2021-10-11 18:08 ` Pali Rohár
2021-10-11 18:00 ` [PATCH 10/22] PCI: mvebu: " Naveen Naidu
` (2 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 17:56 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
Thomas Petazzoni, Pali Rohár, Lorenzo Pieralisi,
Rob Herring, Krzysztof Wilczyński,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:56 ` [PATCH 09/22] PCI: aardvark: " Naveen Naidu
@ 2021-10-11 18:08 ` Pali Rohár
2021-10-11 18:28 ` Naveen Naidu
[not found] ` <20211011182526.kboaxqofdpd2jjrl@theprophet>
0 siblings, 2 replies; 16+ messages in thread
From: Pali Rohár @ 2021-10-11 18:08 UTC (permalink / raw)
To: Naveen Naidu
Cc: bhelgaas, linux-kernel-mentees, linux-pci, linux-kernel,
Thomas Petazzoni, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 18:08 ` Pali Rohár
@ 2021-10-11 18:28 ` Naveen Naidu
[not found] ` <20211011182526.kboaxqofdpd2jjrl@theprophet>
1 sibling, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 18:28 UTC (permalink / raw)
To: Pali Rohár
Cc: bhelgaas, linux-kernel-mentees, linux-pci, linux-kernel,
Thomas Petazzoni, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20211011182526.kboaxqofdpd2jjrl@theprophet>]
* Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
[not found] ` <20211011182526.kboaxqofdpd2jjrl@theprophet>
@ 2021-10-11 18:41 ` Pali Rohár
2021-10-12 15:59 ` Naveen Naidu
0 siblings, 1 reply; 16+ messages in thread
From: Pali Rohár @ 2021-10-11 18:41 UTC (permalink / raw)
To: Naveen Naidu
Cc: bhelgaas, linux-kernel-mentees, linux-pci, linux-kernel,
Thomas Petazzoni, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 18:41 ` Pali Rohár
@ 2021-10-12 15:59 ` Naveen Naidu
2021-10-13 2:13 ` Bjorn Helgaas
0 siblings, 1 reply; 16+ messages in thread
From: Naveen Naidu @ 2021-10-12 15:59 UTC (permalink / raw)
To: Pali Rohár
Cc: bhelgaas, linux-kernel-mentees, linux-pci, linux-kernel,
Thomas Petazzoni, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-12 15:59 ` Naveen Naidu
@ 2021-10-13 2:13 ` Bjorn Helgaas
2021-10-13 17:59 ` Pali Rohár
0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2021-10-13 2:13 UTC (permalink / raw)
To: Naveen Naidu
Cc: Pali Rohár, Rob Herring, Lorenzo Pieralisi,
Krzysztof Wilczyński, linux-pci, linux-kernel,
Thomas Petazzoni, bhelgaas, linux-kernel-mentees,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-13 2:13 ` Bjorn Helgaas
@ 2021-10-13 17:59 ` Pali Rohár
0 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2021-10-13 17:59 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Naveen Naidu, Rob Herring, Lorenzo Pieralisi,
Krzysztof Wilczyński, linux-pci, linux-kernel,
Thomas Petazzoni, bhelgaas, linux-kernel-mentees,
moderated list:PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 10/22] PCI: mvebu: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
` (5 preceding siblings ...)
2021-10-11 17:56 ` [PATCH 09/22] PCI: aardvark: " Naveen Naidu
@ 2021-10-11 18:00 ` Naveen Naidu
2021-10-11 18:02 ` [PATCH 13/22] PCI: rockchip: " Naveen Naidu
2021-10-11 18:13 ` [PATCH 22/22] PCI: xgene: Use PCI_ERROR_RESPONSE to specify hardware error Naveen Naidu
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 18:00 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
thomas.petazzoni, lorenzo.pieralisi, robh, kw, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 13/22] PCI: rockchip: Use SET_PCI_ERROR_RESPONSE() when device not found
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
` (6 preceding siblings ...)
2021-10-11 18:00 ` [PATCH 10/22] PCI: mvebu: " Naveen Naidu
@ 2021-10-11 18:02 ` Naveen Naidu
2021-10-11 18:13 ` [PATCH 22/22] PCI: xgene: Use PCI_ERROR_RESPONSE to specify hardware error Naveen Naidu
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 18:02 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
Shawn Lin, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Heiko Stuebner,
open list:PCIE DRIVER FOR ROCKCHIP,
moderated list:ARM/Rockchip SoC support
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 22/22] PCI: xgene: Use PCI_ERROR_RESPONSE to specify hardware error
2021-10-11 17:35 [PATCH 00/22] PCI: Unify PCI error response checking Naveen Naidu
` (7 preceding siblings ...)
2021-10-11 18:02 ` [PATCH 13/22] PCI: rockchip: " Naveen Naidu
@ 2021-10-11 18:13 ` Naveen Naidu
8 siblings, 0 replies; 16+ messages in thread
From: Naveen Naidu @ 2021-10-11 18:13 UTC (permalink / raw)
To: bhelgaas
Cc: Naveen Naidu, linux-kernel-mentees, linux-pci, linux-kernel,
Toan Le, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński,
moderated list:PCI DRIVER FOR APPLIEDMICRO XGENE
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
^ permalink raw reply related [flat|nested] 16+ messages in thread