[PATCH v2 0/9] PCI: Improve PCIe Capability RMW concurrency control

[PATCH v2 0/9] PCI: Improve PCIe Capability RMW concurrency control

[Ilpo Järvinen]

PCI Express Capability RMW accessors don't properly protect against
concurrent access. Link Control Register is written by a number of
things in the kernel in a RMW fashion 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. Similarly, Root Control Register can be concurrently
accessed by AER and PME.

Make pcie_capability_clear_and_set_word() (and other RMW accessors that
call it) to use a per device spinlock to protect the RMW operations to
the Capability Registers that require locking. Convert open-coded
LNKCTL RMW operations to use pcie_capability_clear_and_set_word() to
benefit from the locking.

As for LNKCTL2, all current users are safe so I left those changes to
be part of PCIe bandwidth control work I'll send separately later. It
will add LNKCTL2 RMW ops that require locking.

I went with the wrapper approach I suggested to Lukas even if I
personally feel it's over-engineering for a case that doesn't current
exist in the kernel code. That, however, is contained in the first
patch so it can be easily changed without affecting the other patches.

I felt better to do these as fixes first before altering code related
to duplicated ASPM code in drivers. Replacing the custom ASPM code in
drivers with calls to core will not necessarily yield the same states
as previously even if disabling ASPM becomes always possible because
re-enabling ASPM would still be prohibited (some drivers do disable and
enable ASPM back later). Thus, the ASPM "cleanups" would have higher
regression potential.

The series is based on top of the "PCI/ASPM: Handle link retraining
race" patch I sent earlier but is not yet applied.

v2:
- Keep the RMW ops caller API the same
- Make pcie_capability_clear_and_set_word() a wrapper that uses
  locked/unlocked variant based on the capability reg
- Extracted LNKCTL2 changes out from this series to keep this purely
  a series which fixes something (LNKCTL2 RMW lock is necessary only
  when PCIe BW control is introduced).
- Added Fixes tags (it's a bit rathole but yeah, they're there now).
- Renamed cap_lock to pcie_cap_lock
- Changed ath1* to clear the ASPMC field before setting it


Ilpo Järvinen (9):
  PCI: Add locking to RMW PCI Express Capability Register accessors
  PCI: pciehp: Use RMW accessors for changing LNKCTL
  PCI/ASPM: Use RMW accessors for changing LNKCTL
  drm/amdgpu: Use RMW accessors for changing LNKCTL
  drm/radeon: Use RMW accessors for changing LNKCTL
  net/mlx5: Use RMW accessors for changing LNKCTL
  wifi: ath11k: Use RMW accessors for changing LNKCTL
  wifi: ath12k: Use RMW accessors for changing LNKCTL
  wifi: ath10k: Use RMW accessors for changing LNKCTL

 drivers/gpu/drm/amd/amdgpu/cik.c              | 36 +++++------------
 drivers/gpu/drm/amd/amdgpu/si.c               | 36 +++++------------
 drivers/gpu/drm/radeon/cik.c                  | 36 +++++------------
 drivers/gpu/drm/radeon/si.c                   | 37 +++++-------------
 .../ethernet/mellanox/mlx5/core/fw_reset.c    |  9 +----
 drivers/net/wireless/ath/ath10k/pci.c         |  9 +++--
 drivers/net/wireless/ath/ath11k/pci.c         | 10 +++--
 drivers/net/wireless/ath/ath12k/pci.c         | 10 +++--
 drivers/pci/access.c                          | 20 ++++++++--
 drivers/pci/hotplug/pciehp_hpc.c              | 12 ++----
 drivers/pci/pcie/aspm.c                       | 39 ++++++++-----------
 drivers/pci/probe.c                           |  1 +
 include/linux/pci.h                           | 34 +++++++++++++++-
 13 files changed, 128 insertions(+), 161 deletions(-)

-- 
2.30.2

[PATCH v2 1/9] PCI: Add locking to RMW PCI Express Capability Register accessors

[Ilpo Järvinen]

Many places in the kernel write the Link Control and Root Control PCI
Express Capability Registers without proper concurrency control and
this could result in losing the changes one of the writers intended to
make.

Add pcie_cap_lock spinlock into the struct pci_dev and use it to
protect bit changes made in the RMW capability accessors. Protect only
a selected set of registers by differentiating the RMW accessor
internally to locked/unlocked variants using a wrapper which has the
same signature as pcie_capability_clear_and_set_word(). As the
Capability Register (pos) given to the wrapper is always a constant,
the compiler should be able to simplify all the dead-code away.

The RMW locking is only added to pcie_capability_clear_and_set_word()
because so far only the Link Control Register (ASPM, hotplug, various
drivers) and the Root Control Register (AER & PME) require RMW locking.

