[PATCH 00/17] PCI: Improve LNKCTL & LNKCTL2 concurrency control

[PATCH 00/17] PCI: Improve LNKCTL & LNKCTL2 concurrency control

[Ilpo Järvinen]

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

[PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Ilpo Järvinen]

A few places write LNKCTL and LNKCTL2 registers without proper
concurrency control and this could result in losing the changes
one of the writers intended to make.

Add pcie_capability_clear_and_set_word_locked() and helpers to use it
with LNKCTL and LNKCTL2. The concurrency control is provided using a
spinlock in the struct pci_dev.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/access.c | 14 ++++++++++++++
 drivers/pci/probe.c  |  1 +
 include/linux/pci.h  | 17 +++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 3c230ca3de58..d92a3daadd0c 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
 }
 EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
 
+int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
+					      u16 clear, u16 set)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&dev->cap_lock, flags);
+	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
+	spin_unlock_irqrestore(&dev->cap_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
+
 int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 0b2826c4a832..0c14a283f1c7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2318,6 +2318,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 		.end = -1,
 	};
 
+	spin_lock_init(&dev->cap_lock);
 #ifdef CONFIG_PCI_MSI
 	raw_spin_lock_init(&dev->msi_lock);
 #endif
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60b8772b5bd4..82faea085d95 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -467,6 +467,7 @@ struct pci_dev {
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
+	spinlock_t	cap_lock;		/* Protects RMW ops done with locked RMW capability accessors */
 	u32		saved_config_space[16]; /* Config space saved at suspend time */
 	struct hlist_head saved_cap_space;
 	int		rom_attr_enabled;	/* Display of ROM attribute enabled? */
@@ -1221,6 +1222,8 @@ int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
 				       u16 clear, u16 set);
 int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
 					u32 clear, u32 set);
+int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
+					      u16 clear, u16 set);
 
 static inline int pcie_capability_set_word(struct pci_dev *dev, int pos,
 					   u16 set)
@@ -1246,6 +1249,20 @@ static inline int pcie_capability_clear_dword(struct pci_dev *dev, int pos,
 	return pcie_capability_clear_and_set_dword(dev, pos, clear, 0);
 }
 
+static inline int pcie_lnkctl_clear_and_set(struct pci_dev *dev, u16 clear,
+					    u16 set)
+{
+	return pcie_capability_clear_and_set_word_locked(dev, PCI_EXP_LNKCTL,
+							 clear, set);
+}
+
+static inline int pcie_lnkctl2_clear_and_set(struct pci_dev *dev, u16 clear,
+					    u16 set)
+{
+	return pcie_capability_clear_and_set_word_locked(dev, PCI_EXP_LNKCTL2,
+							 clear, set);
+}
+
 /* User-space driven config access */
 int pci_user_read_config_byte(struct pci_dev *dev, int where, u8 *val);
 int pci_user_read_config_word(struct pci_dev *dev, int where, u16 *val);
-- 
2.30.2

[PATCH 02/17] PCI: pciehp: Protect LNKCTL changes

[Ilpo Järvinen]

As hotplug is not the only driver touching LNKCTL, use the
pcie_lnkctl_clear_and_set() helper which handles concurrent changes
correctly.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index f8c70115b691..14ffbb27957c 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -332,17 +332,10 @@ int pciehp_check_link_status(struct controller *ctrl)
 static int __pciehp_link_set(struct controller *ctrl, bool enable)
 {
 	struct pci_dev *pdev = ctrl_dev(ctrl);
-	u16 lnk_ctrl;
 
-	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &lnk_ctrl);
+	pcie_lnkctl_clear_and_set(pdev, PCI_EXP_LNKCTL_LD,
+				  !enable ? PCI_EXP_LNKCTL_LD : 0);
 
-	if (enable)
-		lnk_ctrl &= ~PCI_EXP_LNKCTL_LD;
-	else
-		lnk_ctrl |= PCI_EXP_LNKCTL_LD;
-
-	pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, lnk_ctrl);
-	ctrl_dbg(ctrl, "%s: lnk_ctrl = %x\n", __func__, lnk_ctrl);
 	return 0;
 }
 
-- 
2.30.2

[PATCH 03/17] PCI/ASPM: Use pcie_lnkctl_clear_and_set()

[Ilpo Järvinen]

Don't assume that the device is fully under the control of ASPM and
use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register values.

If configuration fails in pcie_aspm_configure_common_clock(), the
function attempts to restore the old PCI_EXP_LNKCTL_CCC settings. Store
only the old PCI_EXP_LNKCTL_CCC bit for the relevant devices rather
than the content of the whole LNKCTL registers. It aligns better with
how pcie_lnkctl_clear_and_set() expects its parameter and makes the
code more obvious to understand.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/aspm.c | 48 ++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index dde1ef13d0d1..6dbef0332c2b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -147,9 +147,7 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 	u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
 
 	list_for_each_entry(child, &linkbus->devices, bus_list)
-		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_CLKREQ_EN,
-						   val);
+		pcie_lnkctl_clear_and_set(child, PCI_EXP_LNKCTL_CLKREQ_EN, val);
 	link->clkpm_enabled = !!enable;
 }
 
@@ -213,7 +211,6 @@ static bool pcie_wait_for_retrain(struct pci_dev *pdev)
 static bool pcie_retrain_link(struct pcie_link_state *link)
 {
 	struct pci_dev *parent = link->pdev;
-	u16 reg16;
 
 	/*
 	 * Ensure the updated LNKCTL parameters are used during link
@@ -224,17 +221,14 @@ static bool pcie_retrain_link(struct pcie_link_state *link)
 	if (!pcie_wait_for_retrain(parent))
 		return false;
 
-	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
-	reg16 |= PCI_EXP_LNKCTL_RL;
-	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
+	pcie_lnkctl_clear_and_set(parent, 0, PCI_EXP_LNKCTL_RL);
 	if (parent->clear_retrain_link) {
 		/*
 		 * Due to an erratum in some devices the Retrain Link bit
 		 * needs to be cleared again manually to allow the link
 		 * training to succeed.
 		 */
-		reg16 &= ~PCI_EXP_LNKCTL_RL;
-		pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
+		pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_RL, 0);
 	}
 
 	return pcie_wait_for_retrain(parent);
