[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20211021152253.pqc6xp3vnv5fpczj@theprophet>
Date: Thu, 21 Oct 2021 20:52:53 +0530
From: Naveen Naidu <naveennaidu479@...il.com>
To: bhelgaas@...gle.com
Cc: linux-kernel-mentees@...ts.linuxfoundation.org,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
skhan@...uxfoundation.org, Lukas Wunner <lukas@...ner.de>,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
Amey Narkhede <ameynarkhede03@...il.com>
Subject: Re: [PATCH v3 18/25] PCI: pciehp: Use RESPONSE_IS_PCI_ERROR() to
check read from hardware
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@...il.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
Powered by blists - more mailing lists