Fixes: c7f486567c1d ("PCI PM: PCIe PME root port service driver")
Fixes: f12eb72a268b ("PCI/ASPM: Use PCI Express Capability accessors")
Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
Fixes: affa48de8417 ("staging/rdma/hfi1: Add support for enabling/disabling PCIe ASPM")
Fixes: 849a9366cba9 ("misc: rtsx: Add support new chip rts5228 mmc: rtsx: Add support MMC_CAP2_NO_MMC")
Fixes: 3d1e7aa80d1c ("misc: rtsx: Use pcie_capability_clear_and_set_word() for PCI_EXP_LNKCTL")
Fixes: c0e5f4e73a71 ("misc: rtsx: Add support for RTS5261")
Fixes: 3df4fce739e2 ("misc: rtsx: separate aspm mode into MODE_REG and MODE_CFG")
Fixes: 121e9c6b5c4c ("misc: rtsx: modify and fix init_hw function")
Fixes: 19f3bd548f27 ("mfd: rtsx: Remove LCTLR defination")
Fixes: 773ccdfd9cc6 ("mfd: rtsx: Read vendor setting from config space")
Fixes: 8275b77a1513 ("mfd: rts5249: Add support for RTS5250S power saving")
Fixes: 5da4e04ae480 ("misc: rtsx: Add support for RTS5260")
Fixes: 0f49bfbd0f2e ("tg3: Use PCI Express Capability accessors")
Fixes: 5e7dfd0fb94a ("tg3: Prevent corruption at 10 / 100Mbps w CLKREQ")
Fixes: b726e493e8dc ("r8169: sync existing 8168 device hardware start sequences with vendor driver")
Fixes: e6de30d63eb1 ("r8169: more 8168dp support.")
Fixes: 8a06127602de ("Bluetooth: hci_bcm4377: Add new driver for BCM4377 PCIe boards")
Fixes: 6f461f6c7c96 ("e1000e: enable/disable ASPM L0s and L1 and ERT according to hardware errata")
Fixes: 1eae4eb2a1c7 ("e1000e: Disable L1 ASPM power savings for 82573 mobile variants")
Fixes: 8060e169e02f ("ath9k: Enable extended synch for AR9485 to fix L0s recovery issue")
Fixes: 69ce674bfa69 ("ath9k: do btcoex ASPM disabling at initialization time")
Fixes: f37f05503575 ("mt76: mt76x2e: disable pcie_aspm by default")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/access.c | 20 +++++++++++++++++---
 drivers/pci/probe.c  |  1 +
 include/linux/pci.h  | 34 ++++++++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 3c230ca3de58..0b2e90d2f04f 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -497,8 +497,8 @@ int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val)
 }
 EXPORT_SYMBOL(pcie_capability_write_dword);
 
-int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
-				       u16 clear, u16 set)
+int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
+						u16 clear, u16 set)
 {
 	int ret;
 	u16 val;
@@ -512,7 +512,21 @@ int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
 
 	return ret;
 }
-EXPORT_SYMBOL(pcie_capability_clear_and_set_word);
+EXPORT_SYMBOL(pcie_capability_clear_and_set_word_unlocked);
+
+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->pcie_cap_lock, flags);
+	ret = pcie_capability_clear_and_set_word_unlocked(dev, pos, clear, set);
+	spin_unlock_irqrestore(&dev->pcie_cap_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
 
 int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
 					u32 clear, u32 set)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 0b2826c4a832..53ac0d3287a8 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->pcie_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..ab7682ed172f 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	pcie_cap_lock;		/* Protects RMW ops in 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? */
@@ -1217,11 +1218,40 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val);
 int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val);
 int pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val);
 int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val);
-int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
-				       u16 clear, u16 set);
+int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
+						u16 clear, u16 set);
+int pcie_capability_clear_and_set_word_locked(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);
 
+/**
+ * pcie_capability_clear_and_set_word - RMW accessor for PCI Express Capability Registers
+ * @dev:	PCI device structure of the PCI Express device
+ * @pos:	PCI Express Capability Register
+ * @clear:	Clear bitmask
+ * @set:	Set bitmask
+ *
+ * Perform a Read-Modify-Write (RMW) operation using @clear and @set
+ * bitmasks on PCI Express Capability Register at @pos. Certain PCI Express
+ * Capability Registers are accessed concurrently in RMW fashion, hence
+ * require locking which is handled transparently to the caller.
+ */
+static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
+						     int pos,
+						     u16 clear, u16 set)
+{
+	switch (pos) {
+	case PCI_EXP_LNKCTL:
+	case PCI_EXP_RTCTL:
+		return pcie_capability_clear_and_set_word_locked(dev, pos,
+								 clear, set);
+	default:
+		return pcie_capability_clear_and_set_word_unlocked(dev, pos,
+								   clear, set);
+	}
+}
+
 static inline int pcie_capability_set_word(struct pci_dev *dev, int pos,
 					   u16 set)
 {
-- 
2.30.2

[PATCH v2 2/9] PCI: pciehp: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

As hotplug is not the only driver touching LNKCTL, use the RMW
capability accessor which handles concurrent changes correctly.

Fixes: 7f822999e12a ("PCI: pciehp: Add Disable/enable link functions")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/hotplug/pciehp_hpc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index f8c70115b691..26623e2884a3 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -332,17 +332,11 @@ 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_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
+					   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 v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

Don't assume that the device is fully under the control of ASPM and use
RMW capability accessors which do 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.

Fixes: 4ec73791a64b ("PCI: Work around Pericom PCIe-to-PCI bridge Retrain Link erratum")
Fixes: 86fa6a344209 ("PCI: Factor out pcie_retrain_link() function")
Fixes: 2a42d9dba784 ("PCIe: ASPM: Break out of endless loop waiting for PCI config bits to switch")
Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/pcie/aspm.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index dde1ef13d0d1..426fb0bd8e3a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -213,7 +213,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 +223,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_capability_set_word(parent, PCI_EXP_LNKCTL, 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_capability_clear_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
 	}
 
 	return pcie_wait_for_retrain(parent);
@@ -248,7 +244,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 +266,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 +286,16 @@ 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_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
+						   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_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_CCC,
+					   same_clock ? PCI_EXP_LNKCTL_CCC : 0);
 
 	if (pcie_retrain_link(link))
 		return;