@@ -248,7 +242,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;
-	u16 reg16, parent_reg, child_reg[8];
+	u16 reg16, parent_old_ccc, child_old_ccc[8];
 	struct pci_dev *child, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
 	/*
@@ -270,6 +264,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 
 	/* Port might be already in common clock mode */
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
+	parent_old_ccc = reg16 & PCI_EXP_LNKCTL_CCC;
 	if (same_clock && (reg16 & PCI_EXP_LNKCTL_CCC)) {
 		bool consistent = true;
 
@@ -289,22 +284,14 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 	/* Configure downstream component, all functions */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
 		pcie_capability_read_word(child, PCI_EXP_LNKCTL, &reg16);
-		child_reg[PCI_FUNC(child->devfn)] = reg16;
-		if (same_clock)
-			reg16 |= PCI_EXP_LNKCTL_CCC;
-		else
-			reg16 &= ~PCI_EXP_LNKCTL_CCC;
-		pcie_capability_write_word(child, PCI_EXP_LNKCTL, reg16);
+		child_old_ccc[PCI_FUNC(child->devfn)] = reg16 & PCI_EXP_LNKCTL_CCC;
+		pcie_lnkctl_clear_and_set(child, PCI_EXP_LNKCTL_CCC,
+					  same_clock ? PCI_EXP_LNKCTL_CCC : 0);
 	}
 
 	/* Configure upstream component */
-	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
-	parent_reg = reg16;
-	if (same_clock)
-		reg16 |= PCI_EXP_LNKCTL_CCC;
-	else
-		reg16 &= ~PCI_EXP_LNKCTL_CCC;
-	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
+	pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_CCC,
+				  same_clock ? PCI_EXP_LNKCTL_CCC : 0);
 
 	if (pcie_retrain_link(link))
 		return;
@@ -312,9 +299,9 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 	/* Training failed. Restore common clock configurations */
 	pci_err(parent, "ASPM: Could not configure common clock\n");
 	list_for_each_entry(child, &linkbus->devices, bus_list)
-		pcie_capability_write_word(child, PCI_EXP_LNKCTL,
-					   child_reg[PCI_FUNC(child->devfn)]);
-	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, parent_reg);
+		pcie_lnkctl_clear_and_set(child, PCI_EXP_LNKCTL_CCC,
+					  child_old_ccc[PCI_FUNC(child->devfn)]);
+	pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_CCC, parent_old_ccc);
 }
 
 /* Convert L0s latency encoding to ns */
@@ -745,10 +732,8 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 	 * in pcie_config_aspm_link().
 	 */
 	if (enable_req & (ASPM_STATE_L1_1 | ASPM_STATE_L1_2)) {
-		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPM_L1, 0);
-		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPM_L1, 0);
+		pcie_lnkctl_clear_and_set(child, PCI_EXP_LNKCTL_ASPM_L1, 0);
+		pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPM_L1, 0);
 	}
 
 	val = 0;
@@ -770,8 +755,7 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 {
-	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
-					   PCI_EXP_LNKCTL_ASPMC, val);
+	pcie_lnkctl_clear_and_set(pdev, PCI_EXP_LNKCTL_ASPMC, val);
 }
 
 static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
-- 
2.30.2

[PATCH 04/17] drm/amdgpu: Use pcie_lnkctl{,2}_clear_and_set() for changing LNKCTL{,2}

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH 05/17] drm/radeon: Use pcie_lnkctl{,2}_clear_and_set() for changing LNKCTL{,2}

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH 06/17] IB/hfi1: Use pcie_lnkctl{,2}_clear_and_set() for changing LNKCTL{,2}

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/infiniband/hw/hfi1/aspm.c | 16 ++++++----------
 drivers/infiniband/hw/hfi1/pcie.c | 28 ++++++----------------------
 2 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/aspm.c b/drivers/infiniband/hw/hfi1/aspm.c
index a3c53be4072c..d3f3b7e9b833 100644
--- a/drivers/infiniband/hw/hfi1/aspm.c
+++ b/drivers/infiniband/hw/hfi1/aspm.c
@@ -66,12 +66,10 @@ static void aspm_hw_enable_l1(struct hfi1_devdata *dd)
 		return;
 
 	/* Enable ASPM L1 first in upstream component and then downstream */
-	pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
-					   PCI_EXP_LNKCTL_ASPMC,
-					   PCI_EXP_LNKCTL_ASPM_L1);
-	pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
-					   PCI_EXP_LNKCTL_ASPMC,
-					   PCI_EXP_LNKCTL_ASPM_L1);
+	pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC,
+				  PCI_EXP_LNKCTL_ASPM_L1);
+	pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC,
+				  PCI_EXP_LNKCTL_ASPM_L1);
 }
 
 void aspm_hw_disable_l1(struct hfi1_devdata *dd)
@@ -79,11 +77,9 @@ void aspm_hw_disable_l1(struct hfi1_devdata *dd)
 	struct pci_dev *parent = dd->pcidev->bus->self;
 
 	/* Disable ASPM L1 first in downstream component and then upstream */
-	pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
-					   PCI_EXP_LNKCTL_ASPMC, 0x0);
+	pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC, 0);
 	if (parent)
-		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPMC, 0x0);
+		pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC, 0);
 }
 
 static  void aspm_enable(struct hfi1_devdata *dd)
diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
index 08732e1ac966..fe7324d38d64 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -1212,14 +1212,10 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
 		    (u32)lnkctl2);
 	/* only write to parent if target is not as high as ours */
 	if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
-		lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
-		lnkctl2 |= target_vector;
-		dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
-			    (u32)lnkctl2);
-		ret = pcie_capability_write_word(parent,
-						 PCI_EXP_LNKCTL2, lnkctl2);
+		pcie_lnkctl2_clear_and_set(parent, PCI_EXP_LNKCTL2_TLS,
+					   target_vector);
 		if (ret) {
-			dd_dev_err(dd, "Unable to write to PCI config\n");
+			dd_dev_err(dd, "Unable to change PCI target speed\n");
 			return_error = 1;
 			goto done;
 		}
@@ -1228,22 +1224,10 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
 	}
 
 	dd_dev_info(dd, "%s: setting target link speed\n", __func__);
-	ret = pcie_capability_read_word(dd->pcidev, PCI_EXP_LNKCTL2, &lnkctl2);
+	ret = pcie_lnkctl2_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL2_TLS,
+					 target_vector);
 	if (ret) {
-		dd_dev_err(dd, "Unable to read from PCI config\n");
-		return_error = 1;
-		goto done;
-	}
-
-	dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
-		    (u32)lnkctl2);
-	lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
-	lnkctl2 |= target_vector;
-	dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
-		    (u32)lnkctl2);
-	ret = pcie_capability_write_word(dd->pcidev, PCI_EXP_LNKCTL2, lnkctl2);
-	if (ret) {
-		dd_dev_err(dd, "Unable to write to PCI config\n");
+		dd_dev_err(dd, "Unable to change PCI link control2\n");
 		return_error = 1;
 		goto done;
 	}
-- 
2.30.2

[PATCH 07/17] e1000e: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH 08/17] net/mlx5: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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, &reg16);
-	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

