| Message ID | 2bc987dc1dfb753c37a65b6c8c98c32e66a4d2a0.1634306198.git.naveennaidu479@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Unify PCI error response checking | expand |
On 15/10, Naveen Naidu wrote: > An MMIO read from a PCI device that doesn't exist or doesn't respond > causes a PCI error. There's no real data to return to satisfy the > CPU read, so most hardware fabricates ~0 data. > > Use RESPONSE_IS_PCI_ERROR() to check the response we get when we read > data from hardware. > > This helps unify PCI error response checking and make error checks > consistent and easier to find. > > Compile tested only. > > Acked-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > --- > 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/ 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
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);