@@ -312,9 +303,11 @@ 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_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_CCC,
+						   child_old_ccc[PCI_FUNC(child->devfn)]);
+	pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_CCC, parent_old_ccc);
 }
 
 /* Convert L0s latency encoding to ns */
-- 
2.30.2

[PATCH v2 4/9] drm/amdgpu: Use RMW accessors 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 bridge, the driver does not even own the
device it's changing the registers for.

Use RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value.

Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts")
Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/amd/amdgpu/cik.c | 36 +++++++++-----------------------
 drivers/gpu/drm/amd/amdgpu/si.c  | 36 +++++++++-----------------------
 2 files changed, 20 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index de6d10390ab2..9be6da37032a 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_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
+			pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
 
 			tmp = RREG32_PCIE(ixPCIE_LC_STATUS1);
 			max_lw = (tmp & PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >>
@@ -1637,21 +1628,14 @@ 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_capability_clear_and_set_word(root, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   bridge_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
+				pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   gpu_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
 
 				/* linkctl2 */
 				pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
diff --git a/drivers/gpu/drm/amd/amdgpu/si.c b/drivers/gpu/drm/amd/amdgpu/si.c
index 7f99e130acd0..fd34c2100bd9 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_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
+			pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
 
 			tmp = RREG32_PCIE(PCIE_LC_STATUS1);
 			max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
@@ -2331,21 +2322,14 @@ 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_clear_and_set_word(root, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   bridge_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
+				pcie_capability_clear_and_set_word(adev->pdev, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   gpu_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
 
 				pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
 							  &tmp16);
-- 
2.30.2

[PATCH v2 5/9] drm/radeon: Use RMW accessors 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 bridge, the driver does not even own the
device it's changing the registers for.

Use RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value.

Fixes: 8a7cd27679d0 ("drm/radeon/cik: add support for pcie gen1/2/3 switching")
Fixes: b9d305dfb66c ("drm/radeon: implement pcie gen2/3 support for SI")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/radeon/cik.c | 36 ++++++++++-------------------------
 drivers/gpu/drm/radeon/si.c  | 37 ++++++++++--------------------------
 2 files changed, 20 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index 5819737c21c6..a6f3c811ceb8 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_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
+			pcie_capability_set_word(rdev->pdev, PCI_EXP_LNKCTL, 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,21 +9582,14 @@ 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_capability_clear_and_set_word(root, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   bridge_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
+				pcie_capability_clear_and_set_word(rdev->pdev, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   gpu_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
 
 				/* linkctl2 */
 				pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index 8d5e4b25609d..a91012447b56 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_capability_set_word(root, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
+			pcie_capability_set_word(rdev->pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_HAWD);
 
 			tmp = RREG32_PCIE(PCIE_LC_STATUS1);
 			max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
@@ -7188,22 +7179,14 @@ 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_capability_clear_and_set_word(root, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   bridge_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
+				pcie_capability_clear_and_set_word(rdev->pdev, PCI_EXP_LNKCTL,
+								   PCI_EXP_LNKCTL_HAWD,
+								   gpu_cfg &
+								   PCI_EXP_LNKCTL_HAWD);
 
 				/* linkctl2 */
 				pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
-- 
2.30.2

[PATCH v2 6/9] net/mlx5: Use RMW accessors 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 RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value.

Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 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..f202150a5093 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_capability_set_word(bridge, PCI_EXP_LNKCTL, 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_capability_clear_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD);
 	if (err)
 		return err;
 
-- 
2.30.2

[PATCH v2 7/9] wifi: ath11k: Use RMW accessors 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 RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value. On restore, clear the ASPMC
field properly.

Fixes: e9603f4bdcc0 ("ath11k: pci: disable ASPM L0sLs before downloading firmware")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/net/wireless/ath/ath11k/pci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 7b33731a50ee..6ba4cef6b1c7 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_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+				   PCI_EXP_LNKCTL_ASPMC);
 
 	set_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags);
 }
@@ -590,8 +590,10 @@ 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_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPMC,
+						   ab_pci->link_ctl &
+						   PCI_EXP_LNKCTL_ASPMC);
 }
 
 static int ath11k_pci_power_up(struct ath11k_base *ab)
-- 
2.30.2

[PATCH v2 8/9] wifi: ath12k: Use RMW accessors 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 RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value. On restore, clear the ASPMC
field properly.

Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/net/wireless/ath/ath12k/pci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
index 9f174daf324c..e1e45eb50f3e 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_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+				   PCI_EXP_LNKCTL_ASPMC);
 
 	set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
 }
@@ -803,8 +803,10 @@ 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_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPMC,
+						   ab_pci->link_ctl &
+						   PCI_EXP_LNKCTL_ASPMC);
 }
 
 static void ath12k_pci_kill_tasklets(struct ath12k_base *ab)
-- 
2.30.2

[PATCH v2 9/9] wifi: ath10k: Use RMW accessors 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 RMW capability accessors which does proper locking to avoid losing
concurrent updates to the register value. On restore, clear the ASPMC
field properly.

Fixes: 76d870ed09ab ("ath10k: enable ASPM")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a7f44f6335fb..9275a672f90c 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1963,8 +1963,9 @@ 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_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
+					   PCI_EXP_LNKCTL_ASPMC,
+					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
 
 	return 0;
 }
