LNKCTL register is written by a number of things in the kernel as RMW
operations without any concurrency control. This could in the unlucky
case lead to losing one of the updates. One of the most obvious path
which can race with most of the other LNKCTL RMW operations seems to be
ASPM policy sysfs write which triggers LNKCTL update.
Add pcie_capability_clear_and_set_word_locked() that uses a per device
spinlock to protect the RMW operations. Introduce helpers for updating
LNKCTL and LNKCTL2 that internally do
pcie_capability_clear_and_set_word_locked(). Convert RMW to use the new
helpers.
These could be mostly marked with Fixes tags but I've not spent the
effort to find those out for each and every patch until this series has
seen some discussion. I certainly will try to find the Fixes tags if
asked to.
There could be a few LNKCTL RMW that are so early into the init that
they would be safe but I was not able to convince myself so I've
included them (namely, some ASPM init paths and hp link enable). Even
if that is the case, it seems safer to use an access pattern with these
registers that is safe even if there would be a few cases where locking
would not be stricly necessary.
As for LNKCTL2, I think all current users are safe but these came up as
a part of PCIe bandwidth control work (for thermal reasons) that will
be adding a writer for LNKCTL2. I'll send PCI BW ctrl patches
separately later as there's plenty of patches in this series already.
If most of this series is deemed worthy of Fixes tags, I could separate
those few LNKCTL2 changes into own patches.
The series is based on top of the "PCI/ASPM: Handle link retraining
race" patch I sent earlier but is not yet applied.
Ilpo Järvinen (17):
PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}
PCI: pciehp: Protect LNKCTL changes
PCI/ASPM: Use pcie_lnkctl_clear_and_set()
drm/amdgpu: Use pcie_lnkctl{,2}_clear_and_set() for changing
LNKCTL{,2}
drm/radeon: Use pcie_lnkctl{,2}_clear_and_set() for changing
LNKCTL{,2}
IB/hfi1: Use pcie_lnkctl{,2}_clear_and_set() for changing LNKCTL{,2}
e1000e: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
net/mlx5: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
wifi: ath9k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
mt76: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
Bluetooth: hci_bcm4377: Use pcie_lnkctl_clear_and_set() for changing
LNKCTL
misc: rtsx: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
net/tg3: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
wifi: ath11k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
wifi: ath12k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
wifi: ath10k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL
drivers/bluetooth/hci_bcm4377.c | 3 +-
drivers/gpu/drm/amd/amdgpu/cik.c | 72 +++++-------------
drivers/gpu/drm/amd/amdgpu/si.c | 74 ++++++-------------
drivers/gpu/drm/radeon/cik.c | 71 +++++-------------
drivers/gpu/drm/radeon/si.c | 72 +++++-------------
drivers/infiniband/hw/hfi1/aspm.c | 16 ++--
drivers/infiniband/hw/hfi1/pcie.c | 28 ++-----
drivers/misc/cardreader/rts5228.c | 6 +-
drivers/misc/cardreader/rts5261.c | 6 +-
drivers/misc/cardreader/rtsx_pcr.c | 8 +-
drivers/net/ethernet/broadcom/tg3.c | 14 ++--
drivers/net/ethernet/intel/e1000e/netdev.c | 6 +-
.../ethernet/mellanox/mlx5/core/fw_reset.c | 9 +--
drivers/net/ethernet/realtek/r8169_main.c | 6 +-
drivers/net/wireless/ath/ath10k/pci.c | 8 +-
drivers/net/wireless/ath/ath11k/pci.c | 8 +-
drivers/net/wireless/ath/ath12k/pci.c | 8 +-
drivers/net/wireless/ath/ath9k/pci.c | 9 ++-
drivers/net/wireless/mediatek/mt76/pci.c | 5 +-
drivers/pci/access.c | 14 ++++
drivers/pci/hotplug/pciehp_hpc.c | 11 +--
drivers/pci/pcie/aspm.c | 48 ++++--------
drivers/pci/probe.c | 1 +
include/linux/pci.h | 17 +++++
24 files changed, 183 insertions(+), 337 deletions(-)
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL/LNKCTL2.
ASPM policy changes can trigger write to LNKCTL outside of driver's
control. And in the case of upstream (parent), the driver does not even
own the device it's changing the registers for.
Use pcie_lnkctl_clear_and_set() and pcie_lnkctl2_clear_and_set() which
do proper locking to avoid losing concurrent updates to the register
value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/gpu/drm/radeon/cik.c | 71 ++++++++++-------------------------
drivers/gpu/drm/radeon/si.c | 72 ++++++++++--------------------------
2 files changed, 40 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index 5819737c21c6..c592b3d68ae6 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -9534,17 +9534,8 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
u16 bridge_cfg2, gpu_cfg2;
u32 max_lw, current_lw, tmp;
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &bridge_cfg);
- pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL,
- &gpu_cfg);
-
- tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
-
- tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, 0, PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(rdev->pdev, 0, PCI_EXP_LNKCTL_HAWD);
tmp = RREG32_PCIE_PORT(PCIE_LC_STATUS1);
max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
@@ -9591,45 +9582,24 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
msleep(100);
/* linkctl */
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(root, PCI_EXP_LNKCTL,
- tmp16);
-
- pcie_capability_read_word(rdev->pdev,
- PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(rdev->pdev,
- PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, PCI_EXP_LNKCTL_HAWD,
+ bridge_cfg & PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(rdev->pdev, PCI_EXP_LNKCTL_HAWD,
+ gpu_cfg & PCI_EXP_LNKCTL_HAWD);
/* linkctl2 */
- pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (bridge_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(root,
- PCI_EXP_LNKCTL2,
- tmp16);
-
- pcie_capability_read_word(rdev->pdev,
- PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (gpu_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(rdev->pdev,
- PCI_EXP_LNKCTL2,
- tmp16);
+ pcie_lnkctl2_clear_and_set(root,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ bridge_cfg2 |
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
+ pcie_lnkctl2_clear_and_set(rdev->pdev,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ gpu_cfg2 |
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
tmp &= ~LC_SET_QUIESCE;
@@ -9643,15 +9613,14 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
speed_cntl &= ~LC_FORCE_DIS_SW_SPEED_CHANGE;
WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
- pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL2, &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
+ tmp16 = 0;
if (speed_cap == PCIE_SPEED_8_0GT)
tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
else if (speed_cap == PCIE_SPEED_5_0GT)
tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
else
tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
- pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL2, tmp16);
+ pcie_lnkctl2_clear_and_set(rdev->pdev, PCI_EXP_LNKCTL2_TLS, tmp16);
speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);
speed_cntl |= LC_INITIATE_LINK_SPEED_CHANGE;
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index 8d5e4b25609d..769464e34f9f 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -7131,17 +7131,8 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
u16 bridge_cfg2, gpu_cfg2;
u32 max_lw, current_lw, tmp;
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &bridge_cfg);
- pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL,
- &gpu_cfg);
-
- tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
-
- tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, 0, PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(rdev->pdev, 0, PCI_EXP_LNKCTL_HAWD);
tmp = RREG32_PCIE(PCIE_LC_STATUS1);
max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
@@ -7188,46 +7179,24 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
msleep(100);
/* linkctl */
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(root,
- PCI_EXP_LNKCTL,
- tmp16);
-
- pcie_capability_read_word(rdev->pdev,
- PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(rdev->pdev,
- PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, PCI_EXP_LNKCTL_HAWD,
+ bridge_cfg & PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(rdev->pdev, PCI_EXP_LNKCTL_HAWD,
+ gpu_cfg & PCI_EXP_LNKCTL_HAWD);
/* linkctl2 */
- pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (bridge_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(root,
- PCI_EXP_LNKCTL2,
- tmp16);
-
- pcie_capability_read_word(rdev->pdev,
- PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (gpu_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(rdev->pdev,
- PCI_EXP_LNKCTL2,
- tmp16);
+ pcie_lnkctl2_clear_and_set(root,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ bridge_cfg2 &
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
+ pcie_lnkctl2_clear_and_set(rdev->pdev,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ gpu_cfg2 &
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
tmp &= ~LC_SET_QUIESCE;
@@ -7241,15 +7210,14 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
speed_cntl &= ~LC_FORCE_DIS_SW_SPEED_CHANGE;
WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
- pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL2, &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
+ tmp16 = 0;
if (speed_cap == PCIE_SPEED_8_0GT)
tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
else if (speed_cap == PCIE_SPEED_5_0GT)
tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
else
tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
- pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL2, tmp16);
+ pcie_lnkctl2_clear_and_set(rdev->pdev, PCI_EXP_LNKCTL2_TLS, tmp16);
speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);
speed_cntl |= LC_INITIATE_LINK_SPEED_CHANGE;
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL of the
upstream (bridge). ASPM policy changes can trigger write to LNKCTL
outside of driver's control.
Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c
index 50022e7565f1..2c3d69f3a107 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c
@@ -332,16 +332,11 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev)
pci_cfg_access_lock(sdev);
}
/* PCI link toggle */
- err = pci_read_config_word(bridge, cap + PCI_EXP_LNKCTL, ®16);
- if (err)
- return err;
- reg16 |= PCI_EXP_LNKCTL_LD;
- err = pci_write_config_word(bridge, cap + PCI_EXP_LNKCTL, reg16);
+ err = pcie_lnkctl_clear_and_set(bridge, 0, PCI_EXP_LNKCTL_LD);
if (err)
return err;
msleep(500);
- reg16 &= ~PCI_EXP_LNKCTL_LD;
- err = pci_write_config_word(bridge, cap + PCI_EXP_LNKCTL, reg16);
+ err = pcie_lnkctl_clear_and_set(bridge, PCI_EXP_LNKCTL_LD, 0);
if (err)
return err;
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL/LNKCTL2.
ASPM policy changes can trigger write to LNKCTL outside of driver's
control. And in the case of upstream (parent), the driver does not even
own the device it's changing the registers for.
Use pcie_lnkctl_clear_and_set() and pcie_lnkctl2_clear_and_set() which
do proper locking to avoid losing concurrent updates to the register
value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/cik.c | 72 +++++++++----------------------
drivers/gpu/drm/amd/amdgpu/si.c | 74 +++++++++-----------------------
2 files changed, 41 insertions(+), 105 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index de6d10390ab2..f9f2c28c7125 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev)
u16 bridge_cfg2, gpu_cfg2;
u32 max_lw, current_lw, tmp;
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &bridge_cfg);
- pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL,
- &gpu_cfg);
-
- tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
-
- tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, 0, PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(adev->pdev, 0, PCI_EXP_LNKCTL_HAWD);
tmp = RREG32_PCIE(ixPCIE_LC_STATUS1);
max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >>
@@ -1637,45 +1628,24 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev)
msleep(100);
/* linkctl */
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(root, PCI_EXP_LNKCTL,
- tmp16);
-
- pcie_capability_read_word(adev->pdev,
- PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(adev->pdev,
- PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, PCI_EXP_LNKCTL_HAWD,
+ bridge_cfg & PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(adev->pdev, PCI_EXP_LNKCTL_HAWD,
+ gpu_cfg & PCI_EXP_LNKCTL_HAWD);
/* linkctl2 */
- pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (bridge_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(root,
- PCI_EXP_LNKCTL2,
- tmp16);
-
- pcie_capability_read_word(adev->pdev,
- PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (gpu_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(adev->pdev,
- PCI_EXP_LNKCTL2,
- tmp16);
+ pcie_lnkctl2_clear_and_set(root,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ bridge_cfg2 &
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
+ pcie_lnkctl2_clear_and_set(adev->pdev,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ gpu_cfg2 &
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
tmp = RREG32_PCIE(ixPCIE_LC_CNTL4);
tmp &= ~PCIE_LC_CNTL4__LC_SET_QUIESCE_MASK;
@@ -1690,16 +1660,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device *adev)
speed_cntl &= ~PCIE_LC_SPEED_CNTL__LC_FORCE_DIS_SW_SPEED_CHANGE_MASK;
WREG32_PCIE(ixPCIE_LC_SPEED_CNTL, speed_cntl);
- pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL2, &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
-
+ tmp16 = 0;
if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN3)
tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
else if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN2)
tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
else
tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
- pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL2, tmp16);
+ pcie_lnkctl2_clear_and_set(adev->pdev, PCI_EXP_LNKCTL2_TLS, tmp16);
speed_cntl = RREG32_PCIE(ixPCIE_LC_SPEED_CNTL);
speed_cntl |= PCIE_LC_SPEED_CNTL__LC_INITIATE_LINK_SPEED_CHANGE_MASK;
diff --git a/drivers/gpu/drm/amd/amdgpu/si.c b/drivers/gpu/drm/amd/amdgpu/si.c
index 7f99e130acd0..e60174d0dfb8 100644
--- a/drivers/gpu/drm/amd/amdgpu/si.c
+++ b/drivers/gpu/drm/amd/amdgpu/si.c
@@ -2276,17 +2276,8 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev)
u16 bridge_cfg2, gpu_cfg2;
u32 max_lw, current_lw, tmp;
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &bridge_cfg);
- pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL,
- &gpu_cfg);
-
- tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
-
- tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
- pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, 0, PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(adev->pdev, 0, PCI_EXP_LNKCTL_HAWD);
tmp = RREG32_PCIE(PCIE_LC_STATUS1);
max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
@@ -2331,44 +2322,23 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev)
mdelay(100);
- pcie_capability_read_word(root, PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(root, PCI_EXP_LNKCTL,
- tmp16);
-
- pcie_capability_read_word(adev->pdev,
- PCI_EXP_LNKCTL,
- &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
- tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
- pcie_capability_write_word(adev->pdev,
- PCI_EXP_LNKCTL,
- tmp16);
-
- pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (bridge_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(root,
- PCI_EXP_LNKCTL2,
- tmp16);
-
- pcie_capability_read_word(adev->pdev,
- PCI_EXP_LNKCTL2,
- &tmp16);
- tmp16 &= ~(PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN);
- tmp16 |= (gpu_cfg2 &
- (PCI_EXP_LNKCTL2_ENTER_COMP |
- PCI_EXP_LNKCTL2_TX_MARGIN));
- pcie_capability_write_word(adev->pdev,
- PCI_EXP_LNKCTL2,
- tmp16);
+ pcie_lnkctl_clear_and_set(root, PCI_EXP_LNKCTL_HAWD,
+ bridge_cfg & PCI_EXP_LNKCTL_HAWD);
+ pcie_lnkctl_clear_and_set(adev->pdev, PCI_EXP_LNKCTL_HAWD,
+ gpu_cfg & PCI_EXP_LNKCTL_HAWD);
+
+ pcie_lnkctl2_clear_and_set(root,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ bridge_cfg2 &
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
+ pcie_lnkctl2_clear_and_set(adev->pdev,
+ PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN,
+ gpu_cfg2 &
+ (PCI_EXP_LNKCTL2_ENTER_COMP |
+ PCI_EXP_LNKCTL2_TX_MARGIN));
tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
tmp &= ~LC_SET_QUIESCE;
@@ -2381,16 +2351,14 @@ static void si_pcie_gen3_enable(struct amdgpu_device *adev)
speed_cntl &= ~LC_FORCE_DIS_SW_SPEED_CHANGE;
WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
- pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL2, &tmp16);
- tmp16 &= ~PCI_EXP_LNKCTL2_TLS;
-
+ tmp16 = 0;
if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN3)
tmp16 |= PCI_EXP_LNKCTL2_TLS_8_0GT; /* gen3 */
else if (adev->pm.pcie_gen_mask & CAIL_PCIE_LINK_SPEED_SUPPORT_GEN2)
tmp16 |= PCI_EXP_LNKCTL2_TLS_5_0GT; /* gen2 */
else
tmp16 |= PCI_EXP_LNKCTL2_TLS_2_5GT; /* gen1 */
- pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL2, tmp16);
+ pcie_lnkctl2_clear_and_set(adev->pdev, PCI_EXP_LNKCTL2_TLS, tmp16);
speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);
speed_cntl |= LC_INITIATE_LINK_SPEED_CHANGE;
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/misc/cardreader/rts5228.c | 6 ++----
drivers/misc/cardreader/rts5261.c | 6 ++----
drivers/misc/cardreader/rtsx_pcr.c | 8 +++-----
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/misc/cardreader/rts5228.c b/drivers/misc/cardreader/rts5228.c
index cfebad51d1d8..74f407fff460 100644
--- a/drivers/misc/cardreader/rts5228.c
+++ b/drivers/misc/cardreader/rts5228.c
@@ -515,8 +515,7 @@ static void rts5228_enable_aspm(struct rtsx_pcr *pcr, bool enable)
val = FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1;
val |= (pcr->aspm_en & 0x02);
rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val);
- pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_ASPMC, pcr->aspm_en);
+ pcie_lnkctl_clear_and_set(pcr->pci, PCI_EXP_LNKCTL_ASPMC, pcr->aspm_en);
pcr->aspm_enabled = enable;
}
@@ -527,8 +526,7 @@ static void rts5228_disable_aspm(struct rtsx_pcr *pcr, bool enable)
if (pcr->aspm_enabled == enable)
return;
- pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_ASPMC, 0);
+ pcie_lnkctl_clear_and_set(pcr->pci, PCI_EXP_LNKCTL_ASPMC, 0);
mask = FORCE_ASPM_VAL_MASK | FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1;
val = FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1;
rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val);
diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
index b1e76030cafd..830b595e5968 100644
--- a/drivers/misc/cardreader/rts5261.c
+++ b/drivers/misc/cardreader/rts5261.c
@@ -596,8 +596,7 @@ static void rts5261_enable_aspm(struct rtsx_pcr *pcr, bool enable)
val |= (pcr->aspm_en & 0x02);
rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val);
- pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_ASPMC, pcr->aspm_en);
+ pcie_lnkctl_clear_and_set(pcr->pci, PCI_EXP_LNKCTL_ASPMC, pcr->aspm_en);
pcr->aspm_enabled = enable;
}
@@ -609,8 +608,7 @@ static void rts5261_disable_aspm(struct rtsx_pcr *pcr, bool enable)
if (pcr->aspm_enabled == enable)
return;
- pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_ASPMC, 0);
+ pcie_lnkctl_clear_and_set(pcr->pci, PCI_EXP_LNKCTL_ASPMC, 0);
rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val);
rtsx_pci_write_register(pcr, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0);
udelay(10);
diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c
index 32b7783e9d4f..a9b26846aec6 100644
--- a/drivers/misc/cardreader/rtsx_pcr.c
+++ b/drivers/misc/cardreader/rtsx_pcr.c
@@ -86,9 +86,8 @@ static void rtsx_comm_set_aspm(struct rtsx_pcr *pcr, bool enable)
return;
if (pcr->aspm_mode == ASPM_MODE_CFG) {
- pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_ASPMC,
- enable ? pcr->aspm_en : 0);
+ pcie_lnkctl_clear_and_set(pcr->pci, PCI_EXP_LNKCTL_ASPMC,
+ enable ? pcr->aspm_en : 0);
} else if (pcr->aspm_mode == ASPM_MODE_REG) {
if (pcr->aspm_en & 0x02)
rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 |
@@ -1315,8 +1314,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr)
rtsx_pci_init_ocp(pcr);
/* Enable clk_request_n to enable clock power management */
- pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL,
- 0, PCI_EXP_LNKCTL_CLKREQ_EN);
+ pcie_lnkctl_clear_and_set(pcr->pci, 0, PCI_EXP_LNKCTL_CLKREQ_EN);
/* Enter L1 when host tx idle */
pci_write_config_byte(pdev, 0x70F, 0x5B);
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
And in the case of upstream (parent), the driver does not even own the
device it's changing LNKCTL for.
Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index bd7ef59b1f2e..29d50aeb2c3e 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6829,11 +6829,9 @@ static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state, int locked)
/* Both device and parent should have the same ASPM setting.
* Disable ASPM in downstream component first and then upstream.
*/
- pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, aspm_dis_mask);
-
+ pcie_lnkctl_clear_and_set(pdev, aspm_dis_mask, 0);
if (parent)
- pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
- aspm_dis_mask);
+ pcie_lnkctl_clear_and_set(parent, aspm_dis_mask, 0);
}
/**
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/net/ethernet/broadcom/tg3.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 58747292521d..f3b30e7af25d 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -4027,8 +4027,7 @@ static int tg3_power_down_prepare(struct tg3 *tp)
/* Restore the CLKREQ setting. */
if (tg3_flag(tp, CLKREQ_BUG))
- pcie_capability_set_word(tp->pdev, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_CLKREQ_EN);
+ pcie_lnkctl_clear_and_set(tp->pdev, 0, PCI_EXP_LNKCTL_CLKREQ_EN);
misc_host_ctrl = tr32(TG3PCI_MISC_HOST_CTRL);
tw32(TG3PCI_MISC_HOST_CTRL,
@@ -5069,13 +5068,14 @@ static int tg3_setup_copper_phy(struct tg3 *tp, bool force_reset)
/* Prevent send BD corruption. */
if (tg3_flag(tp, CLKREQ_BUG)) {
+ u16 clkreq = 0;
+
if (tp->link_config.active_speed == SPEED_100 ||
tp->link_config.active_speed == SPEED_10)
- pcie_capability_clear_word(tp->pdev, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_CLKREQ_EN);
- else
- pcie_capability_set_word(tp->pdev, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_CLKREQ_EN);
+ clkreq = PCI_EXP_LNKCTL_CLKREQ_EN;
+
+ pcie_lnkctl_clear_and_set(tp->pdev, PCI_EXP_LNKCTL_CLKREQ_EN,
+ clkreq);
}
tg3_test_and_report_link_chg(tp, current_link_up);
--
2.30.2
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register value.
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/net/ethernet/realtek/r8169_main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a7e376e7e689..c0294a833681 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2686,14 +2686,12 @@ static void __rtl_ephy_init(struct rtl8169_private *tp,
static void rtl_disable_clock_request(struct rtl8169_private *tp)
{
- pcie_capability_clear_word(tp->pci_dev, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_CLKREQ_EN);
+ pcie_lnkctl_clear_and_set(tp->pci_dev, PCI_EXP_LNKCTL_CLKREQ_EN, 0);
}
static void rtl_enable_clock_request(struct rtl8169_private *tp)
{
- pcie_capability_set_word(tp->pci_dev, PCI_EXP_LNKCTL,
- PCI_EXP_LNKCTL_CLKREQ_EN);
+ pcie_lnkctl_clear_and_set(tp->pci_dev, 0, PCI_EXP_LNKCTL_CLKREQ_EN);
}
static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
--
2.30.2
On 11.05.2023 15:14, Ilpo Järvinen wrote:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
>
> Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> losing concurrent updates to the register value.
>
Wouldn't it be more appropriate to add proper locking to the
underlying pcie_capability_clear_and_set_word()?
> Suggested-by: Lukas Wunner <[email protected]>
> Signed-off-by: Ilpo Järvinen <[email protected]>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index a7e376e7e689..c0294a833681 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2686,14 +2686,12 @@ static void __rtl_ephy_init(struct rtl8169_private *tp,
>
> static void rtl_disable_clock_request(struct rtl8169_private *tp)
> {
> - pcie_capability_clear_word(tp->pci_dev, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_CLKREQ_EN);
> + pcie_lnkctl_clear_and_set(tp->pci_dev, PCI_EXP_LNKCTL_CLKREQ_EN, 0);
> }
>
> static void rtl_enable_clock_request(struct rtl8169_private *tp)
> {
> - pcie_capability_set_word(tp->pci_dev, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_CLKREQ_EN);
> + pcie_lnkctl_clear_and_set(tp->pci_dev, 0, PCI_EXP_LNKCTL_CLKREQ_EN);
> }
>
> static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
On 11.05.2023 22:02, Lukas Wunner wrote:
> On Thu, May 11, 2023 at 09:49:52PM +0200, Heiner Kallweit wrote:
>> On 11.05.2023 15:14, Ilpo Järvinen wrote:
>>> Don't assume that only the driver would be accessing LNKCTL. ASPM
>>> policy changes can trigger write to LNKCTL outside of driver's control.
>>>
>>> Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
>>> losing concurrent updates to the register value.
>>
>> Wouldn't it be more appropriate to add proper locking to the
>> underlying pcie_capability_clear_and_set_word()?
>
> PCI config space accessors such as this one are also used in hot paths
> (e.g. interrupt handlers). They should be kept lean (and lockless)
I *think* in case the system uses threaded interrupts you may need locking
also in interrupt handlers.
> by default. We only need locking for specific PCIe Extended Capabilities
> which are concurrently accessed by PCI core code and drivers.
>
> Thanks,
>
> Lukas
On Thu, May 11, 2023 at 11:00:02PM +0300, Ilpo J?rvinen wrote:
> On Thu, 11 May 2023, Heiner Kallweit wrote:
> > On 11.05.2023 15:14, Ilpo J?rvinen wrote:
> > > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > > policy changes can trigger write to LNKCTL outside of driver's control.
> > >
> > > Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> > > losing concurrent updates to the register value.
> > >
> >
> > Wouldn't it be more appropriate to add proper locking to the
> > underlying pcie_capability_clear_and_set_word()?
>
> As per discussion for the other patch, that's where this series is now
> aiming to in v2.
That discussion wasn't cc'ed to Heiner. For reference, this is the
message Ilpo is referring to:
https://lore.kernel.org/linux-pci/ZF1AjOKDVlbNFJPK@bhelgaas/
On 11.05.2023 22:10, Lukas Wunner wrote:
> On Thu, May 11, 2023 at 11:00:02PM +0300, Ilpo Järvinen wrote:
>> On Thu, 11 May 2023, Heiner Kallweit wrote:
>>> On 11.05.2023 15:14, Ilpo Järvinen wrote:
>>>> Don't assume that only the driver would be accessing LNKCTL. ASPM
>>>> policy changes can trigger write to LNKCTL outside of driver's control.
>>>>
>>>> Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
>>>> losing concurrent updates to the register value.
>>>>
>>>
>>> Wouldn't it be more appropriate to add proper locking to the
>>> underlying pcie_capability_clear_and_set_word()?
>>
>> As per discussion for the other patch, that's where this series is now
>> aiming to in v2.
>
> That discussion wasn't cc'ed to Heiner. For reference, this is the
> message Ilpo is referring to:
>
> https://lore.kernel.org/linux-pci/ZF1AjOKDVlbNFJPK@bhelgaas/
Thanks for the link!
On Thu, May 11, 2023 at 09:49:52PM +0200, Heiner Kallweit wrote:
> On 11.05.2023 15:14, Ilpo J?rvinen wrote:
> > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > policy changes can trigger write to LNKCTL outside of driver's control.
> >
> > Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> > losing concurrent updates to the register value.
>
> Wouldn't it be more appropriate to add proper locking to the
> underlying pcie_capability_clear_and_set_word()?
PCI config space accessors such as this one are also used in hot paths
(e.g. interrupt handlers). They should be kept lean (and lockless)
by default. We only need locking for specific PCIe Extended Capabilities
which are concurrently accessed by PCI core code and drivers.
Thanks,
Lukas
On Thu, 11 May 2023, Heiner Kallweit wrote:
> On 11.05.2023 15:14, Ilpo Järvinen wrote:
> > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > policy changes can trigger write to LNKCTL outside of driver's control.
> >
> > Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> > losing concurrent updates to the register value.
> >
>
> Wouldn't it be more appropriate to add proper locking to the
> underlying pcie_capability_clear_and_set_word()?
As per discussion for the other patch, that's where this series is now
aiming to in v2.
--
i.