[PATCH 09/17] wifi: ath9k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/ath/ath9k/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index a09f9d223f3d..c2130fe6c9e6 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -837,15 +837,16 @@ static void ath_pci_aspm_init(struct ath_common *common)
 	if ((ath9k_hw_get_btcoex_scheme(ah) != ATH_BTCOEX_CFG_NONE) &&
 	    (AR_SREV_9285(ah))) {
 		/* Bluetooth coexistence requires disabling ASPM. */
-		pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL,
-			PCI_EXP_LNKCTL_ASPM_L0S | PCI_EXP_LNKCTL_ASPM_L1);
+		pcie_lnkctl_clear_and_set(pdev, PCI_EXP_LNKCTL_ASPM_L0S |
+						PCI_EXP_LNKCTL_ASPM_L1, 0);
 
 		/*
 		 * Both upstream and downstream PCIe components should
 		 * have the same ASPM settings.
 		 */
-		pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
-			PCI_EXP_LNKCTL_ASPM_L0S | PCI_EXP_LNKCTL_ASPM_L1);
+		pcie_lnkctl_clear_and_set(parent,
+					  PCI_EXP_LNKCTL_ASPM_L0S |
+					  PCI_EXP_LNKCTL_ASPM_L1, 0);
 
 		ath_info(common, "Disabling ASPM since BTCOEX is enabled\n");
 		return;
-- 
2.30.2

[PATCH 10/17] mt76: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/mediatek/mt76/pci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/pci.c b/drivers/net/wireless/mediatek/mt76/pci.c
index 4c1c159fbb62..8c6444f5fac3 100644
--- a/drivers/net/wireless/mediatek/mt76/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/pci.c
@@ -39,9 +39,8 @@ void mt76_pci_disable_aspm(struct pci_dev *pdev)
 	/* 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_conf);
+	pcie_lnkctl_clear_and_set(pdev, aspm_conf, 0);
 	if (parent)
-		pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
-					   aspm_conf);
+		pcie_lnkctl_clear_and_set(parent, aspm_conf, 0);
 }
 EXPORT_SYMBOL_GPL(mt76_pci_disable_aspm);
-- 
2.30.2

[PATCH 11/17] Bluetooth: hci_bcm4377: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/bluetooth/hci_bcm4377.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm4377.c b/drivers/bluetooth/hci_bcm4377.c
index 19ad0e788646..e2b489f678d9 100644
--- a/drivers/bluetooth/hci_bcm4377.c
+++ b/drivers/bluetooth/hci_bcm4377.c
@@ -2232,8 +2232,7 @@ static void bcm4377_disable_aspm(struct bcm4377_data *bcm4377)
 	 * or if the BIOS hasn't handed over control to us. We must *always*
 	 * disable ASPM for this device due to hardware errata though.
 	 */
-	pcie_capability_clear_word(bcm4377->pdev, PCI_EXP_LNKCTL,
-				   PCI_EXP_LNKCTL_ASPMC);
+	pcie_lnkctl_clear_and_set(bcm4377->pdev, PCI_EXP_LNKCTL_ASPMC, 0);
 }
 
 static void bcm4377_pci_free_irq_vectors(void *data)
-- 
2.30.2

[PATCH 12/17] misc: rtsx: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH 13/17] net/tg3: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH 15/17] wifi: ath11k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/ath/ath11k/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 7b33731a50ee..d0885d30dcbc 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -581,8 +581,8 @@ static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
 		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
 
 	/* disable L0s and L1 */
-	pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-				   ab_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
+	pcie_lnkctl_clear_and_set(ab_pci->pdev,
+				  ab_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC, 0);
 
 	set_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags);
 }
@@ -590,8 +590,8 @@ static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
 static void ath11k_pci_aspm_restore(struct ath11k_pci *ab_pci)
 {
 	if (test_and_clear_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags))
-		pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-					   ab_pci->link_ctl);
+		pcie_lnkctl_clear_and_set(ab_pci->pdev, 0,
+					  ab_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
 }
 
 static int ath11k_pci_power_up(struct ath11k_base *ab)
-- 
2.30.2

[PATCH 16/17] wifi: ath12k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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 <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/ath/ath12k/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
index 9f174daf324c..fa88a0b88520 100644
--- a/drivers/net/wireless/ath/ath12k/pci.c
+++ b/drivers/net/wireless/ath/ath12k/pci.c
@@ -794,8 +794,8 @@ static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
 		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
 
 	/* disable L0s and L1 */
-	pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-				   ab_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
+	pcie_lnkctl_clear_and_set(ab_pci->pdev,
+				  ab_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC, 0);
 
 	set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
 }
@@ -803,8 +803,8 @@ static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
 static void ath12k_pci_aspm_restore(struct ath12k_pci *ab_pci)
 {
 	if (test_and_clear_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags))
-		pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-					   ab_pci->link_ctl);
+		pcie_lnkctl_clear_and_set(ab_pci->pdev, 0,
+					  ab_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
 }
 
 static void ath12k_pci_kill_tasklets(struct ath12k_base *ab)
-- 
2.30.2

[PATCH 17/17] wifi: ath10k: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

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. Convert one of the
writes to only touch PCI_EXP_LNKCTL_ASPMC field which the driver itself
has been changing, leave the other fields untouched.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a7f44f6335fb..d18dfb495194 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1963,8 +1963,8 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 	ath10k_pci_irq_enable(ar);
 	ath10k_pci_rx_post(ar);
 
-	pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
-				   ar_pci->link_ctl);
+	pcie_lnkctl_clear_and_set(ar_pci->pdev, 0,
+				  ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
 
 	return 0;
 }
@@ -2821,8 +2821,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
 
 	pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
 				  &ar_pci->link_ctl);
-	pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
-				   ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
+	pcie_lnkctl_clear_and_set(ar_pci->pdev,
+				  ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC, 0);
 
 	/*
 	 * Bring the target up cleanly.
-- 
2.30.2

Re: [PATCH 06/17] IB/hfi1: Use pcie_lnkctl{,2}_clear_and_set() for changing LNKCTL{,2}

[Dean Luick]

On 5/11/2023 8:14 AM, Ilpo Järvinen wrote:
> 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 <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/infiniband/hw/hfi1/aspm.c | 16 ++++++----------
>  drivers/infiniband/hw/hfi1/pcie.c | 28 ++++++----------------------
>  2 files changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hfi1/aspm.c b/drivers/infiniband/hw/hfi1/aspm.c
> index a3c53be4072c..d3f3b7e9b833 100644
> --- a/drivers/infiniband/hw/hfi1/aspm.c
> +++ b/drivers/infiniband/hw/hfi1/aspm.c
> @@ -66,12 +66,10 @@ static void aspm_hw_enable_l1(struct hfi1_devdata *dd)
>               return;
>
>       /* Enable ASPM L1 first in upstream component and then downstream */
> -     pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> -                                        PCI_EXP_LNKCTL_ASPMC,
> -                                        PCI_EXP_LNKCTL_ASPM_L1);
> -     pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
> -                                        PCI_EXP_LNKCTL_ASPMC,
> -                                        PCI_EXP_LNKCTL_ASPM_L1);
> +     pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC,
> +                               PCI_EXP_LNKCTL_ASPM_L1);
> +     pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC,
> +                               PCI_EXP_LNKCTL_ASPM_L1);
>  }
>
>  void aspm_hw_disable_l1(struct hfi1_devdata *dd)
> @@ -79,11 +77,9 @@ void aspm_hw_disable_l1(struct hfi1_devdata *dd)
>       struct pci_dev *parent = dd->pcidev->bus->self;
>
>       /* Disable ASPM L1 first in downstream component and then upstream */
> -     pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
> -                                        PCI_EXP_LNKCTL_ASPMC, 0x0);
> +     pcie_lnkctl_clear_and_set(dd->pcidev, PCI_EXP_LNKCTL_ASPMC, 0);
>       if (parent)
> -             pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> -                                                PCI_EXP_LNKCTL_ASPMC, 0x0);
> +             pcie_lnkctl_clear_and_set(parent, PCI_EXP_LNKCTL_ASPMC, 0);
>  }
>
>  static  void aspm_enable(struct hfi1_devdata *dd)
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
> index 08732e1ac966..fe7324d38d64 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -1212,14 +1212,10 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
>                   (u32)lnkctl2);
>       /* only write to parent if target is not as high as ours */
>       if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
> -             lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
> -             lnkctl2 |= target_vector;
> -             dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> -                         (u32)lnkctl2);
> -             ret = pcie_capability_write_word(parent,
> -                                              PCI_EXP_LNKCTL2, lnkctl2);
> +             pcie_lnkctl2_clear_and_set(parent, PCI_EXP_LNKCTL2_TLS,
> +                                        target_vector);

