| Message ID | c21290fe02a7a342a8b93c692586b6a2b6cde9e0.1634825082.git.naveennaidu479@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Unify PCI error response checking | expand |
On 21/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. > > 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..f472f83f6cce 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 not added your Acked-by tag from the v1 [1] of the patch series, since the RESPONSE_IS_PCI_ERROR macro definition slightly changed. I hope this was the right thing to do. [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
On Thu, Oct 21, 2021 at 08:52:53PM +0530, Naveen Naidu wrote: > Lukas, I have not added your Acked-by tag from the v1 [1] of the patch > series, since the RESPONSE_IS_PCI_ERROR macro definition slightly > changed. I hope this was the right thing to do. [...] > 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? My Acked-by still stands. As for the macro name, I'm fine with whatever Bjorn and the community settle on. :) Thanks, Lukas
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 3024d7e85e6a..f472f83f6cce 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);
An MMIO read from a PCI device that doesn't exist or doesn't respond causes a PCI error. There's no real data to return to satisfy the CPU read, so most hardware fabricates ~0 data. Use RESPONSE_IS_PCI_ERROR() to check the response we get when we read data from hardware. This helps unify PCI error response checking and make error checks consistent and easier to find. Compile tested only. Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> --- drivers/pci/hotplug/pciehp_hpc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)