* [PATCH 3/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*()
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
@ 2020-07-06 9:31 ` Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 4/11 " Saheed Olayemi Bolarinwa
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Saheed Olayemi Bolarinwa @ 2020-07-06 9:31 UTC (permalink / raw)
To: helgaas
Cc: Bolarinwa Olayemi Saheed, bjorn, Lukas Wunner, skhan, linux-pci,
linux-kernel-mentees, linux-kernel
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
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_dword() to
confirm success or failure.
Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*()
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
2020-07-06 9:31 ` [PATCH 3/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*() Saheed Olayemi Bolarinwa
@ 2020-07-06 9:31 ` Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default Saheed Olayemi Bolarinwa
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Saheed Olayemi Bolarinwa @ 2020-07-06 9:31 UTC (permalink / raw)
To: helgaas
Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci,
linux-kernel-mentees, linux-kernel
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
On failure pcie_capability_read_dword() sets it's last parameter,
val to 0.
However, with Patch 11/11, 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.
Return a value that indicate the result of pcie_capability_read_dword().
Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
drivers/pci/hotplug/pciehp_hpc.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5af281d97d4f..0b691e37fd04 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -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)
--
2.18.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
2020-07-06 9:31 ` [PATCH 3/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*() Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 4/11 " Saheed Olayemi Bolarinwa
@ 2020-07-06 9:31 ` Saheed Olayemi Bolarinwa
2020-07-10 0:14 ` Bjorn Helgaas
2020-07-06 9:31 ` [PATCH 7/11 RFC] PCI: Validate with the return value of pcie_capability_read_*() Saheed Olayemi Bolarinwa
` (3 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: Saheed Olayemi Bolarinwa @ 2020-07-06 9:31 UTC (permalink / raw)
To: helgaas
Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci,
linux-kernel-mentees, linux-kernel
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
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 11/11
changes the behaviour of pcie_capability_read_word() so on falure 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 <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 7/11 RFC] PCI: Validate with the return value of pcie_capability_read_*()
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
` (2 preceding siblings ...)
2020-07-06 9:31 ` [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default Saheed Olayemi Bolarinwa
@ 2020-07-06 9:31 ` Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 8/11 RFC] PCI/PM: Use error return value from pcie_capability_read_*() Saheed Olayemi Bolarinwa
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Saheed Olayemi Bolarinwa @ 2020-07-06 9:31 UTC (permalink / raw)
To: helgaas
Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci,
linux-kernel-mentees, linux-kernel
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
On failure pcie_capability_read_dword() sets it's last parameter,
val to 0.
However, with Patch 11/11, 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 <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
drivers/pci/probe.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2f66988cea25..3c87a8a1d4b5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
--
2.18.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 8/11 RFC] PCI/PM: Use error return value from pcie_capability_read_*()
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
` (3 preceding siblings ...)
2020-07-06 9:31 ` [PATCH 7/11 RFC] PCI: Validate with the return value of pcie_capability_read_*() Saheed Olayemi Bolarinwa
@ 2020-07-06 9:31 ` Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 10/11 RFC] PCI/ASPM: " Saheed Olayemi Bolarinwa
2020-07-10 0:11 ` [PATCH 0/11 RFC] PCI: Remove "*val = 0" " Bjorn Helgaas
6 siblings, 0 replies; 10+ messages in thread
From: Saheed Olayemi Bolarinwa @ 2020-07-06 9:31 UTC (permalink / raw)
To: helgaas
Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci,
linux-kernel-mentees, linux-kernel
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
On failure pcie_capability_read_dword() sets it's last parameter,
val to 0.
However, with Patch 11/11, 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 <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 10/11 RFC] PCI/ASPM: Use error return value from pcie_capability_read_*()
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
` (4 preceding siblings ...)
2020-07-06 9:31 ` [PATCH 8/11 RFC] PCI/PM: Use error return value from pcie_capability_read_*() Saheed Olayemi Bolarinwa
@ 2020-07-06 9:31 ` Saheed Olayemi Bolarinwa
2020-07-10 0:11 ` [PATCH 0/11 RFC] PCI: Remove "*val = 0" " Bjorn Helgaas
6 siblings, 0 replies; 10+ messages in thread
From: Saheed Olayemi Bolarinwa @ 2020-07-06 9:31 UTC (permalink / raw)
To: helgaas
Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci,
linux-kernel-mentees, linux-kernel
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
On failure pcie_capability_read_dword() sets it's last parameter,
val to 0.
However, with Patch 11/11, 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 <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
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, ®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;
}
--
2.18.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/11 RFC] PCI: Remove "*val = 0" from pcie_capability_read_*()
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
` (5 preceding siblings ...)
2020-07-06 9:31 ` [PATCH 10/11 RFC] PCI/ASPM: " Saheed Olayemi Bolarinwa
@ 2020-07-10 0:11 ` Bjorn Helgaas
2020-07-10 21:32 ` Saheed Bolarinwa
6 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2020-07-10 0:11 UTC (permalink / raw)
To: Saheed Olayemi Bolarinwa; +Cc: linux-pci
The cc list is really screwed up. Here's what I got:
Cc: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>,
bjorn@helgaas.com,
skhan@linuxfoundation.org,
linux-pci@vger.kernel.org,
linux-kernel-mentees@lists.linuxfoundation.org,
linux-kernel@vger.kernel.org,
Russell Currey <ruscur@russell.cc>,
Sam Bobroff <sbobroff@linux.ibm.com>,
"Oliver O'Halloran" <oohall@gmail.com>,
linuxppc-dev@lists.ozlabs.org,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Len Brown Lukas Wunner <lenb@kernel.orglukas@wunner.de>,
linux-acpi@vger.kernel.org,
Mike Marciniszyn <mike.marciniszyn@intel.com>,
Dennis Dalessandro <dennis.dalessandro@intel.com>,
Doug Ledford <dledford@redhat.com>,
Jason Gunthorpe <jgg@ziepe.ca>,
linux-rdma@vger.kernel.org,
Arnd Bergmann <arnd@arndb.de>,
"Greg Kroah-Hartman linux-rdma @ vger . kernel . org" <gregkh@linuxfoundation.org>
The addresses for Len Brown and Lukas Wunner are corrupted, as is Greg
KH's. And my reply-all didn't work -- it *should* have copied this to
everybody in the list, but Mutt apparently couldn't interpret the cc
list at all, so it left the cc list of my reply empty.
I added linux-pci by hand just so this will show up on the list.
On Mon, Jul 06, 2020 at 11:31:10AM +0200, Saheed Olayemi Bolarinwa wrote:
> From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
>
> *** BLURB HERE ***
This series isn't structured quite right. You should replace the
"BLURB HERE" above with an actual description of the series.
> Bolarinwa Olayemi Saheed (9):
> IB/hfi1: Confirm that pcie_capability_read_dword() is successful
> misc: rtsx: Confirm that pcie_capability_read_word() is successful
> PCI/AER: Use error return value from pcie_capability_read_*()
> PCI/ASPM: Use error return value from pcie_capability_read_*()
> PCI: pciehp: Fix wrong failure check on pcie_capability_read_*()
> PCI: pciehp: Prevent wrong failure check on pcie_capability_read_*()
> PCI: pciehp: Make "Power On" the default in pciehp_get_power_status()
> PCI/ACPI: Prevent wrong failure check on pcie_capability_read_*()
> PCI: Prevent wrong failure check on pcie_capability_read_*()
> PCI: Remove "*val = 0" fom pcie_capability_read_*()
And the subject claims 11 patches ("PATCH 0/11"), the header above
claims 9, the list actually contains 10, and only 8 seem to have made
it to the mailing list:
$ b4 am -om/ https://lore.kernel.org/r/20200706093121.9731-1-refactormyself@gmail.com
Looking up https://lore.kernel.org/r/20200706093121.9731-1-refactormyself%40gmail.com
Grabbing thread from lore.kernel.org/linux-kernel-mentees
Analyzing 9 messages in the thread
---
Thread incomplete, attempting to backfill
Grabbing thread from lore.kernel.org/linuxppc-dev
Grabbing thread from lore.kernel.org/lkml
Server returned an error: 404
Grabbing thread from lore.kernel.org/linux-acpi
Server returned an error: 404
Grabbing thread from lore.kernel.org/linux-pci
Server returned an error: 404
Grabbing thread from lore.kernel.org/linux-rdma
Server returned an error: 404
---
Writing m/20200706_refactormyself_pci_remove_val_0_from_pcie_capability_read.mbx
ERROR: missing [1/11]!
[PATCH 2/11 RFC] misc: rtsx: Confirm that pcie_capability_read_word() is successful
[PATCH 3/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*()
[PATCH 4/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*()
[PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default
ERROR: missing [6/11]!
[PATCH 7/11 RFC] PCI: Validate with the return value of pcie_capability_read_*()
[PATCH 8/11 RFC] PCI/PM: Use error return value from pcie_capability_read_*()
ERROR: missing [9/11]!
[PATCH 10/11 RFC] PCI/ASPM: Use error return value from pcie_capability_read_*()
[PATCH 11/11 RFC] PCI: Remove "*val = 0" from pcie_capability_read_*()
---
Total patches: 8
---
WARNING: Thread incomplete!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default
2020-07-06 9:31 ` [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default Saheed Olayemi Bolarinwa
@ 2020-07-10 0:14 ` Bjorn Helgaas
2020-07-10 21:42 ` Saheed Bolarinwa
0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2020-07-10 0:14 UTC (permalink / raw)
To: Saheed Olayemi Bolarinwa
Cc: bjorn, skhan, linux-pci, linux-kernel-mentees, linux-kernel
On Mon, Jul 06, 2020 at 11:31:15AM +0200, Saheed Olayemi Bolarinwa wrote:
> From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
>
> 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 11/11
> changes the behaviour of pcie_capability_read_word() so on falure the
> "Off" value will be set.
s/falure/failure/
Split this into two patches. The removal of the default case should
be in its own patch to make it trivial to review.
> 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 <bjorn@helgaas.com>
> Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/11 RFC] PCI: Remove "*val = 0" from pcie_capability_read_*()
2020-07-10 0:11 ` [PATCH 0/11 RFC] PCI: Remove "*val = 0" " Bjorn Helgaas
@ 2020-07-10 21:32 ` Saheed Bolarinwa
0 siblings, 0 replies; 10+ messages in thread
From: Saheed Bolarinwa @ 2020-07-10 21:32 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci
On 7/10/20 2:11 AM, Bjorn Helgaas wrote:
> The cc list is really screwed up. Here's what I got:
>
> Cc: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>,
> bjorn@helgaas.com,
> skhan@linuxfoundation.org,
> linux-pci@vger.kernel.org,
> linux-kernel-mentees@lists.linuxfoundation.org,
> linux-kernel@vger.kernel.org,
> Russell Currey <ruscur@russell.cc>,
> Sam Bobroff <sbobroff@linux.ibm.com>,
> "Oliver O'Halloran" <oohall@gmail.com>,
> linuxppc-dev@lists.ozlabs.org,
> "Rafael J. Wysocki" <rjw@rjwysocki.net>,
> Len Brown Lukas Wunner <lenb@kernel.orglukas@wunner.de>,
> linux-acpi@vger.kernel.org,
> Mike Marciniszyn <mike.marciniszyn@intel.com>,
> Dennis Dalessandro <dennis.dalessandro@intel.com>,
> Doug Ledford <dledford@redhat.com>,
> Jason Gunthorpe <jgg@ziepe.ca>,
> linux-rdma@vger.kernel.org,
> Arnd Bergmann <arnd@arndb.de>,
> "Greg Kroah-Hartman linux-rdma @ vger . kernel . org" <gregkh@linuxfoundation.org>
>
> The addresses for Len Brown and Lukas Wunner are corrupted, as is Greg
> KH's. And my reply-all didn't work -- it *should* have copied this to
> everybody in the list, but Mutt apparently couldn't interpret the cc
> list at all, so it left the cc list of my reply empty.
>
> I added linux-pci by hand just so this will show up on the list.
Thank you. I have fixed this and some other issues, I just sent version 3.
- Saheed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default
2020-07-10 0:14 ` Bjorn Helgaas
@ 2020-07-10 21:42 ` Saheed Bolarinwa
0 siblings, 0 replies; 10+ messages in thread
From: Saheed Bolarinwa @ 2020-07-10 21:42 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: bjorn, skhan, linux-pci, linux-kernel-mentees, linux-kernel
On 7/10/20 2:14 AM, Bjorn Helgaas wrote:
> On Mon, Jul 06, 2020 at 11:31:15AM +0200, Saheed Olayemi Bolarinwa wrote:
>> From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
>>
>> 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 11/11
>> changes the behaviour of pcie_capability_read_word() so on falure the
>> "Off" value will be set.
> s/falure/failure/
>
> Split this into two patches. The removal of the default case should
> be in its own patch to make it trivial to review.
>
Thank you for the review. It is now split into two in the version 3
which I just sent.
- Saheed
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-10 22:41 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200706093121.9731-1-refactormyself@gmail.com>
2020-07-06 9:31 ` [PATCH 3/11 RFC] PCI: pciehp: Validate with the return value of pcie_capability_read_*() Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 4/11 " Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 5/11 RFC] PCI: pciehp: Make "Power On" the default Saheed Olayemi Bolarinwa
2020-07-10 0:14 ` Bjorn Helgaas
2020-07-10 21:42 ` Saheed Bolarinwa
2020-07-06 9:31 ` [PATCH 7/11 RFC] PCI: Validate with the return value of pcie_capability_read_*() Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 8/11 RFC] PCI/PM: Use error return value from pcie_capability_read_*() Saheed Olayemi Bolarinwa
2020-07-06 9:31 ` [PATCH 10/11 RFC] PCI/ASPM: " Saheed Olayemi Bolarinwa
2020-07-10 0:11 ` [PATCH 0/11 RFC] PCI: Remove "*val = 0" " Bjorn Helgaas
2020-07-10 21:32 ` Saheed Bolarinwa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).