Re: [PATCH v2 18/24] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to check read from hardware
flat view
From: Naveen Naidu <hidden>
Date: 2021-10-15 15:16:19
Also in:
linux-kernel-mentees, lkml
On 15/10, Naveen Naidu wrote:
quoted hunk ↗ jump to hunk
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data. Use RESPONSE_IS_PCI_ERROR() to check the response we get when we read data from hardware. This helps unify PCI error response checking and make error checks consistent and easier to find. Compile tested only. Acked-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Naveen Naidu <redacted> --- drivers/pci/hotplug/pciehp_hpc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 3024d7e85e6a..8a2f6bb643b5 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c@@ -89,7 +89,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout) do { pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); - if (slot_status == (u16) ~0) { + if (RESPONSE_IS_PCI_ERROR(&slot_status)) { ctrl_info(ctrl, "%s: no response from device\n", __func__); return 0;@@ -165,7 +165,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd, pcie_wait_cmd(ctrl); pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl); - if (slot_ctrl == (u16) ~0) { + if (RESPONSE_IS_PCI_ERROR(&slot_ctrl)) { ctrl_info(ctrl, "%s: no response from device\n", __func__); goto out; }@@ -236,7 +236,7 @@ int pciehp_check_link_active(struct controller *ctrl) int ret; ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); - if (ret == PCIBIOS_DEVICE_NOT_FOUND || lnk_status == (u16)~0) + if (ret == PCIBIOS_DEVICE_NOT_FOUND || RESPONSE_IS_PCI_ERROR(&lnk_status)) return -ENODEV; ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);@@ -443,7 +443,7 @@ int pciehp_card_present(struct controller *ctrl) int ret; ret = pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); - if (ret == PCIBIOS_DEVICE_NOT_FOUND || slot_status == (u16)~0) + if (ret == PCIBIOS_DEVICE_NOT_FOUND || RESPONSE_IS_PCI_ERROR(&slot_status)) return -ENODEV; return !!(slot_status & PCI_EXP_SLTSTA_PDS);@@ -621,7 +621,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id) read_status: pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status); - if (status == (u16) ~0) { + if (RESPONSE_IS_PCI_ERROR(&status)) { ctrl_info(ctrl, "%s: no response from device\n", __func__); if (parent) pm_runtime_put(parent);-- 2.25.1
Lukas, I have added your Acked-by tag from the v1 [1] of patch series, since this patch has not changed in v2. I hope that's okay. If not, I apologize for that and can resend the patch series without the tag. Apologies for the inconvenience. [1]: https://lore.kernel.org/linux-pci/20211011194740.GA14357@wunner.de/ (local) Also, regarding your comments from v1 patch series [1] about re-naming the RESPONSE_IS_PCI_ERROR to RESPONSE_IS_PCI_TIMEOUT. We could indeed change the change to RESPONSE_IS_PCI_TIMEOUT for pciehp, but then I'm afraid that picehp would be the odd one out. I mean, since in all the other places we are using RESPONE_IS_PCI_TIMEOUT to see if any error occured while reading from a device. RESPONSE_IS_PCI_ERROR stills gives an idea to the readers that some PCI error occured. It was my understanding that timeout is also a kind of PCI error (I might be horribly wrong here, given my very less experience with PCI subsystem) so it would be okay to use RESPONSE_IS_PCI_ERROR here. If that is not the case please let me know. But I am not sure what to do here? If RESPONSE_IS_PCI_ERROR does not fit here, should the right option would be to revert/remove this patch from the series? Thanks, Naveen