@@ -2821,8 +2822,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_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
+				   PCI_EXP_LNKCTL_ASPMC);
 
 	/*
 	 * Bring the target up cleanly.
-- 
2.30.2

Re: [PATCH v2 8/9] wifi: ath12k: Use RMW accessors for changing LNKCTL

[Kalle Valo]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> 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 RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Acked-by: Kalle Valo <kvalo@kernel.org>

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 7/9] wifi: ath11k: Use RMW accessors for changing LNKCTL

[Kalle Valo]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> 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 RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: e9603f4bdcc0 ("ath11k: pci: disable ASPM L0sLs before downloading firmware")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Acked-by: Kalle Valo <kvalo@kernel.org>

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

[Kalle Valo]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> 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 RMW capability accessors which does proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Acked-by: Kalle Valo <kvalo@kernel.org>

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 6/9] net/mlx5: Use RMW accessors for changing LNKCTL

[Moshe Shemesh]

On 5/17/2023 1:52 PM, Ilpo Järvinen wrote:
> 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 RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value.
>
> Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event")
> Suggested-by: Lukas Wunner<lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen<ilpo.jarvinen@linux.intel.com>
> Cc:stable@vger.kernel.org
Reviewed-by: Moshe Shemesh <moshe@nvidia.com>

Thanks!

Re: [PATCH v2 1/9] PCI: Add locking to RMW PCI Express Capability Register accessors

[Rafael J. Wysocki]

On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Many places in the kernel write the Link Control and Root Control PCI
> Express Capability Registers without proper concurrency control and
> this could result in losing the changes one of the writers intended to
> make.
>
> Add pcie_cap_lock spinlock into the struct pci_dev and use it to
> protect bit changes made in the RMW capability accessors. Protect only
> a selected set of registers by differentiating the RMW accessor
> internally to locked/unlocked variants using a wrapper which has the
> same signature as pcie_capability_clear_and_set_word(). As the
> Capability Register (pos) given to the wrapper is always a constant,
> the compiler should be able to simplify all the dead-code away.
>
> The RMW locking is only added to pcie_capability_clear_and_set_word()
> because so far only the Link Control Register (ASPM, hotplug, various
> drivers) and the Root Control Register (AER & PME) require RMW locking.
>
> Fixes: c7f486567c1d ("PCI PM: PCIe PME root port service driver")
> Fixes: f12eb72a268b ("PCI/ASPM: Use PCI Express Capability accessors")
> Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
> Fixes: affa48de8417 ("staging/rdma/hfi1: Add support for enabling/disabling PCIe ASPM")
> Fixes: 849a9366cba9 ("misc: rtsx: Add support new chip rts5228 mmc: rtsx: Add support MMC_CAP2_NO_MMC")
> Fixes: 3d1e7aa80d1c ("misc: rtsx: Use pcie_capability_clear_and_set_word() for PCI_EXP_LNKCTL")
> Fixes: c0e5f4e73a71 ("misc: rtsx: Add support for RTS5261")
> Fixes: 3df4fce739e2 ("misc: rtsx: separate aspm mode into MODE_REG and MODE_CFG")
> Fixes: 121e9c6b5c4c ("misc: rtsx: modify and fix init_hw function")
> Fixes: 19f3bd548f27 ("mfd: rtsx: Remove LCTLR defination")
> Fixes: 773ccdfd9cc6 ("mfd: rtsx: Read vendor setting from config space")
> Fixes: 8275b77a1513 ("mfd: rts5249: Add support for RTS5250S power saving")
> Fixes: 5da4e04ae480 ("misc: rtsx: Add support for RTS5260")
> Fixes: 0f49bfbd0f2e ("tg3: Use PCI Express Capability accessors")
> Fixes: 5e7dfd0fb94a ("tg3: Prevent corruption at 10 / 100Mbps w CLKREQ")
> Fixes: b726e493e8dc ("r8169: sync existing 8168 device hardware start sequences with vendor driver")
> Fixes: e6de30d63eb1 ("r8169: more 8168dp support.")
> Fixes: 8a06127602de ("Bluetooth: hci_bcm4377: Add new driver for BCM4377 PCIe boards")
> Fixes: 6f461f6c7c96 ("e1000e: enable/disable ASPM L0s and L1 and ERT according to hardware errata")
> Fixes: 1eae4eb2a1c7 ("e1000e: Disable L1 ASPM power savings for 82573 mobile variants")
> Fixes: 8060e169e02f ("ath9k: Enable extended synch for AR9485 to fix L0s recovery issue")
> Fixes: 69ce674bfa69 ("ath9k: do btcoex ASPM disabling at initialization time")
> Fixes: f37f05503575 ("mt76: mt76x2e: disable pcie_aspm by default")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

No objections, so

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/pci/access.c | 20 +++++++++++++++++---
>  drivers/pci/probe.c  |  1 +
>  include/linux/pci.h  | 34 ++++++++++++++++++++++++++++++++--
>  3 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 3c230ca3de58..0b2e90d2f04f 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -497,8 +497,8 @@ int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val)
>  }
>  EXPORT_SYMBOL(pcie_capability_write_dword);
>
> -int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
> -                                      u16 clear, u16 set)
> +int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
> +                                               u16 clear, u16 set)
>  {
>         int ret;
>         u16 val;
> @@ -512,7 +512,21 @@ int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
>
>         return ret;
>  }
> -EXPORT_SYMBOL(pcie_capability_clear_and_set_word);
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_unlocked);
> +
> +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->pcie_cap_lock, flags);
> +       ret = pcie_capability_clear_and_set_word_unlocked(dev, pos, clear, set);
> +       spin_unlock_irqrestore(&dev->pcie_cap_lock, flags);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
>
>  int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
>                                         u32 clear, u32 set)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0b2826c4a832..53ac0d3287a8 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->pcie_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..ab7682ed172f 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      pcie_cap_lock;          /* Protects RMW ops in 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? */
> @@ -1217,11 +1218,40 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val);
>  int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val);
>  int pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val);
>  int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val);
> -int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
> -                                      u16 clear, u16 set);
> +int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
> +                                               u16 clear, u16 set);
> +int pcie_capability_clear_and_set_word_locked(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);
>
> +/**
> + * pcie_capability_clear_and_set_word - RMW accessor for PCI Express Capability Registers
> + * @dev:       PCI device structure of the PCI Express device
> + * @pos:       PCI Express Capability Register
> + * @clear:     Clear bitmask
> + * @set:       Set bitmask
> + *
> + * Perform a Read-Modify-Write (RMW) operation using @clear and @set
> + * bitmasks on PCI Express Capability Register at @pos. Certain PCI Express
> + * Capability Registers are accessed concurrently in RMW fashion, hence
> + * require locking which is handled transparently to the caller.
> + */
> +static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
> +                                                    int pos,
> +                                                    u16 clear, u16 set)
> +{
> +       switch (pos) {
> +       case PCI_EXP_LNKCTL:
> +       case PCI_EXP_RTCTL:
> +               return pcie_capability_clear_and_set_word_locked(dev, pos,
> +                                                                clear, set);
> +       default:
> +               return pcie_capability_clear_and_set_word_unlocked(dev, pos,
> +                                                                  clear, set);
> +       }
> +}
> +
>  static inline int pcie_capability_set_word(struct pci_dev *dev, int pos,
>                                            u16 set)
>  {
> --
> 2.30.2
>

Re: [PATCH v2 2/9] PCI: pciehp: Use RMW accessors for changing LNKCTL

[Rafael J. Wysocki]

On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> As hotplug is not the only driver touching LNKCTL, use the RMW
> capability accessor which handles concurrent changes correctly.
>
> Fixes: 7f822999e12a ("PCI: pciehp: Add Disable/enable link functions")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index f8c70115b691..26623e2884a3 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -332,17 +332,11 @@ 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_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
> +                                          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
>

Re: [PATCH v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

[Rafael J. Wysocki]

On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Don't assume that the device is fully under the control of ASPM and use
> RMW capability accessors which do 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.
>
> Fixes: 4ec73791a64b ("PCI: Work around Pericom PCIe-to-PCI bridge Retrain Link erratum")
> Fixes: 86fa6a344209 ("PCI: Factor out pcie_retrain_link() function")
> Fixes: 2a42d9dba784 ("PCIe: ASPM: Break out of endless loop waiting for PCI config bits to switch")
> Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/pci/pcie/aspm.c | 39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index dde1ef13d0d1..426fb0bd8e3a 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -213,7 +213,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 +223,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_capability_set_word(parent, PCI_EXP_LNKCTL, 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_capability_clear_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
>         }
>
>         return pcie_wait_for_retrain(parent);
> @@ -248,7 +244,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 +266,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 +286,16 @@ 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_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> +                                                  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_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> +                                          PCI_EXP_LNKCTL_CCC,
> +                                          same_clock ? PCI_EXP_LNKCTL_CCC : 0);
>
>         if (pcie_retrain_link(link))
>                 return;
> @@ -312,9 +303,11 @@ 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_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> +                                                  PCI_EXP_LNKCTL_CCC,
> +                                                  child_old_ccc[PCI_FUNC(child->devfn)]);
> +       pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> +                                          PCI_EXP_LNKCTL_CCC, parent_old_ccc);
>  }
>
>  /* Convert L0s latency encoding to ns */
> --
> 2.30.2
>

Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

