[PATCH v5 01/11] PCI: Add locking to RMW PCI Express Capability Register accessors

[PATCH v5 01/11] 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.

So far only the Link Control Register (ASPM, hotplug, link retraining,
various drivers) and the Root Control Register (AER & PME) seem to
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>
Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
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 8bac3ce02609..f1587fb0ba71 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2324,6 +2324,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 c69a2cc1f412..7ee498cd1f37 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 v5 02/11] PCI: Make link retraining use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

Don't assume that the device is fully under the control of PCI core.
Use RMW capability accessors in link retraining which do proper locking
to avoid losing concurrent updates to the register values.

Fixes: 4ec73791a64b ("PCI: Work around Pericom PCIe-to-PCI bridge Retrain Link erratum")
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>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Cc: stable@vger.kernel.org
---

pci/enumeration branch moves the link retraining code into PCI core and
also conflicts with a link retraining fix in pci/aspm. The changelog
(and patch splitting) takes the move into account by not referring to
ASPM while the change itself is not based on pci/enumeration (as per
Bjorn's preference).
---
 drivers/pci/pci.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0..f7315b13bb82 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4927,7 +4927,6 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
 int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
 {
 	int rc;
-	u16 lnkctl;
 
 	/*
 	 * Ensure the updated LNKCTL parameters are used during link
@@ -4939,17 +4938,14 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
 	if (rc)
 		return rc;
 
-	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &lnkctl);
-	lnkctl |= PCI_EXP_LNKCTL_RL;
-	pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, lnkctl);
+	pcie_capability_set_word(pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
 	if (pdev->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.
 		 */
-		lnkctl &= ~PCI_EXP_LNKCTL_RL;
-		pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, lnkctl);
+		pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
 	}
 
 	return pcie_wait_for_link_status(pdev, use_lt, !use_lt);
-- 
2.30.2

[PATCH v5 03/11] 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>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
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 8711325605f0..fd713abdfb9f 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 ? 0 : PCI_EXP_LNKCTL_LD);
 
