LinuxLists.cc - [PATCH 0/13] PCI: Remove '*val = 0' from pcie_capability_read

2020-07-07 23:04:20

Subject: [PATCH 0/13] PCI: Remove 'val = 0' from pcie_capability_read_()

From: Bolarinwa Olayemi Saheed <[email protected]>

MERGING:
Only Patch 13/13 depend on all preceeding patchs. All other patches are
independent of one another. Hence, please merge PATCH 13/13 only after
other patches in this series have been merged.

PATCH 6/13:
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/13 to 12/13:
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 13/13 does not introduce any bug.

PATCH 13/13:
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 13/13.
Remove the reset of *val to 0 when pci_read_config_*() fails.

Bolarinwa Olayemi Saheed (13):
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/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(-)

--
2.18.2

2020-07-07 23:04:23

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 2/13] 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 13/13, 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.

Suggested-by: Bjorn Helgaas <[email protected]>
Signed-off-by: Bolarinwa Olayemi Saheed <[email protected]>

---
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(-)

diff --git a/drivers/misc/cardreader/rts5227.c b/drivers/misc/cardreader/rts5227.c
index 3a9467aaa435..7a20a4898d07 100644
--- a/drivers/misc/cardreader/rts5227.c
+++ b/drivers/misc/cardreader/rts5227.c
@@ -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);
diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c
index 6c6c9e95a29f..2b05e8663431 100644
--- a/drivers/misc/cardreader/rts5249.c
+++ b/drivers/misc/cardreader/rts5249.c
@@ -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);
diff --git a/drivers/misc/cardreader/rts5260.c b/drivers/misc/cardreader/rts5260.c
index 7a9dbb778e84..934aeaeebfaf 100644
--- a/drivers/misc/cardreader/rts5260.c
+++ b/drivers/misc/cardreader/rts5260.c
@@ -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);
diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
index 195822ec858e..2b6f61696e19 100644
--- a/drivers/misc/cardreader/rts5261.c
+++ b/drivers/misc/cardreader/rts5261.c
@@ -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);
--
2.18.2

2020-07-07 23:04:34

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 8/13] PCI: pciehp: Check return value of pcie_capability_read_*()

2020-07-07 23:04:36

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 10/13] PCI/PM: Check 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.
However, with Patch 13/13, 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.

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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b..9f18ffbf7bd4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
--
2.18.2

2020-07-07 23:04:39

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 12/13] PCI/ASPM: 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.
However, with Patch 13/13, 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.

Suggested-by: Bjorn Helgaas <[email protected]>
Signed-off-by: Bolarinwa Olayemi Saheed <[email protected]>
---
drivers/pci/pcie/aspm.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index b17e5ffd31b1..32aa9d57672a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -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, &reg32);
- if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+ ret = pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
+ if (ret || !(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
capable = 0;
enabled = 0;
break;
}
- pcie_capability_read_word(child, PCI_EXP_LNKCTL, &reg16);
- if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
+ ret = pcie_capability_read_word(child, PCI_EXP_LNKCTL, &reg16);
+ 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, &reg16);
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, &reg16);
- if (!(reg16 & PCI_EXP_LNKSTA_LT))
+ ret = pcie_capability_read_word(parent, PCI_EXP_LNKSTA, &reg16);
+ 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, &reg16);
- if (!(reg16 & PCI_EXP_LNKSTA_SLC))
+ ret = pcie_capability_read_word(child, PCI_EXP_LNKSTA, &reg16);
+ 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, &reg16);
- if (!(reg16 & PCI_EXP_LNKSTA_SLC))
+ ret = pcie_capability_read_word(parent, PCI_EXP_LNKSTA, &reg16);
+ 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, &reg16);
- if (same_clock && (reg16 & PCI_EXP_LNKCTL_CCC)) {
+ ret = pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
+ 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,
&reg16);
- if (!(reg16 & PCI_EXP_LNKCTL_CCC)) {
+ if (ret || !(reg16 & PCI_EXP_LNKCTL_CCC)) {
consistent = false;
break;
}
--
2.18.2

2020-07-07 23:04:47

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 13/13] PCI: Remove 'val = 0' from pcie_capability_read_()

From: 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.

Suggested-by: Bjorn Helgaas <[email protected]>
Signed-off-by: Bolarinwa Olayemi Saheed <[email protected]>
---
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(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 79c4a2ef269a..ec95edbb1ac8 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -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;
}

--
2.18.2

2020-07-07 23:05:50

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 9/13] PCI: Check return value of pcie_capability_read_*()

2020-07-07 23:07:04

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 5/13] PCI: pciehp: Check the return value of pcie_capability_read_*()

From: Bolarinwa Olayemi Saheed <[email protected]>

On any failure pcie_capability_read_word() sets it's last parameter, *val
to 0. If pci_config_read_word() fails the *val is reset to 0. Any function
which check only for a frabricated ~0 which fail in this case.

Include a check on the return value of pcie_capability_read_word() to
confirm success or failure.

Suggested-by: Bjorn Helgaas <[email protected]>
Signed-off-by: Bolarinwa Olayemi Saheed <[email protected]>

---
drivers/pci/hotplug/pciehp_hpc.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 53433b37e181..5af281d97d4f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -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);
--
2.18.2

2020-07-07 23:08:01

by Saheed O. Bolarinwa

[permalink] [raw]

Subject: [PATCH 6/13] PCI: pciehp: Make "Power On" the default

From: 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. Patch 13/13
changes the behaviour of pcie_capability_read_word() so on failure the
"Off" value will be set.

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.

Suggested-by: Bjorn Helgaas <[email protected]>
Signed-off-by: Bolarinwa Olayemi Saheed <[email protected]>
---
drivers/pci/hotplug/pciehp_hpc.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 0b691e37fd04..78f806a9c6f1 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -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)
--
2.18.2

2020-07-08 06:56:00

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 2/13] misc: rtsx: Check the return value of pcie_capability_read_*()

On Wed, Jul 08, 2020 at 12:03:13AM +0200, Saheed Olayemi Bolarinwa wrote:
> 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 13/13, 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.
>
> Suggested-by: Bjorn Helgaas <[email protected]>
> Signed-off-by: Bolarinwa Olayemi Saheed <[email protected]>

Acked-by: Greg Kroah-Hartman <[email protected]>

Subject: [PATCH 0/13] PCI: Remove '*val = 0' from pcie_capability_read_*()

Subject: [PATCH 2/13] misc: rtsx: Check the return value of pcie_capability_read_*()

Subject: [PATCH 8/13] PCI: pciehp: Check return value of pcie_capability_read_*()

Subject: [PATCH 10/13] PCI/PM: Check return value of pcie_capability_read_*()

Subject: [PATCH 12/13] PCI/ASPM: Check the return value of pcie_capability_read_*()

Subject: [PATCH 13/13] PCI: Remove '*val = 0' from pcie_capability_read_*()

Subject: [PATCH 9/13] PCI: Check return value of pcie_capability_read_*()

Subject: [PATCH 5/13] PCI: pciehp: Check the return value of pcie_capability_read_*()

Subject: [PATCH 6/13] PCI: pciehp: Make "Power On" the default

Subject: Re: [PATCH 2/13] misc: rtsx: Check the return value of pcie_capability_read_*()

Subject: [PATCH 0/13] PCI: Remove 'val = 0' from pcie_capability_read_()

Subject: [PATCH 13/13] PCI: Remove 'val = 0' from pcie_capability_read_()