[Bjorn Helgaas]

On Wed, May 17, 2023 at 01:52:35PM +0300, 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 RMW capability accessors which does proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
> 
> Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index a7f44f6335fb..9275a672f90c 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -1963,8 +1963,9 @@ 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_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> +					   PCI_EXP_LNKCTL_ASPMC,
> +					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
>  
>  	return 0;
>  }
> @@ -2821,8 +2822,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_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> +				   PCI_EXP_LNKCTL_ASPMC);

These ath drivers all have the form:

  1) read LNKCTL
  2) save LNKCTL value in ->link_ctl
  3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
     to disable ASPM
  4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM

These patches close the hole between 1) and 3) where other LNKCTL
updates could interfere, which is definitely a good thing.

But the hole between 1) and 4) is much bigger and still there.  Any
update by the PCI core in that interval would be lost.

Straw-man proposal:

  - Change pci_disable_link_state() so it ignores aspm_disabled and
    always disables ASPM even if platform firmware hasn't granted
    ownership.  Maybe this should warn and taint the kernel.

  - Change drivers to use pci_disable_link_state() instead of writing
    LNKCTL directly.

Bjorn

Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Wed, 24 May 2023, Bjorn Helgaas wrote:

> On Wed, May 17, 2023 at 01:52:35PM +0300, 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 RMW capability accessors which does proper locking to avoid losing
> > concurrent updates to the register value. On restore, clear the ASPMC
> > field properly.
> > 
> > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > index a7f44f6335fb..9275a672f90c 100644
> > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > @@ -1963,8 +1963,9 @@ 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_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > +					   PCI_EXP_LNKCTL_ASPMC,
> > +					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> >  
> >  	return 0;
> >  }
> > @@ -2821,8 +2822,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_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > +				   PCI_EXP_LNKCTL_ASPMC);
> 
> These ath drivers all have the form:
> 
>   1) read LNKCTL
>   2) save LNKCTL value in ->link_ctl
>   3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
>      to disable ASPM
>   4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> 
> These patches close the hole between 1) and 3) where other LNKCTL
> updates could interfere, which is definitely a good thing.
> 
> But the hole between 1) and 4) is much bigger and still there.  Any
> update by the PCI core in that interval would be lost.

Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the 
updates to _the other fields_ in LNKCTL are not lost.

