Fix wrong failure check on pcie_capability_read_*()
cover Fix wrong failure check on pcie_capability_read_*()
From: Bolarinwa Olayemi Saheed <[email protected]>
On failure, pcie_capabiility_read_*() will set the status value, its last parameter to 0 and not ~0.
This bug fix checks for the proper value.
Bolarinwa Olayemi Saheed (2):
PCI/PME: Fix wrong failure check on pcie_capability_read_*()
PCI: pciehp: Fix wrong failure check on pcie_capability_read_*()
drivers/pci/hotplug/pciehp_hpc.c | 10 +++++----- drivers/pci/pcie/pme.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)
Signature · 1 line
2.18.2
PATCH 1/2 PCI/PME: Fix wrong failure check on pcie_capability_read_*()
From: Bolarinwa Olayemi Saheed <[email protected]>
On failure, pcie_capabiility_read_*() will set rtsta to 0 and not ~0. This bug fix checks for the proper value.
--- 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 (rtsta == (u32 )0 )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 (rtsta == (u32 )0 || !(rtsta &PCI_EXP_RTSTA_PME )) {spin_unlock_irqrestore (&data->lock, flags);return IRQ_NONE ; }
Signature · 1 line
2.18.2
PATCH 2/2 PCI: pciehp: Fix wrong failure check on pcie_capability_read_*()
In reply to [email protected]From: Bolarinwa Olayemi Saheed <[email protected]>
On failure, pcie_capabiility_read_*() will set the status value, its last parameter to 0 and not ~0.
This bug fix checks for the proper value.
--- 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 (slot_status == (u16)0) { 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 (slot_ctrl == (u16 )0 ) {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 || lnk_status == (u16 )0 )return -ENODEV ; ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA );
@@ -440,7 +440,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 || slot_status == (u16 )0 )return -ENODEV ;return !!(slot_status &PCI_EXP_SLTSTA_PDS );
@@ -592,7 +592,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 (status == (u16 )0 ) {ctrl_info (ctrl,"%s: no response from device\n" , __func__);if (parent)pm_runtime_put (parent);
Signature · 1 line
2.18.2
Re: [PATCH 2/2] PCI: pciehp: Fix wrong failure check on pcie_capability_read_*()
Quoted 3 lines · On Fri, Jun 19, 2020 at 10:12:19PM +0200, [email protected] wrote:
On failure, pcie_capabiility_read_*() will set the status value, its last parameter to 0 and not ~0.
This bug fix checks for the proper value.
If a config space read times out, the PCIe controller fabricates an "all ones" response. The code is checking for such a timeout, not for an error. Hence the code is fine.
Thanks,
Lukas
Quoted 57 lines
Signed-off-by Bolarinwa Olayemi Saheed <[email protected]>--- 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 (slot_status == (u16)0) { 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 (slot_ctrl == (u16 )0 ) {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 || lnk_status == (u16 )0 )return -ENODEV ; ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA );@@ -440,7 +440,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 || slot_status == (u16 )0 )return -ENODEV ;return !!(slot_status &PCI_EXP_SLTSTA_PDS );@@ -592,7 +592,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 (status == (u16 )0 ) {ctrl_info (ctrl,"%s: no response from device\n" , __func__);if (parent)pm_runtime_put (parent);Signature · 1 line
2.18.2
Re: [PATCH 2/2] PCI: pciehp: Fix wrong failure check on pcie_capability_read_*()
Quoted 8 lines · Lukas Wunner
Quoted 3 lines · On Fri, Jun 19, 2020 at 10:12:19PM +0200, [email protected] wrote:
On failure, pcie_capabiility_read_*() will set the status value, its last parameter to 0 and not ~0.
This bug fix checks for the proper value.If a config space read times out, the PCIe controller fabricates an "all ones" response. The code is checking for such a timeout, not for an error. Hence the code is fine.
In the typical case, the pci_read_config_word() done by pcie_capability_read_word() will not return an error, so if the read times out, we should see slot_status == ~0.
But if it's possible to set dev->error_state == pci_channel_io_perm_failure, pci_read_config_word() will return an error because pci_dev_is_disconnected(), so slot_status would be 0.
There are a dozen or so places that set dev->error_state. It doesn't look *likely* that any of them would cause this, but it doesn't instill confidence.
It would be a lot nicer if we didn't have to worry about both the 0 and ~0 cases. I keep coming back to the idea of removing the "*val = 0" code from pcie_capability_read_word() so we wouldn't have that special case.
In any case, this particular patch doesn't seem like quite the right fix, so I'll drop it.
Quoted 56 lines
Quoted 56 lines
Signed-off-by Bolarinwa Olayemi Saheed <[email protected]>--- 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 (slot_status == (u16)0) { 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 (slot_ctrl == (u16 )0 ) {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 || lnk_status == (u16 )0 )return -ENODEV ; ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA );@@ -440,7 +440,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 || slot_status == (u16 )0 )return -ENODEV ;return !!(slot_status &PCI_EXP_SLTSTA_PDS );@@ -592,7 +592,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 (status == (u16 )0 ) {ctrl_info (ctrl,"%s: no response from device\n" , __func__);if (parent)pm_runtime_put (parent);Signature · 1 line
2.18.2