You are missing an assignment to "ret" above.

-Dean
External recipient

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Bjorn Helgaas]

On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> A few places write LNKCTL and LNKCTL2 registers without proper
> concurrency control and this could result in losing the changes
> one of the writers intended to make.
> 
> Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> with LNKCTL and LNKCTL2. The concurrency control is provided using a
> spinlock in the struct pci_dev.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks for raising this issue!  Definitely looks like something that
needs attention.

> ---
>  drivers/pci/access.c | 14 ++++++++++++++
>  drivers/pci/probe.c  |  1 +
>  include/linux/pci.h  | 17 +++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 3c230ca3de58..d92a3daadd0c 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
>  }
>  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  
> +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> +					      u16 clear, u16 set)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&dev->cap_lock, flags);
> +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);

I didn't see the prior discussion with Lukas, so maybe this was
answered there, but is there any reason not to add locking to
pcie_capability_clear_and_set_word() and friends directly?  

It would be nice to avoid having to decide whether to use the locked
or unlocked versions.  It would also be nice to preserve the use of
PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
need for some of these patches, e.g., the use of
pcie_capability_clear_word(), where it's not obvious at the call site
why a change is needed.

Bjorn

>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
>  	if (pci_dev_is_disconnected(dev)) {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0b2826c4a832..0c14a283f1c7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2318,6 +2318,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>  		.end = -1,
>  	};
>  
> +	spin_lock_init(&dev->cap_lock);
>  #ifdef CONFIG_PCI_MSI
>  	raw_spin_lock_init(&dev->msi_lock);
>  #endif
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60b8772b5bd4..82faea085d95 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -467,6 +467,7 @@ struct pci_dev {
>  	pci_dev_flags_t dev_flags;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */
>  
> +	spinlock_t	cap_lock;		/* Protects RMW ops done with locked RMW capability accessors */
>  	u32		saved_config_space[16]; /* Config space saved at suspend time */
>  	struct hlist_head saved_cap_space;
>  	int		rom_attr_enabled;	/* Display of ROM attribute enabled? */
> @@ -1221,6 +1222,8 @@ int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
>  				       u16 clear, u16 set);
>  int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
>  					u32 clear, u32 set);
> +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> +					      u16 clear, u16 set);
>  
>  static inline int pcie_capability_set_word(struct pci_dev *dev, int pos,
>  					   u16 set)
> @@ -1246,6 +1249,20 @@ static inline int pcie_capability_clear_dword(struct pci_dev *dev, int pos,
>  	return pcie_capability_clear_and_set_dword(dev, pos, clear, 0);
>  }
>  
> +static inline int pcie_lnkctl_clear_and_set(struct pci_dev *dev, u16 clear,
> +					    u16 set)
> +{
> +	return pcie_capability_clear_and_set_word_locked(dev, PCI_EXP_LNKCTL,
> +							 clear, set);
> +}
> +
> +static inline int pcie_lnkctl2_clear_and_set(struct pci_dev *dev, u16 clear,
> +					    u16 set)
> +{
> +	return pcie_capability_clear_and_set_word_locked(dev, PCI_EXP_LNKCTL2,
> +							 clear, set);
> +}
> +
>  /* User-space driven config access */
>  int pci_user_read_config_byte(struct pci_dev *dev, int where, u8 *val);
>  int pci_user_read_config_word(struct pci_dev *dev, int where, u16 *val);
> -- 
> 2.30.2
> 

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Thu, 11 May 2023, Bjorn Helgaas wrote:

> On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > A few places write LNKCTL and LNKCTL2 registers without proper
> > concurrency control and this could result in losing the changes
> > one of the writers intended to make.
> > 
> > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > spinlock in the struct pci_dev.
> > 
> > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks for raising this issue!  Definitely looks like something that
> needs attention.
> 
> > ---
> >  drivers/pci/access.c | 14 ++++++++++++++
> >  drivers/pci/probe.c  |  1 +
> >  include/linux/pci.h  | 17 +++++++++++++++++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 3c230ca3de58..d92a3daadd0c 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> >  }
> >  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> >  
> > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > +					      u16 clear, u16 set)
> > +{
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> 
> I didn't see the prior discussion with Lukas, so maybe this was
> answered there, but is there any reason not to add locking to
> pcie_capability_clear_and_set_word() and friends directly?  
>
> It would be nice to avoid having to decide whether to use the locked
> or unlocked versions.  It would also be nice to preserve the use of
> PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
> need for some of these patches, e.g., the use of
> pcie_capability_clear_word(), where it's not obvious at the call site
> why a change is needed.

There wasn't that big discussion about it (internally). I brought both
alternatives up and Lukas just said he didn't know what's the best 
approach (+ gave a weak nudge towards the separate accessor so I went 
with it to make forward progress). Based on that I don't think he had a 
strong opinion on it.

I'm certainly fine to just use it in the normal accessor functions that 
do RMW and add the locking there. It would certainly have to those good 
sides you mentioned.

-- 
 i.

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Bjorn Helgaas]