I know this might result in drivers/pci/pcie/aspm.c disagreeing what
the state of the ASPM is (as shown under sysfs) compared with LNKCTL 
value but the cause can no longer be due racing RMW. Essentially, 4) is 
seen as an override to what core did if it changed ASPMC in between. 
Technically, something is still "lost" like you say but for a different 
reason than this series is trying to fix.

> Straw-man proposal:
> 
>   - Change pci_disable_link_state() so it ignores aspm_disabled and
>     always disables ASPM even if platform firmware hasn't granted
>     ownership.  Maybe this should warn and taint the kernel.
> 
>   - Change drivers to use pci_disable_link_state() instead of writing
>     LNKCTL directly.

I fully agree that's the direction we should be moving, yes. However, I'm 
a bit hesitant to take that leap in one step. These drivers currently not 
only disable ASPM but also re-enable it (assuming we guessed the intent
right).

If I directly implement that proposal, ASPM is not going to be re-enabled 
when PCI core does not allowing it. Could it cause some power related 
regression?

My plan is to make another patch series after these to realize exactly 
what you're proposing. It would allow better to isolate the problems that 
related to the lack of ASPM.

I hope this two step approach is an acceptable way forward? I can of 
course add those patches on top of these if that would be preferrable.


-- 
 i.

Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Thu, 25 May 2023, Ilpo Järvinen wrote:

> On Wed, 24 May 2023, Bjorn Helgaas wrote:
> 
> > On Wed, May 17, 2023 at 01:52:35PM +0300, 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 RMW capability accessors which does proper locking to avoid losing
> > > concurrent updates to the register value. On restore, clear the ASPMC
> > > field properly.
> > > 
> > > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > index a7f44f6335fb..9275a672f90c 100644
> > > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > > @@ -1963,8 +1963,9 @@ 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_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > +					   PCI_EXP_LNKCTL_ASPMC,
> > > +					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> > >  
> > >  	return 0;
> > >  }
> > > @@ -2821,8 +2822,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_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > +				   PCI_EXP_LNKCTL_ASPMC);
> > 
> > These ath drivers all have the form:
> > 
> >   1) read LNKCTL
> >   2) save LNKCTL value in ->link_ctl
> >   3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> >      to disable ASPM
> >   4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> > 
> > These patches close the hole between 1) and 3) where other LNKCTL
> > updates could interfere, which is definitely a good thing.
> > 
> > But the hole between 1) and 4) is much bigger and still there.  Any
> > update by the PCI core in that interval would be lost.
> 
> Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the 
> updates to _the other fields_ in LNKCTL are not lost.
> 
> I know this might result in drivers/pci/pcie/aspm.c disagreeing what
> the state of the ASPM is (as shown under sysfs) compared with LNKCTL 
> value but the cause can no longer be due racing RMW. Essentially, 4) is 
> seen as an override to what core did if it changed ASPMC in between. 
> Technically, something is still "lost" like you say but for a different 
> reason than this series is trying to fix.
> 
> > Straw-man proposal:
> > 
> >   - Change pci_disable_link_state() so it ignores aspm_disabled and
> >     always disables ASPM even if platform firmware hasn't granted
> >     ownership.  Maybe this should warn and taint the kernel.
> > 
> >   - Change drivers to use pci_disable_link_state() instead of writing
> >     LNKCTL directly.

Now that I took a deeper look into what pci_disable_link_state() and 
pci_enable_link_state() do, I realized they're not really disable/enable 
pair like I had assumed from their names. Disable adds to ->aspm_disable 
and flags are never removed from that because enable does not touch 
aspm_disable at all but has it's own flag variable. This asymmetry looks 
intentional.

So if ath drivers would do pci_disable_link_state() to realize 1)-3), 
there is no way to undo it in 4). It looks as if ath drivers would 
actually want to use pci_enable_link_state() with different state 
parameters to realize what they want to do in 1)-4).

Any suggestion which way I should go with these ath drivers here, use 
pci_enable_link_state()?

(There are other drivers where pci_disable_link_state() is very much valid 
thing to do.)

-- 
 i.
	
> I fully agree that's the direction we should be moving, yes. However, I'm 
> a bit hesitant to take that leap in one step. These drivers currently not 
> only disable ASPM but also re-enable it (assuming we guessed the intent
> right).
> 
> If I directly implement that proposal, ASPM is not going to be re-enabled 
> when PCI core does not allowing it. Could it cause some power related 
> regression?
> 
> My plan is to make another patch series after these to realize exactly 
> what you're proposing. It would allow better to isolate the problems that 
> related to the lack of ASPM.
> 
> I hope this two step approach is an acceptable way forward? I can of 
> course add those patches on top of these if that would be preferrable.

Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

