PCI: Unify PCI error response checking
Subsystems (22)
cover PCI: Unify PCI error response checking
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(-)
Signature · 6 lines
2.25.1 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
PATCH 1/22 PCI: Add PCI_ERROR_RESPONSE and it's related defintions
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.
--- include/linux/pci.h | 9 +++++++++ 1 file changed, 9 insertions(+)
include/linux/pci.h +9 -0
@@ -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.
Signature · 1 line
2.25.1
PATCH 2/22 PCI: Unify PCI error response checking
In reply to Naveen NaiduAn 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.
--- drivers/pci/access.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
drivers/pci/access.c +11 -11
@@ -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);
Signature · 1 line
2.25.1
Rob Herring
reviewerQuoted 20 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -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.
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 85
Quoted 1 line
drivers/pci/access.c +0
-0return 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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Quoted 22 lines · Rob Herring
Quoted 21 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0@@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
Quoted 8 lines
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 4 lines
drivers/pci/access.c +1
-1addr = 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.
Quoted 7 lines
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 85
Quoted 1 line
drivers/pci/access.c +0
-0return 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.
Quoted 5 lines
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset 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 callingpci_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 :-)
Quoted 1 line
Rob
Rob Herring
reviewerQuoted 33 lines · Naveen Naidu
Quoted 22 lines · Rob Herring
Quoted 21 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0@@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,Quoted 8 lines
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 4 lines
drivers/pci/access.c +1
-1addr = 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.
Quoted 3 lines
Apologies, if this is a lame question. Why is the macro
SET_PCI_ERROR_RESPONSEnot 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.
Quoted 6 lines
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.
Quoted 14 lines
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.
Quoted 7 lines
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 85
Quoted 1 line
drivers/pci/access.c +0
-0return 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.
Quoted 3 lines
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).
Quoted 11 lines
Quoted 5 lines
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset 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 callingpci_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_READandPCI_USER_READ_CONFIGand send a new patch.
I'm expecting Bjorn to chime in.
Rob
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Pali Rohár
maintainerQuoted 10 lines · On Tuesday 12 October 2021 21:51:08 Naveen Naidu wrote:
Quoted 8 lines · Rob Herring
Quoted 1 line · Naveen Naidu
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.
Quoted 3 lines
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.
Quoted 11 lines
Quoted 5 lines
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset 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 callingpci_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_READandPCI_USER_READ_CONFIGand 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.
[+cc Pali]
Quoted 30 lines · Rob Herring
Quoted 20 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -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.
Quoted 6 lines
drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 85
Quoted 1 line
drivers/pci/access.c +0
-0return 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 :)
Quoted 4 lines
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset 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 callingpci_bus.ops.read()which expect to get ~0.
That does seem like a really good idea.
Bjorn
Rob Herring
reviewerQuoted 36 lines · Bjorn Helgaas
[+cc Pali]
Quoted 32 lines · Rob Herring
Quoted 20 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -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.
Quoted 1 line
drivers/pci/access.c +0
-0return PCIBIOS_DEVICE_NOT_FOUND ;
Quoted 7 lines
Quoted 4 lines
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/x86and 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.
Quoted 2 lines
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.
Quoted 6 lines
Quoted 4 lines
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset 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 callingpci_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
Quoted 37 lines · Rob Herring
Quoted 36 lines · Bjorn Helgaas
[+cc Pali]
Quoted 32 lines · Rob Herring
Quoted 20 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -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.
Quoted 1 line
drivers/pci/access.c +0
-0return PCIBIOS_DEVICE_NOT_FOUND ;
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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/x86and 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?
Quoted 14 lines
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.
Quoted 2 lines
That's probably a lot of work, but
Naveen has a lot of energy :)There's 210 in
drivers/pci/, 62 in the rest ofdrivers/and 437 inarch/. 332 arePCIBIOS_SUCCESSFULwhich won't change values. Most ofdrivers/pci/andarch/returning the value while the rest ofdrivers/is comparing the returned value (mostly toPCIBIOS_SUCCESSFUL). There could be checks such as 'if (ret > 0)' which are harder to find. A coccinelle patch might be helpful here.
Quoted 6 lines
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
- Convertpci_opsimplementations 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.
Quoted 5 lines
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.Quoted 1 line
Quoted 1 line
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset 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?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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
Pali Rohár
maintainerHello!
Quoted 65 lines · On Wednesday 13 October 2021 22:46:53 Naveen Naidu wrote:
Quoted 37 lines · Rob Herring
Quoted 36 lines · Bjorn Helgaas
[+cc Pali]
Quoted 32 lines · Rob Herring
Quoted 20 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -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.
Quoted 1 line
drivers/pci/access.c +0
-0return PCIBIOS_DEVICE_NOT_FOUND ;Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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/x86and 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.
Quoted 5 lines
Ref:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.htmlI bring this point because Pali mentioned that config read function can return only
PCIBIOS_SUCCESSFULvalue.
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).
Quoted 9 lines
Maybe instead of us trying to change
pci_read_config_word, we might wanna start small with changingPCI_OP_READandPCI_USER_READ_CONFIGsuch that they would only ever returnPCI_SUCCESFULand 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_READto detect errors, we could check the "val" for ~0 value.
Quoted 7 lines
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_READreturn onlyPCI_SUCCESSFULLor 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/
Quoted 88 lines
Quoted 14 lines
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.
Quoted 2 lines
That's probably a lot of work, but
Naveen has a lot of energy :)There's 210 in
drivers/pci/, 62 in the rest ofdrivers/and 437 inarch/. 332 arePCIBIOS_SUCCESSFULwhich won't change values. Most ofdrivers/pci/andarch/returning the value while the rest ofdrivers/is comparing the returned value (mostly toPCIBIOS_SUCCESSFUL). There could be checks such as 'if (ret > 0)' which are harder to find. A coccinelle patch might be helpful here.Quoted 6 lines
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
- Convertpci_opsimplementations 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_SUCCESSFULwhich 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_opsimplementations to Linux error codes one by one"Finally, remove all the PCIBIOS_* references from the
pci_opsimplementation of various drivers.Quoted 5 lines
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.Quoted 1 line
Quoted 1 line
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset the data valueRob, 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_opsimplementation would look like:pci_generic_config_read() { addr = bus->ops->map_bus(); if (!addr) return PCIBIOS_DEVICE_NOT_FOUND; }
Quoted 6 lines
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_READandPCI_USER_READ_CONFIGwill 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.
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 8 lines
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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Quoted 11 lines · Naveen Naidu
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.
Quoted 3 lines
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.
_______________________________________________
Linux-kernel-mentees mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Rob Herring
reviewerQuoted 48 lines · Naveen Naidu
Quoted 37 lines · Rob Herring
Quoted 36 lines · Bjorn Helgaas
[+cc Pali]
Quoted 32 lines · Rob Herring
Quoted 20 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
SET_PCI_ERROR_RESPONSE()to set the error response andRESPONSE_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(-)drivers/pci/access.c +0
-0drivers/pci/access.c int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, · line 84
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -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.
Quoted 1 line
drivers/pci/access.c +0
-0return PCIBIOS_DEVICE_NOT_FOUND ;Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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/x86and everywhere else using standard Linux error codes.
Quoted 6 lines
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.
Quoted 10 lines
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?
Quoted 44 lines
Ref:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.htmlI bring this point because Pali mentioned that config read function can return only
PCIBIOS_SUCCESSFULvalue.Maybe instead of us trying to change
pci_read_config_word, we might wanna start small with changingPCI_OP_READandPCI_USER_READ_CONFIGsuch that they would only ever returnPCI_SUCCESFULand 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_READto 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_READreturn onlyPCI_SUCCESSFULLor converting it to a void, help the PCIBIOS_* work easier?Quoted 14 lines
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.
Quoted 2 lines
That's probably a lot of work, but
Naveen has a lot of energy :)There's 210 in
drivers/pci/, 62 in the rest ofdrivers/and 437 inarch/. 332 arePCIBIOS_SUCCESSFULwhich won't change values. Most ofdrivers/pci/andarch/returning the value while the rest ofdrivers/is comparing the returned value (mostly toPCIBIOS_SUCCESSFUL). There could be checks such as 'if (ret > 0)' which are harder to find. A coccinelle patch might be helpful here.Quoted 6 lines
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
- Convertpci_opsimplementations to Linux error codes one by one.
Quoted 8 lines
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_SUCCESSFULwhich 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.
Quoted 11 lines
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.
Quoted 3 lines
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. :)
Quoted 14 lines
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.
Quoted 5 lines
4. "Convert
pci_opsimplementations to Linux error codes one by one"Finally, remove all the PCIBIOS_* references from the
pci_opsimplementation of various drivers.
Right.
Quoted 5 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Can't we make
PCI_OP_READandPCI_USER_READ_CONFIGset the data valueRob, 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)
Quoted 1 line
SET_PCI_ERROR_RESPONSE(val);
I still don't like that style, but when there's only 2 occurrences, I don't really care.
Quoted 9 lines
And the
pci_opsimplementation would look like:pci_generic_config_read() { addr = bus->ops->map_bus(); if (!addr) return PCIBIOS_DEVICE_NOT_FOUND; }
Quoted 4 lines
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_READandPCI_USER_READ_CONFIGwill set the ~0 value for "val".
Right.
Quoted 9 lines
Pali, would you have concerns with the above design?
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 2 lines
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
Pali Rohár
maintainerQuoted 7 lines · On Wednesday 13 October 2021 16:47:43 Rob Herring wrote:
Quoted 6 lines · Naveen Naidu
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)
Quoted 7 lines
Quoted 4 lines
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.
Quoted 2 lines
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.
Quoted 2 lines · Rob Herring
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.
Quoted 2 lines
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)).
PATCH 3/22 PCI: thunder: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pci-thunder-ecam.c | 20 ++++++++++---------- drivers/pci/controller/pci-thunder-pem.c | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-)
drivers/pci/controller/pci-thunder-ecam.c +10 -10
@@ -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);
drivers/pci/controller/pci-thunder-pem.c +1 -1
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 4/22 PCI: iproc: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pcie-iproc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pcie-iproc.c +1 -1
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 5/22 PCI: mediatek: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pcie-mediatek.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/controller/pcie-mediatek.c +2 -2
@@ -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; }
Signature · 1 line
2.25.1
PATCH 6/22 PCI: exynos: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/dwc/pci-exynos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pci-exynos.c +1 -1
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 7/22 PCI: histb: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/dwc/pcie-histb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-histb.c +1 -1
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 8/22 PCI: kirin: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/dwc/pcie-kirin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-kirin.c +1 -1
@@ -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 ; }
Signature · 6 lines
2.25.1 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
PATCH 9/22 PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pci-aardvark.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pci/controller/pci-aardvark.c +4 -4
@@ -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 ; }
Signature · 1 line
2.25.1
Pali Rohár
maintainerRe: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
Quoted 22 lines · 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(-)drivers/pci/controller/pci-aardvark.c +0
-0
drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 897
Quoted 5 lines
drivers/pci/controller/pci-aardvark.c +1
-1@@ -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?
Quoted 32 lines
drivers/pci/controller/pci-aardvark.c +3
-3return 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 ; }Signature · 1 line
2.25.1
Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
Quoted 25 lines · Pali Rohár
Quoted 24 lines · 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(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,int ret;
Quoted 8 lines
drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 897
Quoted 3 lines
drivers/pci/controller/pci-aardvark.c +1
-1if (!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.
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.
Quoted 29 lines
Quoted 29 lines
drivers/pci/controller/pci-aardvark.c +3
-3return 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 ; }
Quoted 3 lines
Quoted 3 lines
Signature · 1 line
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
A message that is not in the archive (sent off-list or not yet received): 20211011182526.kboaxqofdpd2jjrl@theprophet
Pali Rohár
maintainerRe: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
Quoted 47 lines · On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
Quoted 25 lines · Pali Rohár
Quoted 24 lines · 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(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,int ret;Quoted 8 lines
drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 897
Quoted 3 lines
drivers/pci/controller/pci-aardvark.c +1
-1if (!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_RESPONSEmacro 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.
Quoted 7 lines
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.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
drivers/pci/controller/pci-aardvark.c +3
-3return 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 ; }Signature · 1 line
2.25.1
Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
Quoted 51 lines · Pali Rohár
Quoted 50 lines · On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
Quoted 25 lines · Pali Rohár
Quoted 24 lines · 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(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,int ret;Quoted 8 lines
drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 897
Quoted 3 lines
drivers/pci/controller/pci-aardvark.c +1
-1if (!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_RESPONSEmacro 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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 15 lines
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.cdriver and U-Boot has for (probably same) purposepci_get_ff()macro, see:
https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367
Quoted 6 lines
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 :)
Quoted 6 lines
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.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
drivers/pci/controller/pci-aardvark.c +3
-3return 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 ; }Signature · 1 line
2.25.1
Re: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
Quoted 51 lines · Naveen Naidu
Quoted 50 lines · Pali Rohár
Quoted 26 lines · On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
Quoted 25 lines · Pali Rohár
Quoted 24 lines · 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(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,int ret;Quoted 7 lines
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 897
Quoted 3 lines
drivers/pci/controller/pci-aardvark.c +1
-1if (!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.cdriver and U-Boot has for (probably same) purposepci_get_ff()macro, see:
https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367
Quoted 5 lines
Quoted 5 lines
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).
Quoted 2 lines
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.
Quoted 3 lines
Quoted 3 lines
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
Pali Rohár
maintainerRe: [PATCH 09/22] PCI: aardvark: Use SET_PCI_ERROR_RESPONSE() when device not found
Quoted 63 lines · On Tuesday 12 October 2021 21:13:10 Bjorn Helgaas wrote:
Quoted 51 lines · Naveen Naidu
Quoted 50 lines · Pali Rohár
Quoted 26 lines · On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:
Quoted 25 lines · Pali Rohár
Quoted 24 lines · 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(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,int ret;Quoted 7 lines
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, · line 897
Quoted 3 lines
drivers/pci/controller/pci-aardvark.c +1
-1if (!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.cdriver and U-Boot has for (probably same) purposepci_get_ff()macro, see:
https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367Quoted 5 lines
Quoted 5 lines
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 to0xffffffffregardless of the "size" value becausepci_bus_read_config_byte()andpci_bus_read_config_word()extract out the part they need:
Quoted 5 lines
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 indrivers/pci/access.c).
Ok! No problem if this is something which is not going to be changed.
Quoted 6 lines
Quoted 2 lines
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
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
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
PATCH 10/22 PCI: mvebu: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pci-mvebu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/controller/pci-mvebu.c +2 -2
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 11/22 PCI: altera: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pcie-altera.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pcie-altera.c +1 -1
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 12/22 PCI: rcar: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pcie-rcar-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pcie-rcar-host.c +1 -1
@@ -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; }
Signature · 1 line
2.25.1
PATCH 13/22 PCI: rockchip: Use SET_PCI_ERROR_RESPONSE() when device not found
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/pcie-rockchip-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pcie-rockchip-host.c +1 -1
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 14/22 PCI/ERR: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
In reply to Naveen NaiduAn 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.
--- drivers/pci/pci.c | 10 +++++----- drivers/pci/probe.c | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-)
drivers/pci/pci.c +5 -5
@@ -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);
drivers/pci/probe.c +5 -5
@@ -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;
Signature · 1 line
2.25.1
PATCH 15/22 PCI: vmd: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
In reply to Naveen NaiduAn 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.
--- drivers/pci/controller/vmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/vmd.c +1 -1
@@ -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)) {
Signature · 1 line
2.25.1
Re: [PATCH 15/22] PCI: vmd: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
Quoted 21 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
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(-)drivers/pci/controller/vmd.c +0
-0
drivers/pci/controller/vmd.c static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint, · line 544
Quoted 8 lines
drivers/pci/controller/vmd.c +1
-1@@ -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)) {
PATCH 16/22 PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
In reply to Naveen NaiduAn 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.
--- drivers/pci/hotplug/pciehp_hpc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +5 -5
@@ -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);
Signature · 1 line
2.25.1
Re: [PATCH 16/22] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
Quoted 6 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
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. :-)
Quoted 4 lines
Signed-off-by Naveen Naidu <[email protected]>--- drivers/pci/hotplug/pciehp_hpc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Re: [PATCH 16/22] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
Quoted 11 lines · Lukas Wunner
Quoted 6 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
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 ^^
Quoted 14 lines
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()orIS_PCI_TIMEOUT()would probably be more appropriate. I'll leave the exact bikeshed color for others to decide. :-)Quoted 4 lines
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]>
Pali Rohár
maintainerRe: [PATCH 16/22] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
Quoted 9 lines · On Tuesday 12 October 2021 21:35:13 Naveen Naidu wrote:
Quoted 8 lines · Lukas Wunner
Quoted 6 lines · Naveen Naidu
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data.
Use
RESPONSE_IS_PCI_ERROR()to check the response we get when we read data from hardware.
Quoted 7 lines
Quoted 3 lines
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 toRESPONSE_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.
Quoted 19 lines
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 ^^
Quoted 14 lines
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()orIS_PCI_TIMEOUT()would probably be more appropriate. I'll leave the exact bikeshed color for others to decide. :-)Quoted 4 lines
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]>
Re: [PATCH 16/22] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
Quoted 12 lines · Pali Rohár
Quoted 11 lines · Lukas Wunner
Quoted 10 lines · Naveen Naidu
Quoted 6 lines
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.
Quoted 6 lines
This is not fully correct.
0xffffffffis 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 as0xffffffffand 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
PATCH 17/22 PCI/DPC: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
In reply to Naveen NaiduAn 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.
--- drivers/pci/pcie/dpc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/pcie/dpc.c +2 -2
@@ -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,
Signature · 1 line
2.25.1
PATCH 18/22 PCI/PME: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
In reply to Naveen NaiduAn 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.
--- drivers/pci/pcie/pme.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/pcie/pme.c +2 -2
@@ -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 ; }
Signature · 1 line
2.25.1
PATCH 19/22 PCI: cpqphp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
In reply to Naveen NaiduAn 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.
--- drivers/pci/hotplug/cpqphp_ctrl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/hotplug/cpqphp_ctrl.c +2 -2
@@ -2273,7 +2273,7 @@ static u32 configure_new_device(struct controller *ctrl, struct pci_func *funcwhile ((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 *funcpci_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);
Signature · 1 line
2.25.1
PATCH 20/22 PCI: keystone: Use PCI_ERROR_RESPONSE to specify hardware error
In reply to Naveen NaiduInclude 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.
--- drivers/pci/controller/dwc/pci-keystone.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/pci-keystone.c +2 -2
@@ -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)
Signature · 1 line
2.25.1
PATCH 21/22 PCI: hv: Use PCI_ERROR_RESPONSE to specify hardware read error
In reply to Naveen NaiduInclude 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.
--- drivers/pci/controller/pci-hyperv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pci-hyperv.c +1 -1
@@ -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);
Signature · 1 line
2.25.1
PATCH 22/22 PCI: xgene: Use PCI_ERROR_RESPONSE to specify hardware error
In reply to Naveen NaiduInclude PCI_ERROR_RESPONSE along with 0xffffffff in the comment to specify a hardware error. This makes finding where MMIO read error occurs easier.
--- drivers/pci/controller/pci-xgene.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pci/controller/pci-xgene.c +4 -4
@@ -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 ))
Signature · 1 line
2.25.1