On Thu, May 11, 2023 at 08:35:48PM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Bjorn Helgaas wrote:
> 
> > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > A few places write LNKCTL and LNKCTL2 registers without proper
> > > concurrency control and this could result in losing the changes
> > > one of the writers intended to make.
> > > 
> > > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > > spinlock in the struct pci_dev.
> > > 
> > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > Thanks for raising this issue!  Definitely looks like something that
> > needs attention.
> > 
> > > ---
> > >  drivers/pci/access.c | 14 ++++++++++++++
> > >  drivers/pci/probe.c  |  1 +
> > >  include/linux/pci.h  | 17 +++++++++++++++++
> > >  3 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index 3c230ca3de58..d92a3daadd0c 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> > >  }
> > >  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> > >  
> > > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > > +					      u16 clear, u16 set)
> > > +{
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> > 
> > I didn't see the prior discussion with Lukas, so maybe this was
> > answered there, but is there any reason not to add locking to
> > pcie_capability_clear_and_set_word() and friends directly?  
> >
> > It would be nice to avoid having to decide whether to use the locked
> > or unlocked versions.  It would also be nice to preserve the use of
> > PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
> > need for some of these patches, e.g., the use of
> > pcie_capability_clear_word(), where it's not obvious at the call site
> > why a change is needed.
> 
> There wasn't that big discussion about it (internally). I brought both
> alternatives up and Lukas just said he didn't know what's the best 
> approach (+ gave a weak nudge towards the separate accessor so I went 
> with it to make forward progress). Based on that I don't think he had a 
> strong opinion on it.
> 
> I'm certainly fine to just use it in the normal accessor functions that 
> do RMW and add the locking there. It would certainly have to those good 
> sides you mentioned.

Let's start with that, then.

Many of these are ASPM-related updates that IMHO should not be in
drivers at all.  Drivers should use PCI core interfaces so the core
doesn't get confused.

Bjorn

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Heiner Kallweit]

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 <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  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)

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4487 bytes --]

On Thu, 11 May 2023, Bjorn Helgaas wrote:

> On Thu, May 11, 2023 at 08:35:48PM +0300, Ilpo Järvinen wrote:
> > On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > 
> > > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > > A few places write LNKCTL and LNKCTL2 registers without proper
> > > > concurrency control and this could result in losing the changes
> > > > one of the writers intended to make.
> > > > 
> > > > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > > > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > > > spinlock in the struct pci_dev.
> > > > 
> > > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > 
> > > Thanks for raising this issue!  Definitely looks like something that
> > > needs attention.
> > > 
> > > > ---
> > > >  drivers/pci/access.c | 14 ++++++++++++++
> > > >  drivers/pci/probe.c  |  1 +
> > > >  include/linux/pci.h  | 17 +++++++++++++++++
> > > >  3 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > > index 3c230ca3de58..d92a3daadd0c 100644
> > > > --- a/drivers/pci/access.c
> > > > +++ b/drivers/pci/access.c
> > > > @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> > > >  }
> > > >  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> > > >  
> > > > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > > > +					      u16 clear, u16 set)
> > > > +{
> > > > +	unsigned long flags;
> > > > +	int ret;
> > > > +
> > > > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > > > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > > > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> > > 
> > > I didn't see the prior discussion with Lukas, so maybe this was
> > > answered there, but is there any reason not to add locking to
> > > pcie_capability_clear_and_set_word() and friends directly?  
> > >
> > > It would be nice to avoid having to decide whether to use the locked
> > > or unlocked versions.  It would also be nice to preserve the use of
> > > PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
> > > need for some of these patches, e.g., the use of
> > > pcie_capability_clear_word(), where it's not obvious at the call site
> > > why a change is needed.
> > 
> > There wasn't that big discussion about it (internally). I brought both
> > alternatives up and Lukas just said he didn't know what's the best 
> > approach (+ gave a weak nudge towards the separate accessor so I went 
> > with it to make forward progress). Based on that I don't think he had a 
> > strong opinion on it.
> > 
> > I'm certainly fine to just use it in the normal accessor functions that 
> > do RMW and add the locking there. It would certainly have to those good 
> > sides you mentioned.
> 
> Let's start with that, then.
> 
> Many of these are ASPM-related updates that IMHO should not be in
> drivers at all.  Drivers should use PCI core interfaces so the core
> doesn't get confused.

Ah, yes. I forgot to mention it in the cover letter but I noticed that 
some of those seem to be workarounds for the cases where core refuses to 
disable ASPM. Some sites even explicit have a comment about that after 
the call to pci_disable_link_state():

static void bcm4377_disable_aspm(struct bcm4377_data *bcm4377)
{
        pci_disable_link_state(bcm4377->pdev,
                               PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);

        /*
         * pci_disable_link_state can fail if either CONFIG_PCIEASPM is disabled
         * or if the BIOS hasn't handed over control to us. We must *always*
         * disable ASPM for this device due to hardware errata though.
         */
        pcie_capability_clear_word(bcm4377->pdev, PCI_EXP_LNKCTL,
                                   PCI_EXP_LNKCTL_ASPMC);
}

That kinda feels something that would want a force disable quirk that is 
reliable. There are quirks for some devices which try to disable it but 
could fail for reasons mentioned in that comment. (But I'd prefer to make 
another series out of it rather than putting it into this one.)

It might even be that some drivers don't even bother to make the 
pci_disable_link_state() call because it isn't reliable enough.


-- 
 i.

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

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.

Re: [PATCH 06/17] IB/hfi1: Use pcie_lnkctl{,2}_clear_and_set() for changing LNKCTL{,2}

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

On Thu, 11 May 2023, Dean Luick wrote:

> On 5/11/2023 8:14 AM, Ilpo Järvinen wrote:
> > 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 <lukas@wunner.de>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---

> > diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
> > index 08732e1ac966..fe7324d38d64 100644
> > --- a/drivers/infiniband/hw/hfi1/pcie.c
> > +++ b/drivers/infiniband/hw/hfi1/pcie.c
> > @@ -1212,14 +1212,10 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
> >                   (u32)lnkctl2);
> >       /* only write to parent if target is not as high as ours */
> >       if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
> > -             lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
> > -             lnkctl2 |= target_vector;
> > -             dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> > -                         (u32)lnkctl2);
> > -             ret = pcie_capability_write_word(parent,
> > -                                              PCI_EXP_LNKCTL2, lnkctl2);
> > +             pcie_lnkctl2_clear_and_set(parent, PCI_EXP_LNKCTL2_TLS,
> > +                                        target_vector);
> 
> You are missing an assignment to "ret" above.

Thanks for noticing, I'll fix it in the next version.

-- 
 i.

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Lukas Wunner]

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

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Lukas Wunner]

On Thu, May 11, 2023 at 10:58:40PM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > Many of these are ASPM-related updates that IMHO should not be in
> > drivers at all.  Drivers should use PCI core interfaces so the core
> > doesn't get confused.
> 
> Ah, yes. I forgot to mention it in the cover letter but I noticed that 
> some of those seem to be workarounds for the cases where core refuses to 
> disable ASPM. Some sites even explicit have a comment about that after 
> the call to pci_disable_link_state():
[...]
> That kinda feels something that would want a force disable quirk that is 
> reliable. There are quirks for some devices which try to disable it but 
> could fail for reasons mentioned in that comment. (But I'd prefer to make 
> another series out of it rather than putting it into this one.)