[Bjorn Helgaas]

On Thu, May 25, 2023 at 01:11:51PM +0300, Ilpo Järvinen wrote:
> On Wed, 24 May 2023, Bjorn Helgaas wrote:
> > On Wed, May 17, 2023 at 01:52:35PM +0300, 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 RMW capability accessors which does proper locking to avoid losing
> > > concurrent updates to the register value. On restore, clear the ASPMC
> > > field properly.
> > > 
> > > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > index a7f44f6335fb..9275a672f90c 100644
> > > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > > @@ -1963,8 +1963,9 @@ 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_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > +					   PCI_EXP_LNKCTL_ASPMC,
> > > +					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> > >  
> > >  	return 0;
> > >  }
> > > @@ -2821,8 +2822,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_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > +				   PCI_EXP_LNKCTL_ASPMC);
> > 
> > These ath drivers all have the form:
> > 
> >   1) read LNKCTL
> >   2) save LNKCTL value in ->link_ctl
> >   3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> >      to disable ASPM
> >   4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> > 
> > These patches close the hole between 1) and 3) where other LNKCTL
> > updates could interfere, which is definitely a good thing.
> > 
> > But the hole between 1) and 4) is much bigger and still there.  Any
> > update by the PCI core in that interval would be lost.
> 
> Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the 
> updates to _the other fields_ in LNKCTL are not lost.

Ah, yes, you're right, I missed the masking to PCI_EXP_LNKCTL_ASPMC in
the pcie_capability_clear_word().

> > Straw-man proposal:
> > 
> >   - Change pci_disable_link_state() so it ignores aspm_disabled and
> >     always disables ASPM even if platform firmware hasn't granted
> >     ownership.  Maybe this should warn and taint the kernel.
> > 
> >   - Change drivers to use pci_disable_link_state() instead of writing
> >     LNKCTL directly.
> 
> I fully agree that's the direction we should be moving, yes. However, I'm 
> a bit hesitant to take that leap in one step. These drivers currently not 
> only disable ASPM but also re-enable it (assuming we guessed the intent
> right).
> 
> If I directly implement that proposal, ASPM is not going to be re-enabled 
> when PCI core does not allowing it. Could it cause some power related 
> regression?

IIUC the potential problem only happens with:

  - A platform that enables ASPM but doesn't grant PCIe Capability
    ownership to the OS, and

  - A device where we force-disable ASPM, presumably to avoid some
    hardware defect.

I'm not sure this case is worth worrying about.  A platform that
enables ASPM without allowing the OS to disable it is taking a risk
because it can't know about these device defects or even about user
preferences.  A device that has an ASPM-related defect may use more
power than necessary.  I think that's to be expected.

> My plan is to make another patch series after these to realize exactly 
> what you're proposing. It would allow better to isolate the problems that 
> related to the lack of ASPM.
> 
> I hope this two step approach is an acceptable way forward? I can of 
> course add those patches on top of these if that would be preferrable.

I think two steps is OK.  It's a little more work for the driver
maintainers to review them, but this step is pretty trivial already
reviewed (except for the GPUs, which are probably the most important :)).

Bjorn

Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

[Bjorn Helgaas]

On Fri, May 26, 2023 at 02:48:44PM +0300, Ilpo Järvinen wrote:
> On Thu, 25 May 2023, Ilpo Järvinen wrote:
> > On Wed, 24 May 2023, Bjorn Helgaas wrote:
> > > On Wed, May 17, 2023 at 01:52:35PM +0300, 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 RMW capability accessors which does proper locking to avoid losing
> > > > concurrent updates to the register value. On restore, clear the ASPMC
> > > > field properly.
> > > > 
> > > > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > >  drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > > index a7f44f6335fb..9275a672f90c 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > > > @@ -1963,8 +1963,9 @@ 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_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > +					   PCI_EXP_LNKCTL_ASPMC,
> > > > +					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> > > >  
> > > >  	return 0;
> > > >  }
> > > > @@ -2821,8 +2822,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_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > +				   PCI_EXP_LNKCTL_ASPMC);
> > > 
> > > These ath drivers all have the form:
> > > 
> > >   1) read LNKCTL
> > >   2) save LNKCTL value in ->link_ctl
> > >   3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> > >      to disable ASPM
> > >   4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> > > 
> > > These patches close the hole between 1) and 3) where other LNKCTL
> > > updates could interfere, which is definitely a good thing.
> > > 
> > > But the hole between 1) and 4) is much bigger and still there.  Any
> > > update by the PCI core in that interval would be lost.
> > 
> > Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the 
> > updates to _the other fields_ in LNKCTL are not lost.
> > 
> > I know this might result in drivers/pci/pcie/aspm.c disagreeing what
> > the state of the ASPM is (as shown under sysfs) compared with LNKCTL 
> > value but the cause can no longer be due racing RMW. Essentially, 4) is 
> > seen as an override to what core did if it changed ASPMC in between. 
> > Technically, something is still "lost" like you say but for a different 
> > reason than this series is trying to fix.
> > 
> > > Straw-man proposal:
> > > 
> > >   - Change pci_disable_link_state() so it ignores aspm_disabled and
> > >     always disables ASPM even if platform firmware hasn't granted
> > >     ownership.  Maybe this should warn and taint the kernel.
> > > 
> > >   - Change drivers to use pci_disable_link_state() instead of writing
> > >     LNKCTL directly.
> 
> Now that I took a deeper look into what pci_disable_link_state() and 
> pci_enable_link_state() do, I realized they're not really disable/enable 
> pair like I had assumed from their names. Disable adds to ->aspm_disable 
> and flags are never removed from that because enable does not touch 
> aspm_disable at all but has it's own flag variable. This asymmetry looks 
> intentional.

