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
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 and
RESPONSE_IS_PCI_ERROR() to check the error response during hardware
read.
These definitions make error checks consistent and easier to find.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/access.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 46935695cfb9..e1954bbbd137 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
addr = bus->ops->map_bus(bus, devfn, where);
if (!addr) {
- *val = ~0;
+ SET_PCI_ERROR_RESPONSE(val);
return PCIBIOS_DEVICE_NOT_FOUND;
}
@@ -123,7 +123,7 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
if (!addr) {
- *val = ~0;
+ SET_PCI_ERROR_RESPONSE(val);
return PCIBIOS_DEVICE_NOT_FOUND;
}
@@ -411,10 +411,10 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val);
/*
* Reset *val to 0 if pci_read_config_word() fails, it may
- * have been written as 0xFFFF if hardware error happens
- * during pci_read_config_word().
+ * have been written as 0xFFFF (PCI_ERROR_RESPONSE) if hardware error
+ * happens during pci_read_config_word().
*/
- if (ret)
+ if (RESPONSE_IS_PCI_ERROR(val))
*val = 0;
return ret;
}
@@ -446,10 +446,10 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
/*
* Reset *val to 0 if pci_read_config_dword() fails, it may
- * have been written as 0xFFFFFFFF if hardware error happens
- * during pci_read_config_dword().
+ * have been written as 0xFFFFFFFF (PCI_ERROR_RESPONSE) if hardware
+ * error happens during pci_read_config_dword().
*/
- if (ret)
+ if (RESPONSE_IS_PCI_ERROR(val))
*val = 0;
return ret;
}
@@ -523,7 +523,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
{
if (pci_dev_is_disconnected(dev)) {
- *val = ~0;
+ SET_PCI_ERROR_RESPONSE(val);
return PCIBIOS_DEVICE_NOT_FOUND;
}
return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
@@ -533,7 +533,7 @@ EXPORT_SYMBOL(pci_read_config_byte);
int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
{
if (pci_dev_is_disconnected(dev)) {
- *val = ~0;
+ SET_PCI_ERROR_RESPONSE(val);
return PCIBIOS_DEVICE_NOT_FOUND;
}
return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
@@ -544,7 +544,7 @@ int pci_read_config_dword(const struct pci_dev *dev, int where,
u32 *val)
{
if (pci_dev_is_disconnected(dev)) {
- *val = ~0;
+ SET_PCI_ERROR_RESPONSE(val);
return PCIBIOS_DEVICE_NOT_FOUND;
}
return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
--
2.25.1
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
drivers/pci/controller/dwc/pcie-histb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 86f9d16c50d7..1392732b104f 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -128,7 +128,7 @@ static int histb_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
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 <[email protected]>
---
drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 026fd1e42a55..a37b59a9e0be 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -331,7 +331,7 @@ static int kirin_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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
drivers/pci/controller/pcie-altera.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index 2513e9363236..d6c71b5d1ffb 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -511,7 +511,7 @@ static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
return PCIBIOS_BAD_REGISTER_NUMBER;
if (!altera_pcie_valid_device(pcie, bus, PCI_SLOT(devfn))) {
- *value = 0xffffffff;
+ SET_PCI_ERROR_RESPONSE(value);
return PCIBIOS_DEVICE_NOT_FOUND;
}
--
2.25.1
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 <[email protected]>
---
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
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 <[email protected]>
---
drivers/pci/controller/pcie-rcar-host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index 8f3131844e77..80dbc6f717d6 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -162,7 +162,7 @@ static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
ret = rcar_pcie_config_access(host, RCAR_PCI_ACCESS_READ,
bus, devfn, where, val);
if (ret != PCIBIOS_SUCCESSFUL) {
- *val = 0xffffffff;
+ SET_PCI_ERROR_RESPONSE(val);
return ret;
}
--
2.25.1
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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
data from hardware.
This unifies PCI error response checking and make error checks
consistent and easier to find.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/pci.c | 10 +++++-----
drivers/pci/probe.c | 10 +++++-----
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce2ab62b64cf..c1575364d1ce 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1077,7 +1077,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
return -EIO;
pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
- if (pmcsr == (u16) ~0) {
+ if (RESPONSE_IS_PCI_ERROR(&pmcsr)) {
pci_err(dev, "can't change power state from %s to %s (config space inaccessible)\n",
pci_power_name(dev->current_state),
pci_power_name(state));
@@ -1239,16 +1239,16 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
* After reset, the device should not silently discard config
* requests, but it may still indicate that it needs more time by
* responding to them with CRS completions. The Root Port will
- * generally synthesize ~0 data to complete the read (except when
- * CRS SV is enabled and the read was for the Vendor ID; in that
- * case it synthesizes 0x0001 data).
+ * generally synthesize ~0 (PCI_ERROR_RESPONSE) data to complete
+ * the read (except when CRS SV is enabled and the read was for the
+ * Vendor ID; in that case it synthesizes 0x0001 data).
*
* Wait for the device to return a non-CRS completion. Read the
* Command register instead of Vendor ID so we don't have to
* contend with the CRS SV value.
*/
pci_read_config_dword(dev, PCI_COMMAND, &id);
- while (id == ~0) {
+ while (RESPONSE_IS_PCI_ERROR(&id)) {
if (delay > timeout) {
pci_warn(dev, "not ready %dms after %s; giving up\n",
delay - 1, reset_type);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d9fc02a71baa..55b94d689eca 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -206,14 +206,14 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
* memory BAR or a ROM, bit 0 must be clear; if it's an io BAR, bit
* 1 must be clear.
*/
- if (sz == 0xffffffff)
+ if (RESPONSE_IS_PCI_ERROR(&sz))
sz = 0;
/*
* I don't know how l can have all bits set. Copied from old code.
* Maybe it fixes a bug on some ancient platform.
*/
- if (l == 0xffffffff)
+ if (RESPONSE_IS_PCI_ERROR(&l))
l = 0;
if (type == pci_bar_unknown) {
@@ -1660,7 +1660,7 @@ static int pci_cfg_space_size_ext(struct pci_dev *dev)
if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
return PCI_CFG_SPACE_SIZE;
- if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
+ if (RESPONSE_IS_PCI_ERROR(&status) || pci_ext_cfg_is_aliased(dev))
return PCI_CFG_SPACE_SIZE;
return PCI_CFG_SPACE_EXP_SIZE;
@@ -2336,8 +2336,8 @@ bool pci_bus_generic_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
return false;
- /* Some broken boards return 0 or ~0 if a slot is empty: */
- if (*l == 0xffffffff || *l == 0x00000000 ||
+ /* Some broken boards return 0 or ~0 (PCI_ERROR_RESPONSE) if a slot is empty: */
+ if (RESPONSE_IS_PCI_ERROR(l) || *l == 0x00000000 ||
*l == 0x0000ffff || *l == 0xffff0000)
return false;
--
2.25.1
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 <[email protected]>
> ---
> 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
>
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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
data from hardware.
This helps unify PCI error response checking and make error checks
consistent and easier to find.
Compile tested only.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/hotplug/pciehp_hpc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 3024d7e85e6a..8a2f6bb643b5 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -89,7 +89,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
do {
pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
- if (slot_status == (u16) ~0) {
+ if (RESPONSE_IS_PCI_ERROR(&slot_status)) {
ctrl_info(ctrl, "%s: no response from device\n",
__func__);
return 0;
@@ -165,7 +165,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
pcie_wait_cmd(ctrl);
pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
- if (slot_ctrl == (u16) ~0) {
+ if (RESPONSE_IS_PCI_ERROR(&slot_ctrl)) {
ctrl_info(ctrl, "%s: no response from device\n", __func__);
goto out;
}
@@ -236,7 +236,7 @@ int pciehp_check_link_active(struct controller *ctrl)
int ret;
ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
- if (ret == PCIBIOS_DEVICE_NOT_FOUND || lnk_status == (u16)~0)
+ if (ret == PCIBIOS_DEVICE_NOT_FOUND || RESPONSE_IS_PCI_ERROR(&lnk_status))
return -ENODEV;
ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
@@ -443,7 +443,7 @@ int pciehp_card_present(struct controller *ctrl)
int ret;
ret = pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
- if (ret == PCIBIOS_DEVICE_NOT_FOUND || slot_status == (u16)~0)
+ if (ret == PCIBIOS_DEVICE_NOT_FOUND || RESPONSE_IS_PCI_ERROR(&slot_status))
return -ENODEV;
return !!(slot_status & PCI_EXP_SLTSTA_PDS);
@@ -621,7 +621,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
read_status:
pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
- if (status == (u16) ~0) {
+ if (RESPONSE_IS_PCI_ERROR(&status)) {
ctrl_info(ctrl, "%s: no response from device\n", __func__);
if (parent)
pm_runtime_put(parent);
--
2.25.1
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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
data from hardware.
This helps unify PCI error response checking and make error checks
consistent and easier to find.
Compile tested only.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/pcie/dpc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index c556e7beafe3..561c44d9429c 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -79,7 +79,7 @@ static bool dpc_completed(struct pci_dev *pdev)
u16 status;
pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, &status);
- if ((status != 0xffff) && (status & PCI_EXP_DPC_STATUS_TRIGGER))
+ if ((!RESPONSE_IS_PCI_ERROR(&status)) && (status & PCI_EXP_DPC_STATUS_TRIGGER))
return false;
if (test_bit(PCI_DPC_RECOVERING, &pdev->priv_flags))
@@ -312,7 +312,7 @@ static irqreturn_t dpc_irq(int irq, void *context)
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
- if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
+ if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || RESPONSE_IS_PCI_ERROR(&status))
return IRQ_NONE;
pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
--
2.25.1
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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
data from hardware.
This helps unify PCI error response checking and make error checks
consistent and easier to find.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/controller/vmd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index a5987e52700e..db81bc4cfe8c 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -538,7 +538,7 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
int ret;
ret = pci_read_config_dword(dev, PCI_REG_VMLOCK, &vmlock);
- if (ret || vmlock == ~0)
+ if (ret || RESPONSE_IS_PCI_ERROR(&vmlock))
return -ENODEV;
if (MB2_SHADOW_EN(vmlock)) {
--
2.25.1
Include PCI_ERROR_RESPONSE along with 0xffffffff in the comment to
specify a hardware error. This helps finding where MMIO read error
occurs easier to find.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/controller/dwc/pci-keystone.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 865258d8c53c..25b11610b500 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -748,8 +748,8 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
#ifdef CONFIG_ARM
/*
* When a PCI device does not exist during config cycles, keystone host gets a
- * bus error instead of returning 0xffffffff. This handler always returns 0
- * for this kind of faults.
+ * bus error instead of returning 0xffffffff (PCI_ERROR_RESPONSE).
+ * This handler always returns 0 for this kind of faults.
*/
static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
struct pt_regs *regs)
--
2.25.1
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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
data from hardware.
This helps unify PCI error response checking and make error checks
consistent and easier to find.
Compile tested only.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/pcie/pme.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 1d0dd77fed3a..24588d0b581f 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -224,7 +224,7 @@ static void pcie_pme_work_fn(struct work_struct *work)
break;
pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
- if (rtsta == (u32) ~0)
+ if (RESPONSE_IS_PCI_ERROR(&rtsta))
break;
if (rtsta & PCI_EXP_RTSTA_PME) {
@@ -274,7 +274,7 @@ static irqreturn_t pcie_pme_irq(int irq, void *context)
spin_lock_irqsave(&data->lock, flags);
pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
- if (rtsta == (u32) ~0 || !(rtsta & PCI_EXP_RTSTA_PME)) {
+ if (RESPONSE_IS_PCI_ERROR(&rtsta) || !(rtsta & PCI_EXP_RTSTA_PME)) {
spin_unlock_irqrestore(&data->lock, flags);
return IRQ_NONE;
}
--
2.25.1
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 <[email protected]>
---
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
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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
data from hardware.
This helps unify PCI error response checking and make error checks
consistent and easier to find.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/hotplug/cpqphp_ctrl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c b/drivers/pci/hotplug/cpqphp_ctrl.c
index 1b26ca0b3701..d5274b9b06a5 100644
--- a/drivers/pci/hotplug/cpqphp_ctrl.c
+++ b/drivers/pci/hotplug/cpqphp_ctrl.c
@@ -2273,7 +2273,7 @@ static u32 configure_new_device(struct controller *ctrl, struct pci_func *func
while ((function < max_functions) && (!stop_it)) {
pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(func->device, function), 0x00, &ID);
- if (ID == 0xFFFFFFFF) {
+ if (RESPONSE_IS_PCI_ERROR(&ID)) {
function++;
} else {
/* Setup slot structure. */
@@ -2517,7 +2517,7 @@ static int configure_new_function(struct controller *ctrl, struct pci_func *func
pci_bus_read_config_dword(pci_bus, PCI_DEVFN(device, 0), 0x00, &ID);
pci_bus->number = func->bus;
- if (ID != 0xFFFFFFFF) { /* device present */
+ if (!RESPONSE_IS_PCI_ERROR(&ID)) { /* device present */
/* Setup slot structure. */
new_slot = cpqhp_slot_create(hold_bus_node->base);
--
2.25.1
Include PCI_ERROR_RESPONSE along with 0xFFFFFFFF in the comment to
specify a hardware error. This helps finding where MMIO read error
occurs easier to find.
Signed-off-by: Naveen Naidu <[email protected]>
---
drivers/pci/controller/pci-hyperv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 67c46e52c0dc..7e1102e3d7c6 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1774,7 +1774,7 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
* If the memory enable bit is already set, Hyper-V silently ignores
* the below BAR updates, and the related PCI device driver can not
* work, because reading from the device register(s) always returns
- * 0xFFFFFFFF.
+ * 0xFFFFFFFF (PCI_ERROR_RESPONSE).
*/
list_for_each_entry(hpdev, &hbus->children, list_entry) {
_hv_pcifront_read_config(hpdev, PCI_COMMAND, 2, &command);
--
2.25.1
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 <[email protected]>
> > ---
> > 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
> >
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 <[email protected]>
> > > ---
> > > 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
> > >
On Mon, Oct 11, 2021 at 11:37:33PM +0530, 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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
> data from hardware.
Actually what happens is that PCI read transactions *time out*,
so the host controller fabricates a response.
By contrast, a PCI *error* usually denotes an Uncorrectable or
Correctable Error as specified in section 6.2.2 of the PCIe Base Spec.
Thus something like RESPONSE_IS_PCI_TIMEOUT() or IS_PCI_TIMEOUT() would
probably be more appropriate. I'll leave the exact bikeshed color for
others to decide. :-)
> Signed-off-by: Naveen Naidu <[email protected]>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Acked-by: Lukas Wunner <[email protected]>
On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> read.
>
> These definitions make error checks consistent and easier to find.
>
> Signed-off-by: Naveen Naidu <[email protected]>
> ---
> drivers/pci/access.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 46935695cfb9..e1954bbbd137 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>
> addr = bus->ops->map_bus(bus, devfn, where);
> if (!addr) {
> - *val = ~0;
> + SET_PCI_ERROR_RESPONSE(val);
This to me doesn't look like kernel style. I'd rather see a define
replace just '~0', but I defer to Bjorn.
> return PCIBIOS_DEVICE_NOT_FOUND;
Neither does this using custom error codes rather than standard Linux
errno. I point this out as I that's were I'd start with the config
accessors. Though there are lots of occurrences so we'd need a way to do
this in manageable steps.
Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
and delete the drivers all doing this? Then we have 2 copies (in source)
rather than the many this series modifies. Though I'm not sure if there
are other cases of calling pci_bus.ops.read() which expect to get ~0.
Rob
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 <[email protected]>
> > > > ---
> > > > 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
> > > >
On 11/10, Lukas Wunner wrote:
> On Mon, Oct 11, 2021 at 11:37:33PM +0530, 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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
> > data from hardware.
>
> Actually what happens is that PCI read transactions *time out*,
> so the host controller fabricates a response.
>
Ah! yes. Now that I look at it, RESPONSE_IS_PCI_TIMEOUT() does indeed
seem like a better option to RESPONSE_IS_PCI_ERROR(), since it's more
specfic and depicts the actual condition.
I'll wait for sometime and see if others have any objection/a better
name for the macro and then redo the patch with that.
Thank you very much for the review ^^
> By contrast, a PCI *error* usually denotes an Uncorrectable or
> Correctable Error as specified in section 6.2.2 of the PCIe Base Spec.
>
> Thus something like RESPONSE_IS_PCI_TIMEOUT() or IS_PCI_TIMEOUT() would
> probably be more appropriate. I'll leave the exact bikeshed color for
> others to decide. :-)
>
>
> > Signed-off-by: Naveen Naidu <[email protected]>
> > ---
> > drivers/pci/hotplug/pciehp_hpc.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
>
> Acked-by: Lukas Wunner <[email protected]>
On 11/10, Rob Herring wrote:
> On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > read.
> >
> > These definitions make error checks consistent and easier to find.
> >
> > Signed-off-by: Naveen Naidu <[email protected]>
> > ---
> > drivers/pci/access.c | 22 +++++++++++-----------
> > 1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 46935695cfb9..e1954bbbd137 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> >
> > addr = bus->ops->map_bus(bus, devfn, where);
> > if (!addr) {
> > - *val = ~0;
> > + SET_PCI_ERROR_RESPONSE(val);
>
> This to me doesn't look like kernel style. I'd rather see a define
> replace just '~0', but I defer to Bjorn.
>
Apologies, if this is a lame question. Why is the macro
SET_PCI_ERROR_RESPONSE not a kernel style. I ask this so that I do not
end up making the same mistake again.
Bjorn, did initally make a patch with only replacing '~0' but then
Andrew suggested in the patch [1] that we should use the macro.
[1]:
https://lore.kernel.org/linux-pci/[email protected]/
[Adding Andrew in the CC for this]
Apologies, I should have added this link in the cover letter but I
completely forgot about it.
That's why I decided to go with the macro. If that is not the right
approach please let me know and I can fix it up.
> > return PCIBIOS_DEVICE_NOT_FOUND;
>
> Neither does this using custom error codes rather than standard Linux
> errno. I point this out as I that's were I'd start with the config
> accessors. Though there are lots of occurrences so we'd need a way to do
> this in manageable steps.
>
I am sorry, but I do not have any answer for this. I really do not know
why we return custom error codes instead of standard Linux errno. Maybe
someone else can pitch in on this.
> Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
> and delete the drivers all doing this? Then we have 2 copies (in source)
> rather than the many this series modifies. Though I'm not sure if there
> are other cases of calling pci_bus.ops.read() which expect to get ~0.
>
This seems like a really good idea :) But again, I am not entirely sure
if doing so would give us any unexpected behaviour. I'll wait for some
one to reply to this and if people agree to it, I would be glad to make
the changes to PCI_OP_READ and PCI_USER_READ_CONFIG and send a new
patch.
Thank you very much for the review :-)
> Rob
On Tue, Oct 12, 2021 at 11:21 AM Naveen Naidu <[email protected]> wrote:
>
> On 11/10, Rob Herring wrote:
> > On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > > read.
> > >
> > > These definitions make error checks consistent and easier to find.
> > >
> > > Signed-off-by: Naveen Naidu <[email protected]>
> > > ---
> > > drivers/pci/access.c | 22 +++++++++++-----------
> > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index 46935695cfb9..e1954bbbd137 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> > >
> > > addr = bus->ops->map_bus(bus, devfn, where);
> > > if (!addr) {
> > > - *val = ~0;
> > > + SET_PCI_ERROR_RESPONSE(val);
> >
> > This to me doesn't look like kernel style. I'd rather see a define
> > replace just '~0', but I defer to Bjorn.
> >
>
> Apologies, if this is a lame question. Why is the macro
> SET_PCI_ERROR_RESPONSE not a kernel style. I ask this so that I do not
> end up making the same mistake again.
Generally, we don't do macros if a static inline function will work
because you get more type checking with a function. There's exceptions
like struct initializers which need to work in declarations.
Second, I think the above obfuscates the code. I know exactly what the
original line is doing to val. With SET_PCI_ERROR_RESPONSE(), I have
to go look and it hasn't saved us any LOC to make the caller more
readable. The downside of the original way, is I don't know why we set
val to ~0, but just a define would tell me that.
> Bjorn, did initally make a patch with only replacing '~0' but then
> Andrew suggested in the patch [1] that we should use the macro.
>
> [1]:
> https://lore.kernel.org/linux-pci/[email protected]/
> [Adding Andrew in the CC for this]
He's no longer at Arm nor active upstream.
> Apologies, I should have added this link in the cover letter but I
> completely forgot about it.
>
> That's why I decided to go with the macro. If that is not the right
> approach please let me know and I can fix it up.
>
> > > return PCIBIOS_DEVICE_NOT_FOUND;
> >
> > Neither does this using custom error codes rather than standard Linux
> > errno. I point this out as I that's were I'd start with the config
> > accessors. Though there are lots of occurrences so we'd need a way to do
> > this in manageable steps.
> >
>
> I am sorry, but I do not have any answer for this. I really do not know
> why we return custom error codes instead of standard Linux errno. Maybe
> someone else can pitch in on this.
I don't either. My guess is either just too many places to fix or
somehow it trickles up to userspace (but probably not since the error
codes aren't in a uapi header).
> > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
> > and delete the drivers all doing this? Then we have 2 copies (in source)
> > rather than the many this series modifies. Though I'm not sure if there
> > are other cases of calling pci_bus.ops.read() which expect to get ~0.
> >
>
> This seems like a really good idea :) But again, I am not entirely sure
> if doing so would give us any unexpected behaviour. I'll wait for some
> one to reply to this and if people agree to it, I would be glad to make
> the changes to PCI_OP_READ and PCI_USER_READ_CONFIG and send a new
> patch.
I'm expecting Bjorn to chime in.
Rob
On Tuesday 12 October 2021 21:51:08 Naveen Naidu wrote:
> On 11/10, Rob Herring wrote:
> > On Mon, Oct 11, 2021 at 11:08:32PM +0530, Naveen Naidu wrote:
> > > return PCIBIOS_DEVICE_NOT_FOUND;
> >
> > Neither does this using custom error codes rather than standard Linux
> > errno. I point this out as I that's were I'd start with the config
> > accessors. Though there are lots of occurrences so we'd need a way to do
> > this in manageable steps.
> >
>
> I am sorry, but I do not have any answer for this. I really do not know
> why we return custom error codes instead of standard Linux errno. Maybe
> someone else can pitch in on this.
More people are asking same question and Bjorn recently wrote answer:
https://lore.kernel.org/linux-pci/20211011205851.GA1690395@bhelgaas/
It looks like some relict from past when PCI was implemented only for x86.
Anyway, I was looking at PCI firmware spec (where are these PCBIOS_*
codes defined) and config read function can return only
PCIBIOS_SUCCESSFUL value.
So if kernel PCI API is following this PCBIOS API it means that
controller drivers are implementing it improperly if they returns also
non-success value for read method in some cases. And for me it looks
like very "stupid" API for read as it has basically same meaning as
function with void return value.
> > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
> > and delete the drivers all doing this? Then we have 2 copies (in source)
> > rather than the many this series modifies. Though I'm not sure if there
> > are other cases of calling pci_bus.ops.read() which expect to get ~0.
> >
>
> This seems like a really good idea :) But again, I am not entirely sure
> if doing so would give us any unexpected behaviour. I'll wait for some
> one to reply to this and if people agree to it, I would be glad to make
> the changes to PCI_OP_READ and PCI_USER_READ_CONFIG and send a new
> patch.
If you are going to discuss and change API of config read / write
functions, please CC me.
For example pci-aardvark controller reports not only "unknown error"
happened (via 0xffffffff) but can report exact PCIe error (CRS, UR, CA)
which can be mapped to some linux errnos. I can imagine that it can be
useful for some callers.
On Tuesday 12 October 2021 21:35:13 Naveen Naidu wrote:
> On 11/10, Lukas Wunner wrote:
> > On Mon, Oct 11, 2021 at 11:37:33PM +0530, 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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
> > > data from hardware.
> >
> > Actually what happens is that PCI read transactions *time out*,
> > so the host controller fabricates a response.
> >
>
> Ah! yes. Now that I look at it, RESPONSE_IS_PCI_TIMEOUT() does indeed
> seem like a better option to RESPONSE_IS_PCI_ERROR(), since it's more
> specfic and depicts the actual condition.
This is not fully correct. 0xffffffff is returned when some error
happens. It does not have to be timeout error. Errors like Unsupported
Request, Completer Abort or Configuration Request Retry Status (when
CRSSVE bit is disabled) are also reported as 0xffffffff and they do not
represent timeout. For example Unsupported Request is returned when you
try to read from non-existent device behind some PCIe switch.
Also pci-aardvark.c fabricates value 0xffffffff when trying to read from
config space below the PCIe Root Port when PCIe link is not up.
And I have seen that Completer Abort was returned by PCIe switch when
switch itself did not received reply from device below switch. So it
means that controller can receive some reply from other device even when
no real reply was sent. Which means that timeout can be reported by some
other message.
So I think that generic PCI_ERROR is the best name. You do not know what
really happened (only some controller drivers can provide additional
information, it does not have any standard HW<-->OS API) and application
logic must decide how to process error.
> I'll wait for sometime and see if others have any objection/a better
> name for the macro and then redo the patch with that.
>
> Thank you very much for the review ^^
>
> > By contrast, a PCI *error* usually denotes an Uncorrectable or
> > Correctable Error as specified in section 6.2.2 of the PCIe Base Spec.
> >
> > Thus something like RESPONSE_IS_PCI_TIMEOUT() or IS_PCI_TIMEOUT() would
> > probably be more appropriate. I'll leave the exact bikeshed color for
> > others to decide. :-)
> >
> >
> > > Signed-off-by: Naveen Naidu <[email protected]>
> > > ---
> > > drivers/pci/hotplug/pciehp_hpc.c | 10 +++++-----
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > Acked-by: Lukas Wunner <[email protected]>
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 <[email protected]>
> > > > > ---
> > > > > 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
[+cc Pali]
On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > read.
> >
> > These definitions make error checks consistent and easier to find.
> >
> > Signed-off-by: Naveen Naidu <[email protected]>
> > ---
> > drivers/pci/access.c | 22 +++++++++++-----------
> > 1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 46935695cfb9..e1954bbbd137 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> >
> > addr = bus->ops->map_bus(bus, devfn, where);
> > if (!addr) {
> > - *val = ~0;
> > + SET_PCI_ERROR_RESPONSE(val);
>
> This to me doesn't look like kernel style. I'd rather see a define
> replace just '~0', but I defer to Bjorn.
>
> > return PCIBIOS_DEVICE_NOT_FOUND;
>
> Neither does this using custom error codes rather than standard Linux
> errno. I point this out as I that's were I'd start with the config
> accessors. Though there are lots of occurrences so we'd need a way to do
> this in manageable steps.
I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
using standard Linux error codes. That's probably a lot of work, but
Naveen has a lot of energy :)
> Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
> and delete the drivers all doing this? Then we have 2 copies (in source)
> rather than the many this series modifies. Though I'm not sure if there
> are other cases of calling pci_bus.ops.read() which expect to get ~0.
That does seem like a really good idea.
Bjorn
On Wed, Oct 13, 2021 at 01:12:01AM +0200, Pali Roh?r wrote:
> > On 11/10, Lukas Wunner wrote:
> > > On Mon, Oct 11, 2021 at 11:37:33PM +0530, 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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
> > > > data from hardware.
> > >
> > > Actually what happens is that PCI read transactions *time out*,
> > > so the host controller fabricates a response.
>
> This is not fully correct. 0xffffffff is returned when some error
> happens. It does not have to be timeout error. Errors like Unsupported
> Request, Completer Abort or Configuration Request Retry Status (when
> CRSSVE bit is disabled) are also reported as 0xffffffff and they do not
> represent timeout. For example Unsupported Request is returned when you
> try to read from non-existent device behind some PCIe switch.
This particular patch concerns pciehp and in that context,
"all ones" responses are predominantly timeouts caused by
hot-removed devices.
Thanks,
Lukas
On Tue, Oct 12, 2021 at 9:43 PM Bjorn Helgaas <[email protected]> wrote:
>
> [+cc Pali]
>
> On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> > On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > > read.
> > >
> > > These definitions make error checks consistent and easier to find.
> > >
> > > Signed-off-by: Naveen Naidu <[email protected]>
> > > ---
> > > drivers/pci/access.c | 22 +++++++++++-----------
> > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index 46935695cfb9..e1954bbbd137 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> > >
> > > addr = bus->ops->map_bus(bus, devfn, where);
> > > if (!addr) {
> > > - *val = ~0;
> > > + SET_PCI_ERROR_RESPONSE(val);
> >
> > This to me doesn't look like kernel style. I'd rather see a define
> > replace just '~0', but I defer to Bjorn.
> >
> > > return PCIBIOS_DEVICE_NOT_FOUND;
> >
> > Neither does this using custom error codes rather than standard Linux
> > errno. I point this out as I that's were I'd start with the config
> > accessors. Though there are lots of occurrences so we'd need a way to do
> > this in manageable steps.
>
> I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
> using standard Linux error codes.
Based on Pali's and your replies, I take it that these values
originate in x86 firmware, so the x86 code needs to convert to Linux
error codes and everywhere else can use Linux error codes everywhere.
> That's probably a lot of work, but
> Naveen has a lot of energy :)
There's 210 in drivers/pci/, 62 in the rest of drivers/ and 437 in
arch/. 332 are PCIBIOS_SUCCESSFUL which won't change values. Most of
drivers/pci/ and arch/ returning the value while the rest of drivers/
is comparing the returned value (mostly to PCIBIOS_SUCCESSFUL). There
could be checks such as 'if (ret > 0)' which are harder to find. A
coccinelle patch might be helpful here.
I think we want to do things in the following order:
- Rework any callers expecting a positive return value
- Make the config accessor defines convert positive error codes to
Linux error codes
- Convert pci_ops implementations to Linux error codes one by one.
I also considered we could make the accessors convert negative error
codes back to positive PCIBIOS_ values, then no callers have to be
checked/fixed first.
> > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
> > and delete the drivers all doing this? Then we have 2 copies (in source)
> > rather than the many this series modifies. Though I'm not sure if there
> > are other cases of calling pci_bus.ops.read() which expect to get ~0.
>
> That does seem like a really good idea.
I don't it matters what order we do these, so this can happen first.
Rob
On 13/10, Rob Herring wrote:
> On Tue, Oct 12, 2021 at 9:43 PM Bjorn Helgaas <[email protected]> wrote:
> >
> > [+cc Pali]
> >
> > On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> > > On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > > > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > > > read.
> > > >
> > > > These definitions make error checks consistent and easier to find.
> > > >
> > > > Signed-off-by: Naveen Naidu <[email protected]>
> > > > ---
> > > > drivers/pci/access.c | 22 +++++++++++-----------
> > > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > > index 46935695cfb9..e1954bbbd137 100644
> > > > --- a/drivers/pci/access.c
> > > > +++ b/drivers/pci/access.c
> > > > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> > > >
> > > > addr = bus->ops->map_bus(bus, devfn, where);
> > > > if (!addr) {
> > > > - *val = ~0;
> > > > + SET_PCI_ERROR_RESPONSE(val);
> > >
> > > This to me doesn't look like kernel style. I'd rather see a define
> > > replace just '~0', but I defer to Bjorn.
> > >
> > > > return PCIBIOS_DEVICE_NOT_FOUND;
> > >
> > > Neither does this using custom error codes rather than standard Linux
> > > errno. I point this out as I that's were I'd start with the config
> > > accessors. Though there are lots of occurrences so we'd need a way to do
> > > this in manageable steps.
> >
> > I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
> > using standard Linux error codes.
>
Digging through the mailing list, I see that something similar was
attempted here
https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214437.html
which did not move forward because there were a lot of moving parts (I
guess). But reading through the thread did give me an overview of what
we might wanna do.
The thread does bring up a good point, about not returning any error
values in pci_read_config_*() and converting the function definition to
something like
void pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
The reason stated in the thread was that, the error values returned from
these functions are either ignored or are not used properly. And
whenever an error occurs, the error value ~0 is anyway stored in val, we
could use that to test errors.
Ref:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.html
I bring this point because Pali mentioned that config read function can
return only PCIBIOS_SUCCESSFUL value.
Maybe instead of us trying to change pci_read_config_word, we might
wanna start small with changing PCI_OP_READ and PCI_USER_READ_CONFIG
such that they would only ever return PCI_SUCCESFUL and if any these
config accessor defines detect any error they can fabricate the value ~0
for "val" argument.
And at the caller site, instead of checking the return value of
PCI_OP_READ to detect errors, we could check the "val" for ~0 value.
But I am unable to gauge, if we should take this task before we begin
the project of removing PCIBIOS_* OR if this should be done after we
compelete with PCIBIOS_* work.
I guess the better question would be, if making PCI_OP_READ return only
PCI_SUCCESSFULL or converting it to a void, help the PCIBIOS_* work
easier?
> Based on Pali's and your replies, I take it that these values
> originate in x86 firmware, so the x86 code needs to convert to Linux
> error codes and everywhere else can use Linux error codes everywhere.
>
> > That's probably a lot of work, but
> > Naveen has a lot of energy :)
>
> There's 210 in drivers/pci/, 62 in the rest of drivers/ and 437 in
> arch/. 332 are PCIBIOS_SUCCESSFUL which won't change values. Most of
> drivers/pci/ and arch/ returning the value while the rest of drivers/
> is comparing the returned value (mostly to PCIBIOS_SUCCESSFUL). There
> could be checks such as 'if (ret > 0)' which are harder to find. A
> coccinelle patch might be helpful here.
>
> I think we want to do things in the following order:
> - Rework any callers expecting a positive return value
> - Make the config accessor defines convert positive error codes to
> Linux error codes
> - Convert pci_ops implementations to Linux error codes one by one.
>
Thank you very much for this list, this really helps me. I have been
starting at the screen since morning to come up with something like
this. IIUC, you mean:
1. When you mean "PCIBIOS_SUCCESSFUL which won't change values", did you
mean to say, that we would keep "PCIBIOS_SUCCESSFUL" define as it is
and not bother replacing it with "0"? (Atleast for the first version
of patch, and can be done in a later series)
2. "Rework any callers expecting a positive return value"
This means, find out the places where we have something like
err = pci_read_config_dword();
if (err > 0)
Then change it to:
err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc);
if (err != PCIBIOS_SUCCESSFUL)
Is there any easy way to search for these patterns, or should I look
for each instance of pci_read_config_* and other such variants and
see if such an case exists?
3. "Make the config accessor defines convert positive error codes to Linux error codes"
Do you mean something like:
#define PCI_OP_READ(size, type, len) \
int noinline pci_bus_read_config_##size \
(struct pci_bus *bus, unsigned int devfn, int pos, type *value)
\
{
if (PCI_##size##_BAD) return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER);
...
...
return pcibios_err_to_errno(res);
4. "Convert pci_ops implementations to Linux error codes one by one"
Finally, remove all the PCIBIOS_* references from the pci_ops
implementation of various drivers.
> I also considered we could make the accessors convert negative error
> codes back to positive PCIBIOS_ values, then no callers have to be
> checked/fixed first.
>
> > > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
Rob, When you say this do you mean - we have something like:
#define PCI_OPS_READ()
res = bus->ops->read();
if (res != PCIBIOS_SUCCESSFUL)
SET_PCI_ERROR_RESPONSE(val);
And the pci_ops implementation would look like:
pci_generic_config_read()
{
addr = bus->ops->map_bus();
if (!addr)
return PCIBIOS_DEVICE_NOT_FOUND;
}
This way the controller/drivers does not have to bother fabricating the
~0 value, all they have to do when they detect any error is return the
error. And the PCI_OP_READ and PCI_USER_READ_CONFIG will set the ~0
value for "val".
Pali, would you have concerns with the above design?
> > > and delete the drivers all doing this? Then we have 2 copies (in source)
> > > rather than the many this series modifies. Though I'm not sure if there
> > > are other cases of calling pci_bus.ops.read() which expect to get ~0.
> >
> > That does seem like a really good idea.
>
> I don't it matters what order we do these, so this can happen first.
>
Yes, this makes sense. I can send a patch for this first and then start
working on the PCIBIOS_* project. If anybody has any objection please do
let me know.
Thanks for the comment, it cleared up a lot of my doubts ^^
Thanks,
Naveen
Hello!
On Wednesday 13 October 2021 22:46:53 Naveen Naidu wrote:
> On 13/10, Rob Herring wrote:
> > On Tue, Oct 12, 2021 at 9:43 PM Bjorn Helgaas <[email protected]> wrote:
> > >
> > > [+cc Pali]
> > >
> > > On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> > > > On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > > > > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > > > > read.
> > > > >
> > > > > These definitions make error checks consistent and easier to find.
> > > > >
> > > > > Signed-off-by: Naveen Naidu <[email protected]>
> > > > > ---
> > > > > drivers/pci/access.c | 22 +++++++++++-----------
> > > > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > > > index 46935695cfb9..e1954bbbd137 100644
> > > > > --- a/drivers/pci/access.c
> > > > > +++ b/drivers/pci/access.c
> > > > > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> > > > >
> > > > > addr = bus->ops->map_bus(bus, devfn, where);
> > > > > if (!addr) {
> > > > > - *val = ~0;
> > > > > + SET_PCI_ERROR_RESPONSE(val);
> > > >
> > > > This to me doesn't look like kernel style. I'd rather see a define
> > > > replace just '~0', but I defer to Bjorn.
> > > >
> > > > > return PCIBIOS_DEVICE_NOT_FOUND;
> > > >
> > > > Neither does this using custom error codes rather than standard Linux
> > > > errno. I point this out as I that's were I'd start with the config
> > > > accessors. Though there are lots of occurrences so we'd need a way to do
> > > > this in manageable steps.
> > >
> > > I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
> > > using standard Linux error codes.
> >
>
> Digging through the mailing list, I see that something similar was
> attempted here
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214437.html
> which did not move forward because there were a lot of moving parts (I
> guess). But reading through the thread did give me an overview of what
> we might wanna do.
>
> The thread does bring up a good point, about not returning any error
> values in pci_read_config_*() and converting the function definition to
> something like
>
> void pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
>
> The reason stated in the thread was that, the error values returned from
> these functions are either ignored or are not used properly. And
> whenever an error occurs, the error value ~0 is anyway stored in val, we
> could use that to test errors.
>
> Ref:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.html
>
> I bring this point because Pali mentioned that config read function can
> return only PCIBIOS_SUCCESSFUL value.
Just correction. It is PCBIOS API (for x86). Read Configuration Byte has
defined only one return code: SUCCESSFUL. So if our kernel PCI API
follows this PCBIOS API then it can have only SUCCESSFUL return value.
Read Configuration Word and Read Configuration Dword have defined also
one additional return value: BAD_REGISTER_NUMBER. This return value is
retuned when you try to read 16 or 32-bit value from register which is
not aligned to 16 or 32 bits. Obviously this should not happen as kernel
code should call read function with correct argument. But bugs in kernel
code are possible...
Maybe todays compilers could allows us to define compile time checks
for PCI_OP_READ with len = 2 and 4 that register is valid number? I'm
not sure.
Anyway, runtime check for register alignment is already done in
PCI_OP_READ and PCI_USER_READ_CONFIG and it already returns
PCIBIOS_BAD_REGISTER_NUMBER. So ->read() callback itself should not be
called with incorrect argument (unless there is invisible bug).
> Maybe instead of us trying to change pci_read_config_word, we might
> wanna start small with changing PCI_OP_READ and PCI_USER_READ_CONFIG
> such that they would only ever return PCI_SUCCESFUL and if any these
> config accessor defines detect any error they can fabricate the value ~0
> for "val" argument.
>
> And at the caller site, instead of checking the return value of
> PCI_OP_READ to detect errors, we could check the "val" for ~0 value.
>
> But I am unable to gauge, if we should take this task before we begin
> the project of removing PCIBIOS_* OR if this should be done after we
> compelete with PCIBIOS_* work.
>
> I guess the better question would be, if making PCI_OP_READ return only
> PCI_SUCCESSFULL or converting it to a void, help the PCIBIOS_* work
> easier?
I would like to still see ability to indicate linux errno from read and
write config functions. Just as additional information why operation
failed. Some hardware supports it.
For example it could be used for better workarounding / fixing doing
retry on CRS response when HW does not support it (even it is mandatory
per PCIe spec). For example read / write config functions could return
-EAGAIN and PCI core would know about it... See thread and patch:
https://lore.kernel.org/linux-pci/20211007192553.GA1259781@bhelgaas/
> > Based on Pali's and your replies, I take it that these values
> > originate in x86 firmware, so the x86 code needs to convert to Linux
> > error codes and everywhere else can use Linux error codes everywhere.
> >
> > > That's probably a lot of work, but
> > > Naveen has a lot of energy :)
> >
> > There's 210 in drivers/pci/, 62 in the rest of drivers/ and 437 in
> > arch/. 332 are PCIBIOS_SUCCESSFUL which won't change values. Most of
> > drivers/pci/ and arch/ returning the value while the rest of drivers/
> > is comparing the returned value (mostly to PCIBIOS_SUCCESSFUL). There
> > could be checks such as 'if (ret > 0)' which are harder to find. A
> > coccinelle patch might be helpful here.
> >
> > I think we want to do things in the following order:
> > - Rework any callers expecting a positive return value
> > - Make the config accessor defines convert positive error codes to
> > Linux error codes
> > - Convert pci_ops implementations to Linux error codes one by one.
> >
>
> Thank you very much for this list, this really helps me. I have been
> starting at the screen since morning to come up with something like
> this. IIUC, you mean:
>
> 1. When you mean "PCIBIOS_SUCCESSFUL which won't change values", did you
> mean to say, that we would keep "PCIBIOS_SUCCESSFUL" define as it is
> and not bother replacing it with "0"? (Atleast for the first version
> of patch, and can be done in a later series)
>
> 2. "Rework any callers expecting a positive return value"
>
> This means, find out the places where we have something like
>
> err = pci_read_config_dword();
> if (err > 0)
>
> Then change it to:
>
> err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc);
> if (err != PCIBIOS_SUCCESSFUL)
>
> Is there any easy way to search for these patterns, or should I look
> for each instance of pci_read_config_* and other such variants and
> see if such an case exists?
>
> 3. "Make the config accessor defines convert positive error codes to Linux error codes"
>
> Do you mean something like:
>
> #define PCI_OP_READ(size, type, len) \
> int noinline pci_bus_read_config_##size \
> (struct pci_bus *bus, unsigned int devfn, int pos, type *value)
> \
> {
> if (PCI_##size##_BAD) return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER);
> ...
> ...
> return pcibios_err_to_errno(res);
>
>
> 4. "Convert pci_ops implementations to Linux error codes one by one"
>
> Finally, remove all the PCIBIOS_* references from the pci_ops
> implementation of various drivers.
>
> > I also considered we could make the accessors convert negative error
> > codes back to positive PCIBIOS_ values, then no callers have to be
> > checked/fixed first.
> >
> > > > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
>
> Rob, When you say this do you mean - we have something like:
>
> #define PCI_OPS_READ()
> res = bus->ops->read();
> if (res != PCIBIOS_SUCCESSFUL)
> SET_PCI_ERROR_RESPONSE(val);
>
> And the pci_ops implementation would look like:
>
> pci_generic_config_read()
> {
> addr = bus->ops->map_bus();
> if (!addr)
> return PCIBIOS_DEVICE_NOT_FOUND;
> }
>
> This way the controller/drivers does not have to bother fabricating the
> ~0 value, all they have to do when they detect any error is return the
> error. And the PCI_OP_READ and PCI_USER_READ_CONFIG will set the ~0
> value for "val".
>
> Pali, would you have concerns with the above design?
Beware that not all controllers are using map_bus callback. E.g.
pci-aardvark does not use map_bus and neither pci_generic_config_read(),
instead it implements own read callback.
But idea to fabricate 0xffffffff in PCI core code based on return value
and let controller drivers to just return -errno (without need to
fabricate *value in ptr) is a nice cleanup. It removes lot of repeated
code. And also some bugs :-) as I see that in some cases fabricated
0xffffffff is not set on error.
I like it.
> > > > and delete the drivers all doing this? Then we have 2 copies (in source)
> > > > rather than the many this series modifies. Though I'm not sure if there
> > > > are other cases of calling pci_bus.ops.read() which expect to get ~0.
> > >
> > > That does seem like a really good idea.
> >
> > I don't it matters what order we do these, so this can happen first.
> >
>
> Yes, this makes sense. I can send a patch for this first and then start
> working on the PCIBIOS_* project. If anybody has any objection please do
> let me know.
>
> Thanks for the comment, it cleared up a lot of my doubts ^^
>
> Thanks,
> Naveen
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 <[email protected]>
> > > > > > ---
> > > > > > 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
On Wed, Oct 13, 2021 at 10:46:53PM +0530, Naveen Naidu wrote:
> 2. "Rework any callers expecting a positive return value"
>
> This means, find out the places where we have something like
>
> err = pci_read_config_dword();
> if (err > 0)
>
> Then change it to:
>
> err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc);
> if (err != PCIBIOS_SUCCESSFUL)
I'm sure this is obvious, but I would try hard not to add any new uses
of PCIBIOS_SUCCESSFUL.
> Is there any easy way to search for these patterns, or should I look
> for each instance of pci_read_config_* and other such variants and
> see if such an case exists?
coccigrep might be able to find things like this, but I'ver never
really become friends with it.
On Wed, Oct 13, 2021 at 12:17 PM Naveen Naidu <[email protected]> wrote:
>
> On 13/10, Rob Herring wrote:
> > On Tue, Oct 12, 2021 at 9:43 PM Bjorn Helgaas <[email protected]> wrote:
> > >
> > > [+cc Pali]
> > >
> > > On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> > > > On Mon, Oct 11, 2021 at 11:08:32PM +0530, 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 and
> > > > > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > > > > read.
> > > > >
> > > > > These definitions make error checks consistent and easier to find.
> > > > >
> > > > > Signed-off-by: Naveen Naidu <[email protected]>
> > > > > ---
> > > > > drivers/pci/access.c | 22 +++++++++++-----------
> > > > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > > > index 46935695cfb9..e1954bbbd137 100644
> > > > > --- a/drivers/pci/access.c
> > > > > +++ b/drivers/pci/access.c
> > > > > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> > > > >
> > > > > addr = bus->ops->map_bus(bus, devfn, where);
> > > > > if (!addr) {
> > > > > - *val = ~0;
> > > > > + SET_PCI_ERROR_RESPONSE(val);
> > > >
> > > > This to me doesn't look like kernel style. I'd rather see a define
> > > > replace just '~0', but I defer to Bjorn.
> > > >
> > > > > return PCIBIOS_DEVICE_NOT_FOUND;
> > > >
> > > > Neither does this using custom error codes rather than standard Linux
> > > > errno. I point this out as I that's were I'd start with the config
> > > > accessors. Though there are lots of occurrences so we'd need a way to do
> > > > this in manageable steps.
> > >
> > > I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
> > > using standard Linux error codes.
> >
>
> Digging through the mailing list, I see that something similar was
> attempted here
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214437.html
> which did not move forward because there were a lot of moving parts (I
> guess). But reading through the thread did give me an overview of what
> we might wanna do.
Skimming it, looks like good advice from Arnd on what to do or not do.
> The thread does bring up a good point, about not returning any error
> values in pci_read_config_*() and converting the function definition to
> something like
>
> void pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
>
> The reason stated in the thread was that, the error values returned from
> these functions are either ignored or are not used properly. And
> whenever an error occurs, the error value ~0 is anyway stored in val, we
> could use that to test errors.
Presumably, there could be some register somewhere where all 1s is
valid? So I think we need the error values.
Also, I seem to recall only the vendor/device IDs are defined to be
all 1s for non-existent devices. Other errors are undefined?
> Ref:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.html
>
> I bring this point because Pali mentioned that config read function can
> return only PCIBIOS_SUCCESSFUL value.
>
> Maybe instead of us trying to change pci_read_config_word, we might
> wanna start small with changing PCI_OP_READ and PCI_USER_READ_CONFIG
> such that they would only ever return PCI_SUCCESFUL and if any these
> config accessor defines detect any error they can fabricate the value ~0
> for "val" argument.
>
> And at the caller site, instead of checking the return value of
> PCI_OP_READ to detect errors, we could check the "val" for ~0 value.
>
> But I am unable to gauge, if we should take this task before we begin
> the project of removing PCIBIOS_* OR if this should be done after we
> compelete with PCIBIOS_* work.
>
> I guess the better question would be, if making PCI_OP_READ return only
> PCI_SUCCESSFULL or converting it to a void, help the PCIBIOS_* work
> easier?
>
> > Based on Pali's and your replies, I take it that these values
> > originate in x86 firmware, so the x86 code needs to convert to Linux
> > error codes and everywhere else can use Linux error codes everywhere.
> >
> > > That's probably a lot of work, but
> > > Naveen has a lot of energy :)
> >
> > There's 210 in drivers/pci/, 62 in the rest of drivers/ and 437 in
> > arch/. 332 are PCIBIOS_SUCCESSFUL which won't change values. Most of
> > drivers/pci/ and arch/ returning the value while the rest of drivers/
> > is comparing the returned value (mostly to PCIBIOS_SUCCESSFUL). There
> > could be checks such as 'if (ret > 0)' which are harder to find. A
> > coccinelle patch might be helpful here.
> >
> > I think we want to do things in the following order:
> > - Rework any callers expecting a positive return value
> > - Make the config accessor defines convert positive error codes to
> > Linux error codes
> > - Convert pci_ops implementations to Linux error codes one by one.
> >
>
> Thank you very much for this list, this really helps me. I have been
> starting at the screen since morning to come up with something like
> this. IIUC, you mean:
>
> 1. When you mean "PCIBIOS_SUCCESSFUL which won't change values", did you
> mean to say, that we would keep "PCIBIOS_SUCCESSFUL" define as it is
> and not bother replacing it with "0"? (Atleast for the first version
> of patch, and can be done in a later series)
Yes, removal of PCIBIOS_SUCCESSFUL can be done after/separately. That
greatly reduces the number of callers to touch.
> 2. "Rework any callers expecting a positive return value"
>
> This means, find out the places where we have something like
>
> err = pci_read_config_dword();
> if (err > 0)
>
> Then change it to:
>
> err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc);
> if (err != PCIBIOS_SUCCESSFUL)
As Bjorn said, don't add more!
Just:
if (err)
Because that works whether we change the error codes or not.
> Is there any easy way to search for these patterns, or should I look
> for each instance of pci_read_config_* and other such variants and
> see if such an case exists?
Besides grep and/or coccigrep, change the function definitions to
return void (or a ptr) and do allyesconfig builds (with 'make -k') .
Using coccinelle directly would make the changes for you. It's fairly
hard to understand and use in my limited experience.
Also keep in mind the error could get passed out of a function and
then checked elsewhere. That you can't really automate checking.
Searching, that seems to be fairly common, but I would guess most
cases are just comparing to 0 if they check. This is what I used:
git grep -W '=\spci_read_config_'
You could then grep/sed the result of this to get the functions, and
then grep using those functions to check the callers.
I also see several cases checking for < 0 already, so we'd actually be
fixing those. :)
>
> 3. "Make the config accessor defines convert positive error codes to Linux error codes"
>
> Do you mean something like:
>
> #define PCI_OP_READ(size, type, len) \
> int noinline pci_bus_read_config_##size \
> (struct pci_bus *bus, unsigned int devfn, int pos, type *value)
> \
> {
> if (PCI_##size##_BAD) return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER);
> ...
> ...
> return pcibios_err_to_errno(res);
Right.
>
> 4. "Convert pci_ops implementations to Linux error codes one by one"
>
> Finally, remove all the PCIBIOS_* references from the pci_ops
> implementation of various drivers.
Right.
>
> > I also considered we could make the accessors convert negative error
> > codes back to positive PCIBIOS_ values, then no callers have to be
> > checked/fixed first.
> >
> > > > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
>
> Rob, When you say this do you mean - we have something like:
>
> #define PCI_OPS_READ()
> res = bus->ops->read();
> if (res != PCIBIOS_SUCCESSFUL)
if (res)
> SET_PCI_ERROR_RESPONSE(val);
I still don't like that style, but when there's only 2 occurrences, I
don't really care.
> And the pci_ops implementation would look like:
>
> pci_generic_config_read()
> {
> addr = bus->ops->map_bus();
> if (!addr)
> return PCIBIOS_DEVICE_NOT_FOUND;
> }
>
> This way the controller/drivers does not have to bother fabricating the
> ~0 value, all they have to do when they detect any error is return the
> error. And the PCI_OP_READ and PCI_USER_READ_CONFIG will set the ~0
> value for "val".
Right.
>
> Pali, would you have concerns with the above design?
>
> > > > and delete the drivers all doing this? Then we have 2 copies (in source)
> > > > rather than the many this series modifies. Though I'm not sure if there
> > > > are other cases of calling pci_bus.ops.read() which expect to get ~0.
> > >
> > > That does seem like a really good idea.
> >
> > I don't it matters what order we do these, so this can happen first.
> >
>
> Yes, this makes sense. I can send a patch for this first and then start
> working on the PCIBIOS_* project. If anybody has any objection please do
> let me know.
>
> Thanks for the comment, it cleared up a lot of my doubts ^^
Sure, thanks for working on this.
Rob
On Wednesday 13 October 2021 16:47:43 Rob Herring wrote:
> On Wed, Oct 13, 2021 at 12:17 PM Naveen Naidu <[email protected]> wrote:
> > The thread does bring up a good point, about not returning any error
> > values in pci_read_config_*() and converting the function definition to
> > something like
> >
> > void pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
> >
> > The reason stated in the thread was that, the error values returned from
> > these functions are either ignored or are not used properly. And
> > whenever an error occurs, the error value ~0 is anyway stored in val, we
> > could use that to test errors.
>
> Presumably, there could be some register somewhere where all 1s is
> valid? So I think we need the error values.
I guess that "Prefetchable Base/Limit Upper 32 Bits" PCI registers can
contains all-ones value and it is valid value in these registers.
And also PCIe regs like "Slot Capabilities Register" can also have all
bits set.
So 0xffffffff does not mean that error happened. It is needed some
application logic which can decide based on other things (like register
number, device state, etc...) if 0xffffffff indicates error or not.
Therefore return errno values can help, but only for controllers which
provide this additional errno information.
> Also, I seem to recall only the vendor/device IDs are defined to be
> all 1s for non-existent devices. Other errors are undefined?
In PCIe spec for vendor id register is mentioned that 0xffff indicates
no Function is present.
On Wed, Oct 13, 2021 at 04:47:43PM -0500, Rob Herring wrote:
> Presumably, there could be some register somewhere where all 1s is
> valid? So I think we need the error values.
We have to assume ~0 is a valid value for any config registers except
the few defined by the spec that have bits required to be 0. There
can be all kinds of vendor-defined stuff in config space that can be
anything.
> Also, I seem to recall only the vendor/device IDs are defined to be
> all 1s for non-existent devices. Other errors are undefined?
I think this case is actually an instance of the PCI controller
fabricating ~0 because a PCI/PCIe error occurred (I think on PCI it's
a Master Abort when nothing responds; on PCIe the read terminates as
an Unsupported Request (PCIe r5.0, sec 2.3.2)).
On 10/11/2021 12:06 PM, 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 RESPONSE_IS_PCI_ERROR() to check the response we get when we read
> data from hardware.
>
> This helps unify PCI error response checking and make error checks
> consistent and easier to find.
>
> Signed-off-by: Naveen Naidu <[email protected]>
> ---
> drivers/pci/controller/vmd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index a5987e52700e..db81bc4cfe8c 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -538,7 +538,7 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
> int ret;
>
> ret = pci_read_config_dword(dev, PCI_REG_VMLOCK, &vmlock);
> - if (ret || vmlock == ~0)
> + if (ret || RESPONSE_IS_PCI_ERROR(&vmlock))
> return -ENODEV;
>
> if (MB2_SHADOW_EN(vmlock)) {
>
Reviewed-by: Jonathan Derrick <[email protected]>