I'm wondering if it's worth cleaning up ASPM handling in drivers first
as the locking issue may then largely solve itself.  The locking could
probably be kept internal to ASPM core code then.

Thanks,

Lukas

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Lukas Wunner]

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/

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Heiner Kallweit]

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!

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

[Heiner Kallweit]

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

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Lukas Wunner]

On Thu, May 11, 2023 at 10:55:06AM -0500, Bjorn Helgaas wrote:
> On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > +					      u16 clear, u16 set)
> > +{
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> 
> I didn't see the prior discussion with Lukas, so maybe this was
> answered there, but is there any reason not to add locking to
> pcie_capability_clear_and_set_word() and friends directly?
> 
> It would be nice to avoid having to decide whether to use the locked
> or unlocked versions.

I think we definitely want to also offer lockless accessors which
can be used in hotpaths such as interrupt handlers if the accessed
registers don't need any locking (e.g. because there are no concurrent
accesses).

So the relatively lean approach chosen here which limits locking to
Link Control and Link Control 2, but allows future expansion to other
registers as well, seemed reasonable to me.

Thanks,

Lukas

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

On Thu, 11 May 2023, Lukas Wunner wrote:

> On Thu, May 11, 2023 at 10:58:40PM +0300, Ilpo Järvinen wrote:
> > On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > > Many of these are ASPM-related updates that IMHO should not be in
> > > drivers at all.  Drivers should use PCI core interfaces so the core
> > > doesn't get confused.
> > 
> > Ah, yes. I forgot to mention it in the cover letter but I noticed that 
> > some of those seem to be workarounds for the cases where core refuses to 
> > disable ASPM. Some sites even explicit have a comment about that after 
> > the call to pci_disable_link_state():
> [...]
> > That kinda feels something that would want a force disable quirk that is 
> > reliable. There are quirks for some devices which try to disable it but 
> > could fail for reasons mentioned in that comment. (But I'd prefer to make 
> > another series out of it rather than putting it into this one.)
> 
> I'm wondering if it's worth cleaning up ASPM handling in drivers first
> as the locking issue may then largely solve itself.  The locking could
> probably be kept internal to ASPM core code then.

For some part yes, but at least those copy-pasted gpu setup codes did some 
other things too.

In any case, it would go against some earlier policy decision:

/**
 * pci_disable_link_state - Disable device's link state, so the link will
 * never enter specific states.  Note that if the BIOS didn't grant ASPM
 * control to the OS, this does nothing because we can't touch the LNKCTL
 * register. Returns 0 or a negative errno.

Is it fine to make core capable of violating that policy?

One question before I trying to come up something is when PCIEASPM is =n, 
should I provide some simple function that just does the LNKCTL write to 
disable it? And another thing is the existing quirks, should they be 
kept depending on the existing behavior or not?


-- 
 i.

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Bjorn Helgaas]

[+cc Emmanuel, Rafael, Heiner, ancient ASPM history]

On Thu, May 11, 2023 at 10:58:40PM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > On Thu, May 11, 2023 at 08:35:48PM +0300, Ilpo Järvinen wrote:
> > > On Thu, 11 May 2023, Bjorn Helgaas wrote:
> > > > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > > > A few places write LNKCTL and LNKCTL2 registers without proper
> > > > > concurrency control and this could result in losing the changes
> > > > > one of the writers intended to make.
> > > > > 
> > > > > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > > > > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > > > > spinlock in the struct pci_dev.
> ...

[beginning of thread is
https://lore.kernel.org/r/20230511131441.45704-1-ilpo.jarvinen@linux.intel.com;
context here is that several drivers clear ASPM config directly,
probably because pci_disable_link_state() doesn't always do it]

> > Many of these are ASPM-related updates that IMHO should not be in
> > drivers at all.  Drivers should use PCI core interfaces so the core
> > doesn't get confused.
> 
> Ah, yes. I forgot to mention it in the cover letter but I noticed that 
> some of those seem to be workarounds for the cases where core refuses to 
> disable ASPM. Some sites even explicit have a comment about that after 
> the call to pci_disable_link_state():
> 
> static void bcm4377_disable_aspm(struct bcm4377_data *bcm4377)
> {
>         pci_disable_link_state(bcm4377->pdev,
>                                PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> 
>         /*
>          * pci_disable_link_state can fail if either CONFIG_PCIEASPM is disabled
>          * or if the BIOS hasn't handed over control to us. We must *always*
>          * disable ASPM for this device due to hardware errata though.
>          */
>         pcie_capability_clear_word(bcm4377->pdev, PCI_EXP_LNKCTL,
>                                    PCI_EXP_LNKCTL_ASPMC);
> }
> 
> That kinda feels something that would want a force disable quirk that is 
> reliable. There are quirks for some devices which try to disable it but 
> could fail for reasons mentioned in that comment. (But I'd prefer to make 
> another series out of it rather than putting it into this one.)
> 
> It might even be that some drivers don't even bother to make the 
> pci_disable_link_state() call because it isn't reliable enough.

Yeah, I noticed that this is problematic.

We went round and round about this ten years ago [1], which resulted
in https://git.kernel.org/linus/2add0ec14c25 ("PCI/ASPM: Warn when
driver asks to disable ASPM, but we can't do it").

I'm not 100% convinced by that anymore.  It's true that if firmware
retains control of the PCIe capability, the OS is technically not
allowed to write to it, and it's conceivable that even a locked OS
update could collide with some SMI or something that also writes to
it.

I can certainly imagine that firmware might know that *enabling* ASPM
might break because of signal integrity issues or something.  It seems
less likely that *disabling* ASPM would break something, but Rafael [2]
and Matthew [3] rightly pointed out that there is some risk.

But the current situation, where pci_disable_link_state() does nothing
if CONFIG_PCIEASPM is unset or if _OSC says firmware owns it, leads to
drivers doing it directly anyway.  I'm not sure that's better than
making pci_disable_link_state() work 100% of the time, regardless of
CONFIG_PCIEASPM and _OSC.  At least then the PCI core would know
what's going on.

Bjorn

[1] https://lore.kernel.org/all/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com/
[2] https://lore.kernel.org/all/1725435.3DlCxYF2FV@vostro.rjw.lan/
[3] https://lore.kernel.org/all/1368303730.2425.47.camel@x230/

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Bjorn Helgaas]

[+cc Emmanuel, Rafael, Heiner]

On Thu, May 11, 2023 at 11:28:05PM +0300, Ilpo Järvinen wrote:
> ...
> One question before I trying to come up something is when PCIEASPM is =n, 
> should I provide some simple function that just does the LNKCTL write to 
> disable it?

The current pci_disable_link_state() stub when CONFIG_PCIEASPM is
unset seems clearly wrong.  In fact, it returns *success* when it
actually did nothing.