Yes, that's an annoying feature.  There's only one caller of
pci_enable_link_state(), so it may be possible to make this more
symmetric.

> So if ath drivers would do pci_disable_link_state() to realize 1)-3), 
> there is no way to undo it in 4). It looks as if ath drivers would 
> actually want to use pci_enable_link_state() with different state 
> parameters to realize what they want to do in 1)-4).

Yeah, that does sound like a problem.  I don't have any great ideas.

Bjorn

Re: [PATCH v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

[Lukas Wunner]

On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen wrote:
> Don't assume that the device is fully under the control of ASPM and use
> RMW capability accessors which do 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.
[...]
> @@ -224,17 +223,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_capability_set_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
>  	if (parent->clear_retrain_link) {

This and several other RMW operations in drivers/pci/pcie/aspm.c
are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
vars in pcie_retrain_link()") which got applied to pci/enumeration
this week:

https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796

As a result the $SUBJECT_PATCH no longer applies cleanly and needs
to be respun.

Thanks,

Lukas

Re: [PATCH v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Fri, 16 Jun 2023, Lukas Wunner wrote:

> On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen wrote:
> > Don't assume that the device is fully under the control of ASPM and use
> > RMW capability accessors which do 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.
> [...]
> > @@ -224,17 +223,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_capability_set_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
> >  	if (parent->clear_retrain_link) {
> 
> This and several other RMW operations in drivers/pci/pcie/aspm.c
> are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
> vars in pcie_retrain_link()") which got applied to pci/enumeration
> this week:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796
> 
> As a result the $SUBJECT_PATCH no longer applies cleanly and needs
> to be respun.

Okay but I'm a bit lost which commit/head in pci repo I should now base 
this series because there's a conflict between pci/aspm and 
pci/enumeration which is not resolved in the repo because pci/enumeration 
hasn't advanced into pci/next yet. Any suggestion?


-- 
 i.

Re: [PATCH v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

[Bjorn Helgaas]

On Mon, Jun 19, 2023 at 05:45:06PM +0300, Ilpo Järvinen wrote:
> On Fri, 16 Jun 2023, Lukas Wunner wrote:
> > On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen wrote:
> > > Don't assume that the device is fully under the control of ASPM and use
> > > RMW capability accessors which do 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.
> > [...]
> > > @@ -224,17 +223,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_capability_set_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
> > >  	if (parent->clear_retrain_link) {
> > 
> > This and several other RMW operations in drivers/pci/pcie/aspm.c
> > are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
> > vars in pcie_retrain_link()") which got applied to pci/enumeration
> > this week:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796
> > 
> > As a result the $SUBJECT_PATCH no longer applies cleanly and needs
> > to be respun.
> 
> Okay but I'm a bit lost which commit/head in pci repo I should now base 
> this series because there's a conflict between pci/aspm and 
> pci/enumeration which is not resolved in the repo because pci/enumeration 
> hasn't advanced into pci/next yet. Any suggestion?

Generally speaking I prefer patches based on the PCI "main" branch
(usually -rc1) because I base topic branches on that.  If there are
conflicts with other pending material, it's great if you can mention
them, but I can resolve them when applying, so no need to repost just
for that.

Bjorn

Re: [PATCH v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Mon, 19 Jun 2023, Bjorn Helgaas wrote:

> On Mon, Jun 19, 2023 at 05:45:06PM +0300, Ilpo Järvinen wrote:
> > On Fri, 16 Jun 2023, Lukas Wunner wrote:
> > > On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen wrote:
> > > > Don't assume that the device is fully under the control of ASPM and use
> > > > RMW capability accessors which do 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.
> > > [...]
> > > > @@ -224,17 +223,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_capability_set_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
> > > >  	if (parent->clear_retrain_link) {
> > > 
> > > This and several other RMW operations in drivers/pci/pcie/aspm.c
> > > are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
> > > vars in pcie_retrain_link()") which got applied to pci/enumeration
> > > this week:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796
> > > 
> > > As a result the $SUBJECT_PATCH no longer applies cleanly and needs
> > > to be respun.
> > 
> > Okay but I'm a bit lost which commit/head in pci repo I should now base 
> > this series because there's a conflict between pci/aspm and 
> > pci/enumeration which is not resolved in the repo because pci/enumeration 
> > hasn't advanced into pci/next yet. Any suggestion?
> 
> Generally speaking I prefer patches based on the PCI "main" branch
> (usually -rc1) because I base topic branches on that.  If there are
> conflicts with other pending material, it's great if you can mention
> them, but I can resolve them when applying, so no need to repost just
> for that.

Just to confirm I'm understanding this correctly... Am I supposed to send
a patch whose changelog description and diff do not agree?? (The diff 
will still modify pcie/aspm.c if it's main-based and the description 
refers to something more generic as per Lukas' request since the code got 
moved into pci.c in the enumeration branch).


-- 
 i.