PCI: Remove '*val = 0' from pcie_capability_read_*()
Subsystems (6)
v3 cover PCI: Remove '*val = 0' from pcie_capability_read_*()
From: Bolarinwa Olayemi Saheed <[email protected]>
v3 CHANGES:
- Split previous PATCH 6/13 into two : PATCH 6/14 and PATCH 7/14
- Fix commit message of PATCH 5/14
- Update Patch numbering and Commit messages
- Add 'Acked by Greg KH' to PATCH 2/14
- Add PATCH version
v2 CHANGES:
- Fix missing comma, causing the email cc error
- Fix typos and numbering errors in commit messages
- Add commit message to 13/13
- Add two more patches: PATCH 3/13 and PATCH 4/13
MERGING:
Patch 7/14 depends on Patch 6/14. However Patch 6/14 has no dependency. Please, merge PATCH 7/14 only after Patch 6/14.
Patch 14/14 depend on all preceeding patchs. Except for Patch 6/14 and Patch 7/14, all other patches are independent of one another. Hence, please merge Patch 14/14 only after other patches in this series have been merged.
PATCH 6/14:
Make the function set status to "Power On" by default and only set to Set "Power Off" only if pcie_capability_read_word() is successful and (slot_ctrl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF.
PATCH 1/14 to 13/14:
Check the return value of pcie_capability_read_*() to ensure success or confirm failure. While maintaining these functions, this ensures that the changes in PATCH 14/14 does not introduce any bug.
PATCH 14/14:
There are several reasons why a PCI capability read may fail whether the device is present or not. If this happens, pcie_capability_read_*() will return -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER or PCIBIOS_DEVICE_NOT_FOUND and *val is set to 0.
This behaviour if further ensured by this code inside pcie_capability_read_*()
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val); /*
* Reset *val to 0 if pci_read_config_dword() fails, it may
* have been written as 0xFFFFFFFF if hardware error happens
* during pci_read_config_dword().
*/
if (ret)
*val = 0;
return ret;
a) Since all pci_generic_config_read() does is read a register value, it may return success after reading a ~0 which *may* have been fabricated by the PCI host bridge due to a read timeout. Hence pci_read_config_*() will return success with a fabricated ~0 in *val, indicating a problem. In this case, the assumed behaviour of pcie_capability_read_*() will be wrong. To avoid error slipping through, more checks are necessary.
b) pci_read_config_*() will return PCIBIOS_DEVICE_NOT_FOUND only if dev->error_state = pci_channel_io_perm_failure (i.e. pci_dev_is_disconnected()) or if pci_generic_config_read() can't find the device. In both cases *val is initially set to ~0 but as shown in the code above pcie_capability_read_*() resets it back to 0. Even with this effort, drivers still have to perform validation checks more so if 0 is a valid value.
Most drivers only consider the case (b) and in some cases, there is the expectation that on timeout *val has a fabricated value of ~0, which *may* not always be true as explained in (a).
In any case, checks need to be done to validate the value read and maybe confirm which error has occurred. It is better left to the drivers to do.
Check the return value of pcie_capability_read_dword() to ensure success and avoid bug as a result of Patch 14/14.
Remove the reset of *val to 0 when pci_read_config_*() fails.
Bolarinwa Olayemi Saheed (14):
IB/hfi1: Check the return value of pcie_capability_read_*()
misc: rtsx: Check the return value of pcie_capability_read_*()
ath9k: Check the return value of pcie_capability_read_*()
iwlegacy: Check the return value of pcie_capability_read_*()
PCI: pciehp: Check the return value of pcie_capability_read_*()
PCI: pciehp: Make "Power On" the default
PCI: pciehp: Check the return value of pcie_capability_read_*() PCI/ACPI: Check the return value of pcie_capability_read_*()
PCI: pciehp: Check the return value of pcie_capability_read_*()
PCI: Check the return value of pcie_capability_read_*() PCI/PM: Check return value of pcie_capability_read_*() PCI/AER: Check the return value of pcie_capability_read_*() PCI/ASPM: Check the return value of pcie_capability_read_*()
PCI: Remove '*val = 0' from pcie_capability_read_*()
drivers/net/wireless/ath/ath9k/pci.c | 5 +++-- drivers/net/wireless/intel/iwlegacy/common.c | 4 ++-- drivers/infiniband/hw/hfi1/aspm.c | 7 ++++--- drivers/misc/cardreader/rts5227.c | 5 +++-- drivers/misc/cardreader/rts5249.c | 5 +++-- drivers/misc/cardreader/rts5260.c | 5 +++-- drivers/misc/cardreader/rts5261.c | 5 +++-- drivers/pci/pcie/aer.c | 5 +++-- drivers/pci/pcie/aspm.c | 33 +++++++++++++++++---------------- drivers/pci/hotplug/pciehp_hpc.c | 47 ++++++++++++++++---------------- drivers/pci/pci-acpi.c | 10 ++++--- drivers/pci/probe.c | 29 ++++++++++++-------- drivers/pci/access.c | 14 -------------- 13 files changed, 87 insertions(+), 87 deletions(-)
Signature · 1 line
2.18.2
PATCH v3 2/14 misc: rtsx: Check the return value of pcie_capability_read_*()
From: Bolarinwa Olayemi Saheed <[email protected]>
On failure pcie_capability_read_dword() sets it's last parameter, val to 0. In which case (val & PCI_EXP_DEVCTL2_LTR_EN) evaluates to 0. However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
This bug can be avoided without changing the function's behaviour if the return value of pcie_capability_read_word is checked to confirm success.
Check the return value of pcie_capability_read_word() to ensure success.
--- drivers/misc/cardreader/rts5227.c | 5 +++-- drivers/misc/cardreader/rts5249.c | 5 +++-- drivers/misc/cardreader/rts5260.c | 5 +++-- drivers/misc/cardreader/rts5261.c | 5 +++-- 4 files changed, 12 insertions(+), 8 deletions(-)
drivers/misc/cardreader/rts5227.c +3 -2
@@ -92,6 +92,7 @@ static void rts5227_force_power_down(struct rtsx_pcr *pcr, u8 pm_state)static int rts5227_extra_init_hw (struct rtsx_pcr *pcr) {u16 cap; +int ret;rtsx_pci_init_cmd (pcr);
@@ -105,8 +106,8 @@ static int rts5227_extra_init_hw(struct rtsx_pcr *pcr)/* LED shine disabled, set initial shine cycle period */ rtsx_pci_add_cmd (pcr,WRITE_REG_CMD ,OLT_LED_CTL ,0x0F ,0x02 );/* Configure LTR */ -pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &cap); -if (cap &PCI_EXP_DEVCTL2_LTR_EN ) + ret =pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &cap); +if (!ret && (cap &PCI_EXP_DEVCTL2_LTR_EN ))rtsx_pci_add_cmd (pcr,WRITE_REG_CMD ,LTR_CTL ,0xFF ,0xA3 );/* Configure OBFF */ rtsx_pci_add_cmd (pcr,WRITE_REG_CMD ,OBFF_CFG ,0x03 ,0x03 );
drivers/misc/cardreader/rts5249.c +3 -2
@@ -95,6 +95,7 @@ static void rts5249_init_from_cfg(struct rtsx_pcr *pcr) {struct rtsx_cr_option *option = &(pcr->option);u32 lval; +int ret;if (CHK_PCI_PID (pcr,PID_524A )) rtsx_pci_read_config_dword(pcr,
@@ -118,8 +119,8 @@ static void rts5249_init_from_cfg(struct rtsx_pcr *pcr)if (option->ltr_en) {u16 val; -pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &val); -if (val &PCI_EXP_DEVCTL2_LTR_EN ) { + ret =pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &val); +if (!ret && (val &PCI_EXP_DEVCTL2_LTR_EN )) { option->ltr_enabled = true; option->ltr_active = true;rtsx_set_ltr_latency (pcr, option->ltr_active_latency);
drivers/misc/cardreader/rts5260.c +3 -2
@@ -498,6 +498,7 @@ static void rts5260_init_from_cfg(struct rtsx_pcr *pcr) {struct rtsx_cr_option *option = &pcr->option;u32 lval; +int ret;rtsx_pci_read_config_dword (pcr,PCR_ASPM_SETTING_5260 , &lval);
@@ -518,8 +519,8 @@ static void rts5260_init_from_cfg(struct rtsx_pcr *pcr)if (option->ltr_en) {u16 val; -pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &val); -if (val &PCI_EXP_DEVCTL2_LTR_EN ) { + ret =pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &val); +if (!ret && (val &PCI_EXP_DEVCTL2_LTR_EN )) { option->ltr_enabled = true; option->ltr_active = true;rtsx_set_ltr_latency (pcr, option->ltr_active_latency);
drivers/misc/cardreader/rts5261.c +3 -2
@@ -438,9 +438,10 @@ static void rts5261_init_from_cfg(struct rtsx_pcr *pcr)rtsx_pci_write_register (pcr,ASPM_FORCE_CTL ,0xFF ,0 );if (option->ltr_en) {u16 val; +int ret; -pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &val); -if (val &PCI_EXP_DEVCTL2_LTR_EN ) { + ret =pcie_capability_read_word (pcr->pci,PCI_EXP_DEVCTL2 , &val); +if (!ret && (val &PCI_EXP_DEVCTL2_LTR_EN )) { option->ltr_enabled = true; option->ltr_active = true;rtsx_set_ltr_latency (pcr, option->ltr_active_latency);
Signature · 1 line
2.18.2
PATCH v3 5/14 PCI: pciehp: Check the return value of pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
On any failure pcie_capability_read_word() sets it's last parameter, *val to 0, this value may have be earlier set by pci_config_read_word() to ~0. So, any function which check only for a frabricated ~0 will fail in this case.
Checking for the return value of pcie_capability_read_dword() will help assert failure or success of this function. But more checks may be needed to assure the validity of the value.
Include a check on the return value of pcie_capability_read_word() to confirm success or failure.
--- drivers/pci/hotplug/pciehp_hpc.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +9 -7
@@ -86,10 +86,11 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout) {struct pci_dev *pdev =ctrl_dev (ctrl);u16 slot_status; +int ret; do { - pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); - if (slot_status == (u16) ~0) { + ret = pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); + if (ret || (slot_status == (u16) ~0)) { ctrl_info(ctrl, "%s: no response from device\n", __func__); return 0;
@@ -156,6 +157,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd, {struct pci_dev *pdev =ctrl_dev (ctrl);u16 slot_ctrl_orig, slot_ctrl; +int ret;mutex_lock (&ctrl->ctrl_lock);
@@ -164,8 +166,8 @@ 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 ) { + ret =pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl); +if (ret || (slot_ctrl == (u16 ) ~0 )) {ctrl_info (ctrl,"%s: no response from device\n" , __func__); goto out; }
@@ -430,7 +432,7 @@ void pciehp_get_latch_status(struct controller *ctrl, u8 *status)* removed immediately after the check so the caller may need to take * this into account. * -* It the hotplug controller itself is not available anymore returns +* If the hotplug controller itself is not available anymore returns * %-ENODEV. */ int pciehp_card_present (struct controller *ctrl)
@@ -591,8 +593,8 @@ 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 ) { + ret =pcie_capability_read_word (pdev,PCI_EXP_SLTSTA , &status); +if (ret || (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
PATCH v3 6/14 PCI: pciehp: Make "Power On" the default
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
The default case of the switch statement is redundant since PCI_EXP_SLTCTL_PCC is only a single bit. pcie_capability_read_word() currently causes "On" value to be set if it fails.
Make the switch-statement set status to "Power On" by default or when (slot_ctrl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON.
--- drivers/pci/hotplug/pciehp_hpc.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +5 -11
@@ -399,22 +399,16 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status) {struct pci_dev *pdev =ctrl_dev (ctrl);u16 slot_ctrl; +int ret; -pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl); + *status =1 ;/* On */ + ret =pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl);ctrl_dbg (ctrl,"%s: SLOTCTRL %x value read %x\n" , __func__,pci_pcie_cap (ctrl->pcie->port) +PCI_EXP_SLTCTL , slot_ctrl); -switch (slot_ctrl &PCI_EXP_SLTCTL_PCC ) { -case PCI_EXP_SLTCTL_PWR_ON : - *status =1 ;/* On */ -break ; -case PCI_EXP_SLTCTL_PWR_OFF : +if (!ret && + ((slot_ctrl &PCI_EXP_SLTCTL_PCC ) ==PCI_EXP_SLTCTL_PWR_OFF )) *status =0 ;/* Off */ -break ; -default : - *status =0xFF ; -break ; - } }void pciehp_get_latch_status (struct controller *ctrl,u8 *status)
Signature · 6 lines
2.18.2 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
PATCH v3 7/14 PCI: pciehp: Check the return value of pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
If pcie_capability_read_word() fail, slot_ctrl will be 0 and the switch expression evaluates to 0. So *status = 1 "ON". However, with Patch 14/14 it is possible that slot_ctrl is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x), so the switch expression evaluates to PCI_EXP_SLTCTL_PCC. This means that on failure *status = 1 "OFF", since PCI_EXP_SLTCTL_PCC = PCI_EXP_SLTCTL_PWR_OFF.
Use an if-statement and include a check on the return value of pcie_capability_read_word() to confirm success or failure.
--- drivers/pci/hotplug/pciehp_hpc.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +5 -9
@@ -394,20 +394,16 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status) {struct pci_dev *pdev =ctrl_dev (ctrl);u16 slot_ctrl; +int ret; -pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl); + *status =1 ;/* On */ + ret =pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl);ctrl_dbg (ctrl,"%s: SLOTCTRL %x value read %x\n" , __func__,pci_pcie_cap (ctrl->pcie->port) +PCI_EXP_SLTCTL , slot_ctrl); -switch (slot_ctrl &PCI_EXP_SLTCTL_PCC ) { -case PCI_EXP_SLTCTL_PWR_OFF : +if (!ret && + ((slot_ctrl &PCI_EXP_SLTCTL_PCC ) ==PCI_EXP_SLTCTL_PWR_OFF )) *status =0 ;/* Off */ -break ; -case PCI_EXP_SLTCTL_PWR_ON : -default : - *status =1 ;/* On */ -break ; - } }void pciehp_get_latch_status (struct controller *ctrl,u8 *status)
Signature · 6 lines
2.18.2 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
PATCH v3 9/14 PCI: pciehp: Check return value of pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
On failure pcie_capability_read_dword() sets it's last parameter, val to 0.
However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
This bug can be avoided if the return value of pcie_capability_read_word is checked to confirm success.
Check the return value of pcie_capability_read_dword() to ensure success. Return a value that indicate the result of pcie_capability_read_word().
--- drivers/pci/hotplug/pciehp_hpc.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +9 -6
@@ -277,10 +277,11 @@ static void pcie_wait_for_presence(struct pci_dev *pdev) {int timeout =1250 ;u16 slot_status; +int ret; do { - pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); - if (slot_status & PCI_EXP_SLTSTA_PDS) + ret = pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); + if (!ret && (slot_status & PCI_EXP_SLTSTA_PDS)) return; msleep(10); timeout -= 10;
@@ -354,12 +355,13 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *hotplug_slot,struct controller *ctrl =to_ctrl (hotplug_slot);struct pci_dev *pdev =ctrl_dev (ctrl);u16 slot_ctrl; +int ret;pci_config_pm_runtime_get (pdev); -pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl); + ret =pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl);pci_config_pm_runtime_put (pdev); *status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC |PCI_EXP_SLTCTL_PIC )) >>6 ; -return 0 ; +return pcibios_err_to_errno (ret); }int pciehp_get_attention_status (struct hotplug_slot *hotplug_slot,u8 *status)
@@ -367,9 +369,10 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status)struct controller *ctrl =to_ctrl (hotplug_slot);struct pci_dev *pdev =ctrl_dev (ctrl);u16 slot_ctrl; +int ret;pci_config_pm_runtime_get (pdev); -pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl); + ret =pcie_capability_read_word (pdev,PCI_EXP_SLTCTL , &slot_ctrl);pci_config_pm_runtime_put (pdev);ctrl_dbg (ctrl,"%s: SLOTCTRL %x, value read %x\n" , __func__,pci_pcie_cap (ctrl->pcie->port) +PCI_EXP_SLTCTL , slot_ctrl);
@@ -389,7 +392,7 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status)break ; } -return 0 ; +return pcibios_err_to_errno (ret); }void pciehp_get_power_status (struct controller *ctrl,u8 *status)
Signature · 6 lines
2.18.2 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
PATCH v3 10/14 PCI: Check return value of pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
On failure pcie_capability_read_dword() sets it's last parameter, val to 0.
However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
This bug can be avoided if the return value of pcie_capability_read_word is checked to confirm success.
Check the return value of pcie_capability_read_word() to ensure success.
--- drivers/pci/probe.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
drivers/pci/probe.c +17 -12
@@ -1121,10 +1121,11 @@ EXPORT_SYMBOL(pci_add_new_bus);static void pci_enable_crs (struct pci_dev *pdev) {u16 root_cap =0 ; +int ret;/* Enable CRS Software Visibility if supported */ -pcie_capability_read_word (pdev,PCI_EXP_RTCAP , &root_cap); -if (root_cap &PCI_EXP_RTCAP_CRSVIS ) + ret =pcie_capability_read_word (pdev,PCI_EXP_RTCAP , &root_cap); +if (!ret && (root_cap &PCI_EXP_RTCAP_CRSVIS ))pcie_capability_set_word (pdev,PCI_EXP_RTCTL ,PCI_EXP_RTCTL_CRSSVE ); }
@@ -1519,9 +1520,10 @@ void set_pcie_port_type(struct pci_dev *pdev)void set_pcie_hotplug_bridge (struct pci_dev *pdev) {u32 reg32; +int ret; -pcie_capability_read_dword (pdev,PCI_EXP_SLTCAP , ®32); -if (reg32 &PCI_EXP_SLTCAP_HPC ) + ret =pcie_capability_read_dword (pdev,PCI_EXP_SLTCAP , ®32); +if (!ret && (reg32 &PCI_EXP_SLTCAP_HPC )) pdev->is_hotplug_bridge =1 ; }
@@ -2057,10 +2059,11 @@ int pci_configure_extended_tags(struct pci_dev *dev, void *ign)bool pcie_relaxed_ordering_enabled (struct pci_dev *dev) {u16 v; +int ret; -pcie_capability_read_word (dev,PCI_EXP_DEVCTL , &v); + ret =pcie_capability_read_word (dev,PCI_EXP_DEVCTL , &v); -return !!(v &PCI_EXP_DEVCTL_RELAX_EN ); +return (!ret && !!(v &PCI_EXP_DEVCTL_RELAX_EN )); }EXPORT_SYMBOL (pcie_relaxed_ordering_enabled);
@@ -2096,16 +2099,17 @@ static void pci_configure_ltr(struct pci_dev *dev)struct pci_host_bridge *host =pci_find_host_bridge (dev->bus);struct pci_dev *bridge;u32 cap, ctl; +int ret;if (!pci_is_pcie (dev))return ; -pcie_capability_read_dword (dev,PCI_EXP_DEVCAP2 , &cap); -if (!(cap &PCI_EXP_DEVCAP2_LTR )) + ret =pcie_capability_read_dword (dev,PCI_EXP_DEVCAP2 , &cap); +if (ret || !(cap &PCI_EXP_DEVCAP2_LTR ))return ; -pcie_capability_read_dword (dev,PCI_EXP_DEVCTL2 , &ctl); -if (ctl &PCI_EXP_DEVCTL2_LTR_EN ) { + ret =pcie_capability_read_dword (dev,PCI_EXP_DEVCTL2 , &ctl); +if (!ret && (ctl &PCI_EXP_DEVCTL2_LTR_EN )) {if (pci_pcie_type (dev) ==PCI_EXP_TYPE_ROOT_PORT ) { dev->ltr_path =1 ;return ;
@@ -2142,12 +2146,13 @@ static void pci_configure_eetlp_prefix(struct pci_dev *dev)struct pci_dev *bridge;int pcie_type;u32 cap; +int ret;if (!pci_is_pcie (dev))return ; -pcie_capability_read_dword (dev,PCI_EXP_DEVCAP2 , &cap); -if (!(cap &PCI_EXP_DEVCAP2_EE_PREFIX )) + ret =pcie_capability_read_dword (dev,PCI_EXP_DEVCAP2 , &cap); +if (ret || !(cap &PCI_EXP_DEVCAP2_EE_PREFIX ))return ; pcie_type =pci_pcie_type (dev);
Signature · 1 line
2.18.2
PATCH v3 11/14 PCI/PM: Check return value of pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
On failure pcie_capability_read_dword() sets it's last parameter, val to 0.
However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
This bug can be avoided if the return value of pcie_capability_read_dword is checked to confirm success.
Check the return value of pcie_capability_read_dword() to ensure success.
--- drivers/pci/pci.c | 52 ++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 19 deletions(-)
drivers/pci/pci.c +33 -19
@@ -3207,6 +3207,7 @@ void pci_configure_ari(struct pci_dev *dev) {u32 cap;struct pci_dev *bridge; +int ret;if (pcie_ari_disabled || !pci_is_pcie (dev) || dev->devfn)return ;
@@ -3215,8 +3216,8 @@ void pci_configure_ari(struct pci_dev *dev)if (!bridge)return ; -pcie_capability_read_dword (bridge,PCI_EXP_DEVCAP2 , &cap); -if (!(cap &PCI_EXP_DEVCAP2_ARI )) + ret =pcie_capability_read_dword (bridge,PCI_EXP_DEVCAP2 , &cap); +if (ret || !(cap &PCI_EXP_DEVCAP2_ARI ))return ;if (pci_find_ext_capability (dev,PCI_EXT_CAP_ID_ARI )) {
@@ -3606,6 +3607,7 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)struct pci_bus *bus = dev->bus;struct pci_dev *bridge;u32 cap, ctl2; +int ret;if (!pci_is_pcie (dev))return -EINVAL ;
@@ -3629,28 +3631,29 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)while (bus->parent) { bridge = bus->self; -pcie_capability_read_dword (bridge,PCI_EXP_DEVCAP2 , &cap); + ret =pcie_capability_read_dword (bridge,PCI_EXP_DEVCAP2 , + &cap);switch (pci_pcie_type (bridge)) {/* Ensure switch ports support AtomicOp routing */ case PCI_EXP_TYPE_UPSTREAM :case PCI_EXP_TYPE_DOWNSTREAM : -if (!(cap &PCI_EXP_DEVCAP2_ATOMIC_ROUTE )) +if (ret || !(cap &PCI_EXP_DEVCAP2_ATOMIC_ROUTE ))return -EINVAL ;break ;/* Ensure root port supports all the sizes we care about */ case PCI_EXP_TYPE_ROOT_PORT : -if ((cap & cap_mask) != cap_mask) +if (ret || ((cap & cap_mask) != cap_mask))return -EINVAL ;break ; }/* Ensure upstream ports don't block AtomicOps on egress */ if (pci_pcie_type (bridge) ==PCI_EXP_TYPE_UPSTREAM ) { -pcie_capability_read_dword (bridge,PCI_EXP_DEVCTL2 , - &ctl2); -if (ctl2 &PCI_EXP_DEVCTL2_ATOMIC_EGRESS_BLOCK ) + ret =pcie_capability_read_dword (bridge, +PCI_EXP_DEVCTL2 , &ctl2); + if (!ret && (ctl2 & PCI_EXP_DEVCTL2_ATOMIC_EGRESS_BLOCK)return -EINVAL ; }
@@ -4507,12 +4510,13 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);bool pcie_has_flr (struct pci_dev *dev) {u32 cap; +int ret;if (dev->dev_flags &PCI_DEV_FLAGS_NO_FLR_RESET )return false; -pcie_capability_read_dword (dev,PCI_EXP_DEVCAP , &cap); -return cap &PCI_EXP_DEVCAP_FLR ; + ret =pcie_capability_read_dword (dev,PCI_EXP_DEVCAP , &cap); +return (!ret && !!(cap &PCI_EXP_DEVCAP_FLR )); }EXPORT_SYMBOL_GPL (pcie_has_flr);
@@ -4672,7 +4676,7 @@ static bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active,msleep (20 );for (;;) {pcie_capability_read_word (pdev,PCI_EXP_LNKSTA , &lnk_status); - ret = !!(lnk_status &PCI_EXP_LNKSTA_DLLLA ); + ret = !!(!ret && (lnk_status &PCI_EXP_LNKSTA_DLLLA ));if (ret == active)break ;if (timeout <=0 )
@@ -5774,6 +5778,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,enum pci_bus_speed next_speed;enum pcie_link_width next_width;u32 bw, next_bw; +int ret;if (speed) *speed =PCI_SPEED_UNKNOWN ;
@@ -5783,7 +5788,12 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev, bw =0 ;while (dev) { -pcie_capability_read_word (dev,PCI_EXP_LNKSTA , &lnksta); + ret =pcie_capability_read_word (dev,PCI_EXP_LNKSTA , &lnksta); + +if (ret) { + dev =pci_upstream_bridge (dev); +continue ; + } next_speed = pcie_link_speed[lnksta &PCI_EXP_LNKSTA_CLS ]; next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
@@ -5820,6 +5830,7 @@ EXPORT_SYMBOL(pcie_bandwidth_available);enum pci_bus_speed pcie_get_speed_cap (struct pci_dev *dev) {u32 lnkcap2, lnkcap; +int ret;/* * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18. The
@@ -5830,16 +5841,18 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)* should use the Supported Link Speeds field in Link Capabilities, * where only 2.5 GT/s and 5.0 GT/s speeds were defined. */ -pcie_capability_read_dword (dev,PCI_EXP_LNKCAP2 , &lnkcap2); + ret =pcie_capability_read_dword (dev,PCI_EXP_LNKCAP2 , &lnkcap2);/* PCIe r3.0-compliant */ -if (lnkcap2) +if (!ret && lnkcap2)return PCIE_LNKCAP2_SLS2SPEED (lnkcap2); -pcie_capability_read_dword (dev,PCI_EXP_LNKCAP , &lnkcap); -if ((lnkcap &PCI_EXP_LNKCAP_SLS ) ==PCI_EXP_LNKCAP_SLS_5_0GB ) + ret =pcie_capability_read_dword (dev,PCI_EXP_LNKCAP , &lnkcap); +if (!ret && + ((lnkcap &PCI_EXP_LNKCAP_SLS ) ==PCI_EXP_LNKCAP_SLS_5_0GB ))return PCIE_SPEED_5_0GT ; -else if ((lnkcap &PCI_EXP_LNKCAP_SLS ) ==PCI_EXP_LNKCAP_SLS_2_5GB ) +else if (!ret && + ((lnkcap &PCI_EXP_LNKCAP_SLS ) ==PCI_EXP_LNKCAP_SLS_2_5GB ))return PCIE_SPEED_2_5GT ;return PCI_SPEED_UNKNOWN ;
@@ -5856,9 +5869,10 @@ EXPORT_SYMBOL(pcie_get_speed_cap);enum pcie_link_width pcie_get_width_cap (struct pci_dev *dev) {u32 lnkcap; +int ret; -pcie_capability_read_dword (dev,PCI_EXP_LNKCAP , &lnkcap); -if (lnkcap) + ret =pcie_capability_read_dword (dev,PCI_EXP_LNKCAP , &lnkcap); +if (!ret && lnkcap)return (lnkcap &PCI_EXP_LNKCAP_MLW ) >>4 ;return PCIE_LNK_WIDTH_UNKNOWN ;
Signature · 6 lines
2.18.2 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
RE: [PATCH 11/14 v3] PCI/PM: Check return value of pcie_capability_read_*()
Quoted 34 lines
From: Saheed Olayemi Bolarinwa
Sent: 10 July 2020 22:20
To: [email protected]
From: Bolarinwa Olayemi Saheed <[email protected]>On failure
pcie_capability_read_dword()sets it's last parameter, val to 0.
However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).This bug can be avoided if the return value of
pcie_capability_read_dwordis checked to confirm success.Check the return value of
pcie_capability_read_dword()to ensure success.Suggested-by Bjorn Helgaas <[email protected]>Signed-off-by Bolarinwa Olayemi Saheed <[email protected]>--- drivers/pci/pci.c | 52 ++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 19 deletions(-)drivers/pci/pci.c +1
-0@@ -3207,6 +3207,7 @@ void pci_configure_ari(struct pci_dev *dev) {u32 cap;struct pci_dev *bridge; +int ret;if (pcie_ari_disabled || !pci_is_pcie (dev) || dev->devfn)return ;
drivers/pci/pci.c void pci_configure_ari(struct pci_dev *dev) · line 3221
Quoted 8 lines
drivers/pci/pci.c +2
-2@@ -3215,8 +3216,8 @@ void pci_configure_ari(struct pci_dev *dev)if (!bridge)return ; -pcie_capability_read_dword (bridge,PCI_EXP_DEVCAP2 , &cap); -if (!(cap &PCI_EXP_DEVCAP2_ARI )) + ret =pcie_capability_read_dword (bridge,PCI_EXP_DEVCAP2 , &cap); +if (ret || !(cap &PCI_EXP_DEVCAP2_ARI ))return ;
Why not make the function result 64bit?
Then you can return ~0ull on failure and the capability value on success. Gets rid of the horrid error + return value pair.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
PATCH v3 13/14 PCI/ASPM: Check the return value of pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
On failure pcie_capability_read_dword() sets it's last parameter, val to 0.
However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
This bug can be avoided if the return value of pcie_capability_read_dword is checked to confirm success.
Check the return value of pcie_capability_read_dword() to ensure success.
--- drivers/pci/pcie/aspm.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
drivers/pci/pcie/aspm.c +17 -16
@@ -176,7 +176,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)static void pcie_clkpm_cap_init (struct pcie_link_state *link,int blacklist) { -int capable =1 , enabled =1 ; +int ret, capable =1 , enabled =1 ;u32 reg32;u16 reg16;struct pci_dev *child;
@@ -184,14 +184,14 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)/* All functions should have the same cap and state, take the worst */ list_for_each_entry (child, &linkbus->devices, bus_list) { -pcie_capability_read_dword (child,PCI_EXP_LNKCAP , ®32); -if (!(reg32 &PCI_EXP_LNKCAP_CLKPM )) { + ret =pcie_capability_read_dword (child,PCI_EXP_LNKCAP , ®32); +if (ret || !(reg32 &PCI_EXP_LNKCAP_CLKPM )) { capable =0 ; enabled =0 ;break ; } -pcie_capability_read_word (child,PCI_EXP_LNKCTL , ®16); -if (!(reg16 &PCI_EXP_LNKCTL_CLKREQ_EN )) + ret =pcie_capability_read_word (child,PCI_EXP_LNKCTL , ®16); +if (ret || !(reg16 &PCI_EXP_LNKCTL_CLKREQ_EN )) enabled =0 ; } link->clkpm_enabled = enabled;
@@ -205,6 +205,7 @@ static bool pcie_retrain_link(struct pcie_link_state *link)struct pci_dev *parent = link->pdev;unsigned long end_jiffies;u16 reg16; +int ret;pcie_capability_read_word (parent,PCI_EXP_LNKCTL , ®16); reg16 |=PCI_EXP_LNKCTL_RL ;
@@ -222,8 +223,8 @@ static bool pcie_retrain_link(struct pcie_link_state *link)/* Wait for link training end. Break out after waiting for timeout */ end_jiffies = jiffies +LINK_RETRAIN_TIMEOUT ;do { -pcie_capability_read_word (parent,PCI_EXP_LNKSTA , ®16); -if (!(reg16 &PCI_EXP_LNKSTA_LT )) + ret =pcie_capability_read_word (parent,PCI_EXP_LNKSTA , ®16); +if (ret || !(reg16 &PCI_EXP_LNKSTA_LT ))break ;msleep (1 ); }while (time_before (jiffies, end_jiffies));
@@ -237,7 +238,7 @@ static bool pcie_retrain_link(struct pcie_link_state *link)*/ static void pcie_aspm_configure_common_clock (struct pcie_link_state *link) { -int same_clock =1 ; +int ret, same_clock =1 ;u16 reg16, parent_reg, child_reg[8 ];struct pci_dev *child, *parent = link->pdev;struct pci_bus *linkbus = parent->subordinate;
@@ -249,24 +250,24 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)BUG_ON (!pci_is_pcie (child));/* Check downstream component if bit Slot Clock Configuration is 1 */ -pcie_capability_read_word (child,PCI_EXP_LNKSTA , ®16); -if (!(reg16 &PCI_EXP_LNKSTA_SLC )) + ret =pcie_capability_read_word (child,PCI_EXP_LNKSTA , ®16); +if (ret || !(reg16 &PCI_EXP_LNKSTA_SLC )) same_clock =0 ;/* Check upstream component if bit Slot Clock Configuration is 1 */ -pcie_capability_read_word (parent,PCI_EXP_LNKSTA , ®16); -if (!(reg16 &PCI_EXP_LNKSTA_SLC )) + ret =pcie_capability_read_word (parent,PCI_EXP_LNKSTA , ®16); +if (ret || !(reg16 &PCI_EXP_LNKSTA_SLC )) same_clock =0 ;/* Port might be already in common clock mode */ -pcie_capability_read_word (parent,PCI_EXP_LNKCTL , ®16); -if (same_clock && (reg16 &PCI_EXP_LNKCTL_CCC )) { + ret =pcie_capability_read_word (parent,PCI_EXP_LNKCTL , ®16); +if (!ret && same_clock && (reg16 &PCI_EXP_LNKCTL_CCC )) {bool consistent = true;list_for_each_entry (child, &linkbus->devices, bus_list) { -pcie_capability_read_word (child,PCI_EXP_LNKCTL , + ret =pcie_capability_read_word (child,PCI_EXP_LNKCTL , ®16); -if (!(reg16 &PCI_EXP_LNKCTL_CCC )) { +if (ret || !(reg16 &PCI_EXP_LNKCTL_CCC )) { consistent = false;break ; }
Signature · 6 lines
2.18.2 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
PATCH v3 14/14 PCI: Remove '*val = 0' from pcie_capability_read_*()
In reply to Saheed Olayemi BolarinwaFrom: Bolarinwa Olayemi Saheed <[email protected]>
There are several reasons why a PCI capability read may fail whether the device is present or not. If this happens, pcie_capability_read_*() will return -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER or PCIBIOS_DEVICE_NOT_FOUND and *val is set to 0.
This behaviour if further ensured by this code inside pcie_capability_read_*()
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val); /*
* Reset *val to 0 if pci_read_config_dword() fails, it may
* have been written as 0xFFFFFFFF if hardware error happens
* during pci_read_config_dword().
*/
if (ret)
*val = 0;
return ret;
a) Since all pci_generic_config_read() does is read a register value, it may return success after reading a ~0 which *may* have been fabricated by the PCI host bridge due to a read timeout. Hence pci_read_config_*() will return success with a fabricated ~0 in *val, indicating a problem. In this case, the assumed behaviour of pcie_capability_read_*() will be wrong. To avoid error slipping through, more checks are necessary.
b) pci_read_config_*() will return PCIBIOS_DEVICE_NOT_FOUND only if dev->error_state = pci_channel_io_perm_failure (i.e. pci_dev_is_disconnected()) or if pci_generic_config_read() can't find the device. In both cases *val is initially set to ~0 but as shown in the code above pcie_capability_read_*() resets it back to 0. Even with this effort, drivers still have to perform validation checks more so if 0 is a valid value.
Most drivers only consider the case (b) and in some cases, there is the expectation that on timeout *val has a fabricated value of ~0, which *may* not always be true as explained in (a).
In any case, checks need to be done to validate the value read and maybe confirm which error has occurred. It is better left to the drivers to do.
Remove the reset of *val to 0 when pci_read_config_*() fails.
---
This patch depends on all of the preceeding patches in this series, otherwise it will introduce bugs as pointed out in the commit message of each.
drivers/pci/access.c | 14 -------------- 1 file changed, 14 deletions(-)
drivers/pci/access.c +0 -14
@@ -413,13 +413,6 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)if (pcie_capability_reg_implemented (dev, pos)) { ret =pci_read_config_word (dev,pci_pcie_cap (dev) + pos, val); -/* -* Reset *val to 0 if pci_read_config_word() fails, it may -* have been written as 0xFFFF if hardware error happens -* during pci_read_config_word(). -*/ -if (ret) - *val =0 ;return ret; }
@@ -448,13 +441,6 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)if (pcie_capability_reg_implemented (dev, pos)) { ret =pci_read_config_dword (dev,pci_pcie_cap (dev) + pos, val); -/* -* Reset *val to 0 if pci_read_config_dword() fails, it may -* have been written as 0xFFFFFFFF if hardware error happens -* during pci_read_config_dword(). -*/ -if (ret) - *val =0 ;return ret; }
Signature · 1 line
2.18.2
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/14 v3] iwlegacy: Check the return value of pcie_capability_read_*()
+ linux-wireless
Quoted 15 lines · Saheed Olayemi Bolarinwa
From: Bolarinwa Olayemi Saheed <[email protected]>
Cc: [email protected],
[email protected], [email protected], [email protected], [email protected], Stanislaw Gruszka <[email protected]>, [email protected], [email protected]On failure
pcie_capability_read_dword()sets it's last parameter, val to 0. However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
Quoted 7 lines
This bug can be avoided without changing the function's behaviour if the return value of
pcie_capability_read_dwordis checked to confirm success.Check the return value of
pcie_capability_read_dword()to ensure success.Suggested-by Bjorn Helgaas <[email protected]>Signed-off-by Bolarinwa Olayemi Saheed <[email protected]>
Please check your setup, linux-wireless was not in Cc field.
Signature · 1 line
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 4/14 v3] iwlegacy: Check the return value of pcie_capability_read_*()
Fixed
Quoted 19 lines · Kalle Valo
+ linux-wireless
Quoted 15 lines · Saheed Olayemi Bolarinwa
From: Bolarinwa Olayemi Saheed <[email protected]>
Cc: [email protected],
[email protected], [email protected], [email protected], [email protected], Stanislaw Gruszka <[email protected]>, [email protected], [email protected]On failure
pcie_capability_read_dword()sets it's last parameter, val to 0. However, with Patch 14/14, it is possible that val is set to ~0 on failure. This would introduce a bug because (x & x) == (~0 & x).
Quoted 9 lines
Quoted 7 lines
This bug can be avoided without changing the function's behaviour if the return value of
pcie_capability_read_dwordis checked to confirm success.Check the return value of
pcie_capability_read_dword()to ensure success.Suggested-by Bjorn Helgaas <[email protected]>Signed-off-by Bolarinwa Olayemi Saheed <[email protected]>Please check your setup, linux-wireless was not in Cc field.
Thank you for letting me know. I have fixed it and resent to the missing addresses.