I think it should probably clear ASPM Control, at least when the OS
has ownership via _OSC.  I kind of think it should do that independent
of _OSC, but that depends on the conversation at [1].

Bjorn

[1] https://lore.kernel.org/r/ZF1dsvJYYnl8Wv0v@bhelgaas

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

On Thu, 11 May 2023, Lukas Wunner wrote:

> On Thu, May 11, 2023 at 10:55:06AM -0500, Bjorn Helgaas wrote:
> > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > > +					      u16 clear, u16 set)
> > > +{
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> > 
> > I didn't see the prior discussion with Lukas, so maybe this was
> > answered there, but is there any reason not to add locking to
> > pcie_capability_clear_and_set_word() and friends directly?
> > 
> > It would be nice to avoid having to decide whether to use the locked
> > or unlocked versions.
> 
> I think we definitely want to also offer lockless accessors which
> can be used in hotpaths such as interrupt handlers if the accessed
> registers don't need any locking (e.g. because there are no concurrent
> accesses).
> 
> So the relatively lean approach chosen here which limits locking to
> Link Control and Link Control 2, but allows future expansion to other
> registers as well, seemed reasonable to me.

Hi Lukas,

I went through every single use of these functions in the mainline tree 
excluding LNKCTL/LNKCTL2 ones which will be having the lock anyway:

- pcie_capability_clear_and_set_*
- pcie_capability_set_*
- pcie_capability_clear_*

Everything outside of drivers/pci/ is dev init or dev reset related.

Almost all uses inside drivers/pci/ are init/configure/scan/PCI_FIXUP/pci_flr 
or suspend/resume related. With these exceptions:

->set_attention_status() drivers/pci/hotplug/pnv_php.c: pcie_capability_clear_and_set_word(bridge, PCI_EXP_SLTCTL,
spinlock + work (from pme.c) drivers/pci/pci.c: pcie_capability_set_dword(dev, PCI_EXP_RTSTA
spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_set_word(dev, PCI_EXP_RTCTL,
spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_clear_word(dev, PCI_EXP_RTCTL,

So the only case which seems relevant to your concern are those in
drivers/pci/pcie/pme.c which already takes a spinlock so it's not lockless 
as is.

What's more important though, isn't it possible that AER and PME RMW
PCI_EXP_RTCTL at the same time so it would need this RMW locking too 
despite the pme internal spinlock?

Do you still feel there's a need to differentiate this per capability 
given all the information above?


There could of course be open-coded capability RMW ops outside of the ones 
I checked but I suspect the conclusion would still remain pretty much the 
same.


-- 
 i.

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Lukas Wunner]

On Fri, May 12, 2023 at 11:25:32AM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Lukas Wunner wrote:
> > On Thu, May 11, 2023 at 10:55:06AM -0500, Bjorn Helgaas wrote:
> > > I didn't see the prior discussion with Lukas, so maybe this was
> > > answered there, but is there any reason not to add locking to
> > > pcie_capability_clear_and_set_word() and friends directly?
> > > 
> > > It would be nice to avoid having to decide whether to use the locked
> > > or unlocked versions.
> > 
> > I think we definitely want to also offer lockless accessors which
> > can be used in hotpaths such as interrupt handlers if the accessed
> > registers don't need any locking (e.g. because there are no concurrent
> > accesses).
> > 
> > So the relatively lean approach chosen here which limits locking to
> > Link Control and Link Control 2, but allows future expansion to other
> > registers as well, seemed reasonable to me.
> 
> I went through every single use of these functions in the mainline tree 
> excluding LNKCTL/LNKCTL2 ones which will be having the lock anyway:
> 
> - pcie_capability_clear_and_set_*
> - pcie_capability_set_*
> - pcie_capability_clear_*

We're also performing RMW through pcie_capability_read_word() +
pcie_capability_write_word() combos, see drivers/pci/hotplug/pciehp_hpc.c
for examples.


> Do you still feel there's a need to differentiate this per capability 
> given all the information above?

What I think is unnecessary and counterproductive is to add wholesale
locking of any access to the PCI Express Capability Structure.

It's fine to have a single spinlock, but I'd suggest only using it
for registers which are actually accessed concurrently by multiple
places in the kernel.


> spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_set_word(dev, PCI_EXP_RTCTL,
> spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_clear_word(dev, PCI_EXP_RTCTL,
[...]
> What's more important though, isn't it possible that AER and PME RMW
> PCI_EXP_RTCTL at the same time so it would need this RMW locking too 
> despite the pme internal spinlock?

Yes that looks broken, so RTCTL would be another register besides
LNKCTL and LNKCTL2 that needs protection, good catch.

Thanks,

Lukas

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4118 bytes --]

On Sun, 14 May 2023, Lukas Wunner wrote:

> On Fri, May 12, 2023 at 11:25:32AM +0300, Ilpo Järvinen wrote:
> > On Thu, 11 May 2023, Lukas Wunner wrote:
> > > On Thu, May 11, 2023 at 10:55:06AM -0500, Bjorn Helgaas wrote:
> > > > I didn't see the prior discussion with Lukas, so maybe this was
> > > > answered there, but is there any reason not to add locking to
> > > > pcie_capability_clear_and_set_word() and friends directly?
> > > > 
> > > > It would be nice to avoid having to decide whether to use the locked
> > > > or unlocked versions.
> > > 
> > > I think we definitely want to also offer lockless accessors which
> > > can be used in hotpaths such as interrupt handlers if the accessed
> > > registers don't need any locking (e.g. because there are no concurrent
> > > accesses).
> > > 
> > > So the relatively lean approach chosen here which limits locking to
> > > Link Control and Link Control 2, but allows future expansion to other
> > > registers as well, seemed reasonable to me.
> > 
> > I went through every single use of these functions in the mainline tree 
> > excluding LNKCTL/LNKCTL2 ones which will be having the lock anyway:
> > 
> > - pcie_capability_clear_and_set_*
> > - pcie_capability_set_*
> > - pcie_capability_clear_*
> 
> We're also performing RMW through pcie_capability_read_word() +
> pcie_capability_write_word() combos, see drivers/pci/hotplug/pciehp_hpc.c
> for examples.

That's why I said there could be other RMW operations outside of what
I carefully looked at. It, however, does not mean I didn't take any look 
at those.

But since brought it up, lets go through this case with
drivers/pci/hotplug/pciehp_hpc.c, it won't change anything:

All PCI_EXP_SLTSTA ones looked not real RMW but ACK bits type of writes
(real RMW = preverse other bits vs ACK write = other bits are written as 
zeros). Using RMW accessors would need an odd construct such as this
(and pcie_capability_set/clear_word() would be plain wrong):
	pcie_capability_clear_and_set_word(dev, PCI_EXP_SLTSTA,
					   ~PCI_EXP_SLTSTA_CC,
					   PCI_EXP_SLTSTA_CC);

PCI_EXP_SLTCTL write is protected by a mutex, it doesn't look something 
that matches your initial concern about "hot paths (e.g. interrupt 
handlers)".

In general, outside of drivers/pci/hotplug there are not that many 
capability writes (beyond LNKCTL/LNKCTL2 and now also RTCTL). None of 
those seem hot paths.

> > Do you still feel there's a need to differentiate this per capability 
> > given all the information above?
> 
> What I think is unnecessary and counterproductive is to add wholesale
> locking of any access to the PCI Express Capability Structure.
> 
> It's fine to have a single spinlock, but I'd suggest only using it
> for registers which are actually accessed concurrently by multiple
> places in the kernel.

While it does feel entirely unnecessary layer of complexity to me, it would 
be possible to rename the original pcie_capability_clear_and_set_word() to 
pcie_capability_clear_and_set_word_unlocked() and add this into 
include/linux/pci.h:

static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
					int pos, u16 clear, u16 set)
{
	if (pos == PCI_EXP_LNKCTL || pos == PCI_EXP_LNKCTL2 ||
	    pos == PCI_EXP_RTCTL)
		pcie_capability_clear_and_set_word_locked(...);
	else
		pcie_capability_clear_and_set_word_unlocked(...);
}