-	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 v5 04/11] 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: 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>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/pcie/aspm.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 3dafba0b5f41..1bf630059264 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -199,7 +199,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 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, ccc, parent_old_ccc, child_old_ccc[8];
 	struct pci_dev *child, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
 	/*
@@ -221,6 +221,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;
 
@@ -237,34 +238,29 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 		pci_info(parent, "ASPM: current common clock configuration is inconsistent, reconfiguring\n");
 	}
 
+	ccc = same_clock ? PCI_EXP_LNKCTL_CCC : 0;
 	/* 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, ccc);
 	}
 
 	/* 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, ccc);
 
 	if (pcie_retrain_link(link->pdev, true)) {
 
 		/* 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);
 	}
 }
 
-- 
2.30.2

[PATCH v5 05/11] 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 5641cf05d856..e63abdf52b6c 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 f64b87b11b1b..4b81f29e5fd5 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

Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL

[Bjorn Helgaas]

On Mon, Jul 17, 2023 at 03:04:57PM +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.
> 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

Do we have any reports of problems that are fixed by this patch (or by
others in the series)?  If not, I'm not sure it really fits the usual
stable kernel criteria:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4

> ---
>  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 5641cf05d856..e63abdf52b6c 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);

Wow, there's a lot of pointless-looking work going on here:

  set root PCI_EXP_LNKCTL_HAWD
  set GPU  PCI_EXP_LNKCTL_HAWD

  for (i = 0; i < 10; i++) {
    read root PCI_EXP_LNKCTL
    read GPU  PCI_EXP_LNKCTL

    clear root PCI_EXP_LNKCTL_HAWD
    if (root PCI_EXP_LNKCTL_HAWD was set)
      set root PCI_EXP_LNKCTL_HAWD

    clear GPU  PCI_EXP_LNKCTL_HAWD
    if (GPU  PCI_EXP_LNKCTL_HAWD was set)
      set GPU  PCI_EXP_LNKCTL_HAWD
  }

If it really *is* pointless, it would be nice to clean it up, but that
wouldn't be material for this patch, so what you have looks good.

>  				/* linkctl2 */
>  				pcie_capability_read_word(root, PCI_EXP_LNKCTL2,

The PCI_EXP_LNKCTL2 stuff also includes RMW updates.  I don't see any
uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I
guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic?

Bjorn

Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Thu, 20 Jul 2023, Bjorn Helgaas wrote:

> On Mon, Jul 17, 2023 at 03:04:57PM +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.
> > 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
> 
> Do we have any reports of problems that are fixed by this patch (or by
> others in the series)?  If not, I'm not sure it really fits the usual
> stable kernel criteria:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4

I was on the edge with this. The answer to your direct question is no, 
there are no such reports so it would be okay to leave stable out I think. 
This applies to all patches in this series.

Basically, this series came to be after Lukas noted the potential 
concurrency issues with how LNKCTL is unprotected when reviewing 
(internally) my bandwidth controller series. Then I went to look around 
all LNKCTL usage and realized existing things might alreary have similar 
issues.

Do you want me to send another version w/o cc stable or you'll take care 
of that?

> > ---
> >  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 5641cf05d856..e63abdf52b6c 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);
> 
> Wow, there's a lot of pointless-looking work going on here:
> 
>   set root PCI_EXP_LNKCTL_HAWD
>   set GPU  PCI_EXP_LNKCTL_HAWD
> 
>   for (i = 0; i < 10; i++) {
>     read root PCI_EXP_LNKCTL
>     read GPU  PCI_EXP_LNKCTL
> 
>     clear root PCI_EXP_LNKCTL_HAWD
>     if (root PCI_EXP_LNKCTL_HAWD was set)
>       set root PCI_EXP_LNKCTL_HAWD
> 
>     clear GPU  PCI_EXP_LNKCTL_HAWD
>     if (GPU  PCI_EXP_LNKCTL_HAWD was set)
>       set GPU  PCI_EXP_LNKCTL_HAWD
>   }
> 
> If it really *is* pointless, it would be nice to clean it up, but that
> wouldn't be material for this patch, so what you have looks good.

I really don't know if it's needed or not. There's stuff which looks hw 
specific going on besides those things you point out and I've not really 
understood what all that does.

One annoying thing is that this code has been copy-pasted to appear in 
almost identical form in 4 files.

I agree it certainly looks there might be room for cleaning things up here 
but such cleanups look a bit too scary to me w/o hw to test them.

> >  				/* linkctl2 */
> >  				pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> 
> The PCI_EXP_LNKCTL2 stuff also includes RMW updates.  I don't see any
> uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I
> guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic?

Currently no, which is why I left it out from this patchset.

It is going to change soon though as I intend to submit bandwidth 
controller series after this series which will add RMW ops for LNKCTL2.
The LNKCTL2 RMW parts are now in that series rather than in this one.

After adding the bandwidth controller, this driver might be able to use
it instead of tweaking LNKCTL2 directly to alter PCIe link speed (but I 
don't expect myself to be able to test these drivers and it feels too 
risky to make such a change without testing it, unfortunately).


-- 
 i.

Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL

[Alex Deucher]

On Fri, Jul 21, 2023 at 4:18 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Thu, 20 Jul 2023, Bjorn Helgaas wrote:
>
> > On Mon, Jul 17, 2023 at 03:04:57PM +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.
> > > 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
> >
> > Do we have any reports of problems that are fixed by this patch (or by
> > others in the series)?  If not, I'm not sure it really fits the usual
> > stable kernel criteria:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4
>
> I was on the edge with this. The answer to your direct question is no,
> there are no such reports so it would be okay to leave stable out I think.
> This applies to all patches in this series.
>
> Basically, this series came to be after Lukas noted the potential
> concurrency issues with how LNKCTL is unprotected when reviewing
> (internally) my bandwidth controller series. Then I went to look around
> all LNKCTL usage and realized existing things might alreary have similar
> issues.
>
> Do you want me to send another version w/o cc stable or you'll take care
> of that?
>
> > > ---
> > >  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 5641cf05d856..e63abdf52b6c 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);
> >
> > Wow, there's a lot of pointless-looking work going on here:
> >
> >   set root PCI_EXP_LNKCTL_HAWD
> >   set GPU  PCI_EXP_LNKCTL_HAWD
> >
> >   for (i = 0; i < 10; i++) {
> >     read root PCI_EXP_LNKCTL
> >     read GPU  PCI_EXP_LNKCTL
> >
> >     clear root PCI_EXP_LNKCTL_HAWD
> >     if (root PCI_EXP_LNKCTL_HAWD was set)
> >       set root PCI_EXP_LNKCTL_HAWD
> >
> >     clear GPU  PCI_EXP_LNKCTL_HAWD
> >     if (GPU  PCI_EXP_LNKCTL_HAWD was set)
> >       set GPU  PCI_EXP_LNKCTL_HAWD
> >   }
> >
> > If it really *is* pointless, it would be nice to clean it up, but that
> > wouldn't be material for this patch, so what you have looks good.
>
> I really don't know if it's needed or not. There's stuff which looks hw
> specific going on besides those things you point out and I've not really
> understood what all that does.
>
> One annoying thing is that this code has been copy-pasted to appear in
> almost identical form in 4 files.
>
> I agree it certainly looks there might be room for cleaning things up here
> but such cleanups look a bit too scary to me w/o hw to test them.
>
> > >                             /* linkctl2 */
> > >                             pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> >
> > The PCI_EXP_LNKCTL2 stuff also includes RMW updates.  I don't see any
> > uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I
> > guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic?
>
> Currently no, which is why I left it out from this patchset.
>
> It is going to change soon though as I intend to submit bandwidth
> controller series after this series which will add RMW ops for LNKCTL2.
> The LNKCTL2 RMW parts are now in that series rather than in this one.
>
> After adding the bandwidth controller, this driver might be able to use
> it instead of tweaking LNKCTL2 directly to alter PCIe link speed (but I
> don't expect myself to be able to test these drivers and it feels too
> risky to make such a change without testing it, unfortunately).

Thanks for the background.  It was not clear what the point of this
patch set was.  This code and the similar code in radeon is just to
change the link speed of the GPU.  Some older platforms used default
to slower link on boot so we added this code to renegotiate the link
to a faster speed when the driver loaded.  If you are adding core
infrastructure to do that, we can switch to that.  This was just the
programming sequence I got from the hardware team back when this code
was written.  Most platforms I've seen these days come up at the max
supported speed of the platform and endpoint so I don't think the code
actually gets used much anymore.

Taking a step back, what is the end goal of the bandwidth controller
changes?  The reason I ask is that today, we look at the currently
negotiated speed of the link and use that for the baseline in the
driver.  The driver then enables PCIe dynamic power management where
the system management unit on the GPU dynamically adjusts the link
speed, width, and clock on demand based on the PCIe bandwidth
requirements of the currently executing GPU jobs to save power.  This
might conflict with software if the goal is for some software
component to do something similar.

Alex

Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Fri, 21 Jul 2023, Alex Deucher wrote:

> On Fri, Jul 21, 2023 at 4:18 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Thu, 20 Jul 2023, Bjorn Helgaas wrote:
> >
> > > On Mon, Jul 17, 2023 at 03:04:57PM +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.
> > > > 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
> > >
> > > Do we have any reports of problems that are fixed by this patch (or by
> > > others in the series)?  If not, I'm not sure it really fits the usual
> > > stable kernel criteria:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4
> >
> > I was on the edge with this. The answer to your direct question is no,
> > there are no such reports so it would be okay to leave stable out I think.
> > This applies to all patches in this series.
> >
> > Basically, this series came to be after Lukas noted the potential
> > concurrency issues with how LNKCTL is unprotected when reviewing
> > (internally) my bandwidth controller series. Then I went to look around
> > all LNKCTL usage and realized existing things might alreary have similar
> > issues.
> >
> > Do you want me to send another version w/o cc stable or you'll take care
> > of that?
> >
> > > > ---
> > > >  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 5641cf05d856..e63abdf52b6c 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);
> > >
> > > Wow, there's a lot of pointless-looking work going on here:
> > >
> > >   set root PCI_EXP_LNKCTL_HAWD
> > >   set GPU  PCI_EXP_LNKCTL_HAWD
> > >
> > >   for (i = 0; i < 10; i++) {
> > >     read root PCI_EXP_LNKCTL
> > >     read GPU  PCI_EXP_LNKCTL
> > >
> > >     clear root PCI_EXP_LNKCTL_HAWD
> > >     if (root PCI_EXP_LNKCTL_HAWD was set)
> > >       set root PCI_EXP_LNKCTL_HAWD
> > >
> > >     clear GPU  PCI_EXP_LNKCTL_HAWD
> > >     if (GPU  PCI_EXP_LNKCTL_HAWD was set)
> > >       set GPU  PCI_EXP_LNKCTL_HAWD
> > >   }
> > >
> > > If it really *is* pointless, it would be nice to clean it up, but that
> > > wouldn't be material for this patch, so what you have looks good.
> >
> > I really don't know if it's needed or not. There's stuff which looks hw
> > specific going on besides those things you point out and I've not really
> > understood what all that does.
> >
> > One annoying thing is that this code has been copy-pasted to appear in
> > almost identical form in 4 files.
> >
> > I agree it certainly looks there might be room for cleaning things up here
> > but such cleanups look a bit too scary to me w/o hw to test them.
> >
> > > >                             /* linkctl2 */
> > > >                             pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> > >
> > > The PCI_EXP_LNKCTL2 stuff also includes RMW updates.  I don't see any
> > > uses of PCI_EXP_LNKCTL2 outside this driver that look relevant, so I
> > > guess we don't care about making the PCI_EXP_LNKCTL2 updates atomic?
> >
> > Currently no, which is why I left it out from this patchset.
> >
> > It is going to change soon though as I intend to submit bandwidth
> > controller series after this series which will add RMW ops for LNKCTL2.
> > The LNKCTL2 RMW parts are now in that series rather than in this one.
> >
> > After adding the bandwidth controller, this driver might be able to use
> > it instead of tweaking LNKCTL2 directly to alter PCIe link speed (but I
> > don't expect myself to be able to test these drivers and it feels too
> > risky to make such a change without testing it, unfortunately).
> 
> Thanks for the background.  It was not clear what the point of this
> patch set was.

Thanks for chimming in!

There are two reasons, one is to fix the existing RMW races and the other 
is the addition of BW controller. The RMW race issue was discovered while 
Lukas was reviewing BW controller's code and it was disjoint/generic 
enough from the BW controller to go into its own patchset.

> This code and the similar code in radeon is just to
> change the link speed of the GPU.  Some older platforms used default
> to slower link on boot so we added this code to renegotiate the link
> to a faster speed when the driver loaded.  If you are adding core
> infrastructure to do that, we can switch to that.  This was just the
> programming sequence I got from the hardware team back when this code
> was written. Most platforms I've seen these days come up at the max
> supported speed of the platform and endpoint so I don't think the code
> actually gets used much anymore.

If I understood the code correctly, it's the device side with these GPUs 
which triggers the actual Link Speed change based on those HW specific 
writes?

With the BW controller, the speed change would be initiated by setting the 
Root Port to do Link retraining.

> Taking a step back, what is the end goal of the bandwidth controller
> changes?  The reason I ask is that today, we look at the currently
> negotiated speed of the link and use that for the baseline in the
> driver.  The driver then enables PCIe dynamic power management where
> the system management unit on the GPU dynamically adjusts the link
> speed, width, and clock on demand based on the PCIe bandwidth
> requirements of the currently executing GPU jobs to save power.  This
> might conflict with software if the goal is for some software
> component to do something similar.