It would keep the interface exactly the same but protect only a selectable 
set of registers. As pos is always a constant, the compiler should be able 
to optimize all the dead code away.

Would that be ok then?

-- 
 i.


> > spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_set_word(dev, PCI_EXP_RTCTL,
> > spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_clear_word(dev, PCI_EXP_RTCTL,
> [...]
> > What's more important though, isn't it possible that AER and PME RMW
> > PCI_EXP_RTCTL at the same time so it would need this RMW locking too 
> > despite the pme internal spinlock?
> 
> Yes that looks broken, so RTCTL would be another register besides
> LNKCTL and LNKCTL2 that needs protection, good catch.
> 
> Thanks,
> 
> Lukas
> 

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Bjorn Helgaas]

On Mon, May 15, 2023 at 02:59:42PM +0300, Ilpo Järvinen wrote:
> On Sun, 14 May 2023, Lukas Wunner wrote:
> > On Fri, May 12, 2023 at 11:25:32AM +0300, Ilpo Järvinen wrote:
> > > On Thu, 11 May 2023, Lukas Wunner wrote:
> > > > On Thu, May 11, 2023 at 10:55:06AM -0500, Bjorn Helgaas wrote:
> > > > > I didn't see the prior discussion with Lukas, so maybe this was
> > > > > answered there, but is there any reason not to add locking to
> > > > > pcie_capability_clear_and_set_word() and friends directly?
> > > > > 
> > > > > It would be nice to avoid having to decide whether to use the locked
> > > > > or unlocked versions.
> > > > 
> > > > I think we definitely want to also offer lockless accessors which
> > > > can be used in hotpaths such as interrupt handlers if the accessed
> > > > registers don't need any locking (e.g. because there are no concurrent
> > > > accesses).
> > > > ...

> All PCI_EXP_SLTSTA ones looked not real RMW but ACK bits type of writes

PCI_EXP_SLTSTA, PCI_EXP_LNKSTA, etc are typically RW1C and do not need
the usual RMW locking (which I think is what you were saying).

> > ...
> > What I think is unnecessary and counterproductive is to add wholesale
> > locking of any access to the PCI Express Capability Structure.
> > 
> > It's fine to have a single spinlock, but I'd suggest only using it
> > for registers which are actually accessed concurrently by multiple
> > places in the kernel.
> 
> While it does feel entirely unnecessary layer of complexity to me, it would 
> be possible to rename the original pcie_capability_clear_and_set_word() to 
> pcie_capability_clear_and_set_word_unlocked() and add this into 
> include/linux/pci.h:
> 
> static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
> 					int pos, u16 clear, u16 set)
> {
> 	if (pos == PCI_EXP_LNKCTL || pos == PCI_EXP_LNKCTL2 ||
> 	    pos == PCI_EXP_RTCTL)
> 		pcie_capability_clear_and_set_word_locked(...);
> 	else
> 		pcie_capability_clear_and_set_word_unlocked(...);
> }
> 
> It would keep the interface exactly the same but protect only a selectable 
> set of registers. As pos is always a constant, the compiler should be able 
> to optimize all the dead code away.
> 
> Would that be ok then?

Sounds like you have a pretty strong opinion, Lukas, but I guess I
don't really understand the value of having locked and unlocked
variants of RMW accessors.  Config accesses are relatively slow and I
don't think they're used in performance-sensitive paths.  I would
expect the lock to be uncontended and cheap relative to the config
access itself, but I have no actual numbers to back up my speculation.
Is the performance win worth the extra complexity in callers?

Bjorn

Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

[Lukas Wunner]

On Mon, May 15, 2023 at 02:59:42PM +0300, Ilpo Järvinen wrote:
> While it does feel entirely unnecessary layer of complexity to me, it would 
> be possible to rename the original pcie_capability_clear_and_set_word() to 
> pcie_capability_clear_and_set_word_unlocked() and add this into 
> include/linux/pci.h:
> 
> static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
> 					int pos, u16 clear, u16 set)
> {
> 	if (pos == PCI_EXP_LNKCTL || pos == PCI_EXP_LNKCTL2 ||
> 	    pos == PCI_EXP_RTCTL)
> 		pcie_capability_clear_and_set_word_locked(...);
> 	else
> 		pcie_capability_clear_and_set_word_unlocked(...);
> }
> 
> It would keep the interface exactly the same but protect only a selectable 
> set of registers. As pos is always a constant, the compiler should be able 
> to optimize all the dead code away.

That's actually quite neat, I like it.  It documents clearly which
registers need protection because of concurrent RMWs and callers can't
do anything wrong.

Though I'd use a switch/case statement such that future additions
of registers that need protection are always just a clean, one-line
change.

Plus some kernel-doc or code comment to explain that certain
registers in the PCI Express Capability Structure are accessed
concurrently in a RMW fashion, hence require locking.

Since this protects specifically registers in the PCI Express
Capability, whose location is cached in struct pci_dev->pcie_cap,
I'm wondering if pcie_cap_lock is a clearer name.


> PCI_EXP_SLTCTL write is protected by a mutex, it doesn't look something 
> that matches your initial concern about "hot paths (e.g. interrupt 
> handlers)".

PCI_EXP_SLTCTL is definitely modified from the interrupt handler
pciehp_ist(), but one could argue that hotplug interrupts don't
usually occur *that* often.  (We've had interrupt storms though
from broken devices or ones with a shared interrupt etc.)

I guess I'm just generally worried about acquiring a lock that's
not necessary.  E.g. on boot, numerous config space accesses are
performed to enumerate and initialize devices and reducing concurrency
might slow down boot times.  It's just a risk that I'd recommend
to avoid if possible.

Thanks,

Lukas