BW controller is mainly done for thermal reasons (a thermal side cooling 
device is provided per root port for thermald/userspace to tap into) but 
it is not to say there couldn't be other usecases.

If there's another actor besides BW controller altering Link Speeds, I 
think what would happen is that BW controller setting Root Port's Target 
Speed would just upper-bound the Link Speed requests from GPU (which makes 
sense at least from thermal point-of-view).


-- 
 i.

[PATCH v5 06/11] 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

RE: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Monday, July 17, 2023 8:05 AM
> To: linux-pci@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Lorenzo
> Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>;
> Krzysztof Wilczyński <kw@linux.com>; Emmanuel Grumbach
> <emmanuel.grumbach@intel.com>; Rafael J . Wysocki <rafael@kernel.org>;
> Heiner Kallweit <hkallweit1@gmail.com>; Lukas Wunner <lukas@wunner.de>;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David
> Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Cc: Dean Luick <dean.luick@cornelisnetworks.com>; Jonas Dreßler
> <verdre@v0yd.nl>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>;
> stable@vger.kernel.org
> Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing
> LNKCTL
>
> 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

For this and the amdgpu patch:
Acked-by: Alex Deucher <alexander.deucher@amd.com>
I'm not sure if this is stable material however.  Is there some issue today?


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

RE: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL

[Ilpo Järvinen]

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

On Fri, 18 Aug 2023, Deucher, Alexander wrote:

> [Public]
> 
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Monday, July 17, 2023 8:05 AM
> > To: linux-pci@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Lorenzo
> > Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>;
> > Krzysztof Wilczyński <kw@linux.com>; Emmanuel Grumbach
> > <emmanuel.grumbach@intel.com>; Rafael J . Wysocki <rafael@kernel.org>;
> > Heiner Kallweit <hkallweit1@gmail.com>; Lukas Wunner <lukas@wunner.de>;
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Koenig, Christian
> > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David
> > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org
> > Cc: Dean Luick <dean.luick@cornelisnetworks.com>; Jonas Dreßler
> > <verdre@v0yd.nl>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>;
> > stable@vger.kernel.org
> > Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing
> > LNKCTL
> >
> > 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
> 
> For this and the amdgpu patch:
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> I'm not sure if this is stable material however.  Is there some issue today?

These were added without Cc stable into pci.git/pcie-rmw.

-- 
 i.

Re: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL

[Bjorn Helgaas]

On Fri, Aug 18, 2023 at 04:12:57PM +0000, Deucher, Alexander wrote:
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Monday, July 17, 2023 8:05 AM
> > To: linux-pci@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Lorenzo
> > Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>;
> > Krzysztof Wilczyński <kw@linux.com>; Emmanuel Grumbach
> > <emmanuel.grumbach@intel.com>; Rafael J . Wysocki <rafael@kernel.org>;
> > Heiner Kallweit <hkallweit1@gmail.com>; Lukas Wunner <lukas@wunner.de>;
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Koenig, Christian
> > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David
> > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; amd-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org
> > Cc: Dean Luick <dean.luick@cornelisnetworks.com>; Jonas Dreßler
> > <verdre@v0yd.nl>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>;
> > stable@vger.kernel.org
> > Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing
> > LNKCTL
> >
> > 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
> 
> For this and the amdgpu patch:
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> I'm not sure if this is stable material however.  Is there some issue today?

Added your ack, thanks!  I dropped the stable tag on the whole series.

Bjorn

[PATCH v5 07/11] 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>
Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
Reviewed-by: Simon Horman <simon.horman@corigine.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 4804990b7f22..99dcbd006357 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c
@@ -384,16 +384,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 v5 08/11] 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>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Acked-by: Kalle Valo <kvalo@kernel.org>
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 79e2cbe82638..ec40adc1cb23 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 v5 09/11] 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>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Acked-by: Kalle Valo <kvalo@kernel.org>
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 5990a55801f0..e4f08a066ca1 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 v5 10/11] 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>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Acked-by: Kalle Valo <kvalo@kernel.org>
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