diff mbox series

[1/1] PCI/bwctrl: Replace lbms_count with PCIE_LINK_LBMS_SEEN flag

Message ID 20250417124633.11470-1-ilpo.jarvinen@linux.intel.com (mailing list archive)
State New
Headers show
Series [1/1] PCI/bwctrl: Replace lbms_count with PCIE_LINK_LBMS_SEEN flag | expand

Commit Message

Ilpo Järvinen April 17, 2025, 12:46 p.m. UTC
PCIe BW controller counts LBMS assertions for the purposes of the
Target Speed quirk. It was also a plan to expose the LBMS count through
sysfs to allow better diagnosing link related issues. Lukas Wunner
suggested, however, that adding a trace event would be better for
diagnostics purposes. Leaving only Target Speed quirk as an user of the
lbms_count.

The logic in the Target Speed quirk does not require keeping count of
LBMS assertions but a simple flag is enough which can be placed into
pci_dev's priv_flags. The reduced complexity allows removing
pcie_bwctrl_lbms_rwsem.

As bwctrl is not yet probed when the Target Speed quirk runs during
boot, the LBMS in Link Status register has to still be checked by
the quirk.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

This will conflict with the new flags Lukas added due to the hp fixes
but it should be simple to deal with that conflict while merging the
topic branches.

 drivers/pci/hotplug/pciehp_ctrl.c |  2 +-
 drivers/pci/pci.c                 |  2 +-
 drivers/pci/pci.h                 | 10 ++---
 drivers/pci/pcie/bwctrl.c         | 63 +++++++++----------------------
 drivers/pci/quirks.c              | 10 ++---
 5 files changed, 25 insertions(+), 62 deletions(-)


base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8

Comments

Lukas Wunner April 17, 2025, 2:08 p.m. UTC | #1
On Thu, Apr 17, 2025 at 03:46:32PM +0300, Ilpo Järvinen wrote:
> PCIe BW controller counts LBMS assertions for the purposes of the
> Target Speed quirk. It was also a plan to expose the LBMS count through
> sysfs to allow better diagnosing link related issues. Lukas Wunner
> suggested, however, that adding a trace event would be better for
> diagnostics purposes. Leaving only Target Speed quirk as an user of the
> lbms_count.
> 
> The logic in the Target Speed quirk does not require keeping count of
> LBMS assertions but a simple flag is enough which can be placed into
> pci_dev's priv_flags. The reduced complexity allows removing
> pcie_bwctrl_lbms_rwsem.
[...]
> This will conflict with the new flags Lukas added due to the hp fixes
> but it should be simple to deal with that conflict while merging the
> topic branches.

Hm, perhaps it would be easier to merge this if it would use bit 6
instead of bit 4.  Then it would just be a trivial merge conflict
between two topic branches (pci/hotplug and pci/bwctrl, I presume).
The way it is now, it will require manually tweaking the bit after
applying the patch.

> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -557,6 +557,7 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  #define PCI_DPC_RECOVERED 1
>  #define PCI_DPC_RECOVERING 2
>  #define PCI_DEV_REMOVED 3
> +#define PCIE_LINK_LBMS_SEEN	4

The two newly added bits on the pci/hotplug branch use the prefix
"PCI_LINK_".  It would be slightly neater if this used the same prefix.

>  drivers/pci/hotplug/pciehp_ctrl.c |  2 +-
>  drivers/pci/pci.c                 |  2 +-
>  drivers/pci/pci.h                 | 10 ++---
>  drivers/pci/pcie/bwctrl.c         | 63 +++++++++----------------------
>  drivers/pci/quirks.c              | 10 ++---
>  5 files changed, 25 insertions(+), 62 deletions(-)

Obviously a nice, welcome simplification, shaving off 37 LoC.
Thanks for doing this!

Lukas
diff mbox series

Patch

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index d603a7aa7483..bcc938d4420f 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -131,7 +131,7 @@  static void remove_board(struct controller *ctrl, bool safe_removal)
 			      INDICATOR_NOOP);
 
 	/* Don't carry LBMS indications across */
-	pcie_reset_lbms_count(ctrl->pcie->port);
+	pcie_reset_lbms(ctrl->pcie->port);
 }
 
 static int pciehp_enable_slot(struct controller *ctrl);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4d7c9f64ea24..3d94cf33c1b6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4757,7 +4757,7 @@  int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
 	 * to track link speed or width changes made by hardware itself
 	 * in attempt to correct unreliable link operation.
 	 */
-	pcie_reset_lbms_count(pdev);
+	pcie_reset_lbms(pdev);
 	return rc;
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b81e99cd4b62..b7c284dbcb97 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -557,6 +557,7 @@  static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 #define PCI_DPC_RECOVERED 1
 #define PCI_DPC_RECOVERING 2
 #define PCI_DEV_REMOVED 3
+#define PCIE_LINK_LBMS_SEEN	4
 
 static inline void pci_dev_assign_added(struct pci_dev *dev)
 {
@@ -824,14 +825,9 @@  static inline void pcie_ecrc_get_policy(char *str) { }
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
-void pcie_reset_lbms_count(struct pci_dev *port);
-int pcie_lbms_count(struct pci_dev *port, unsigned long *val);
+void pcie_reset_lbms(struct pci_dev *port);
 #else
-static inline void pcie_reset_lbms_count(struct pci_dev *port) {}
-static inline int pcie_lbms_count(struct pci_dev *port, unsigned long *val)
-{
-	return -EOPNOTSUPP;
-}
+static inline void pcie_reset_lbms(struct pci_dev *port) {}
 #endif
 
 struct pci_dev_reset_methods {
diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
index d8d2aa85a229..701e9e8ddfcb 100644
--- a/drivers/pci/pcie/bwctrl.c
+++ b/drivers/pci/pcie/bwctrl.c
@@ -38,12 +38,10 @@ 
 /**
  * struct pcie_bwctrl_data - PCIe bandwidth controller
  * @set_speed_mutex:	Serializes link speed changes
- * @lbms_count:		Count for LBMS (since last reset)
  * @cdev:		Thermal cooling device associated with the port
  */
 struct pcie_bwctrl_data {
 	struct mutex set_speed_mutex;
-	atomic_t lbms_count;
 	struct thermal_cooling_device *cdev;
 };
 
@@ -202,15 +200,14 @@  int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
 
 static void pcie_bwnotif_enable(struct pcie_device *srv)
 {
-	struct pcie_bwctrl_data *data = srv->port->link_bwctrl;
 	struct pci_dev *port = srv->port;
 	u16 link_status;
 	int ret;
 
-	/* Count LBMS seen so far as one */
+	/* Note down if LBMS has been seen so far */
 	ret = pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status);
 	if (ret == PCIBIOS_SUCCESSFUL && link_status & PCI_EXP_LNKSTA_LBMS)
-		atomic_inc(&data->lbms_count);
+		set_bit(PCIE_LINK_LBMS_SEEN, &port->priv_flags);
 
 	pcie_capability_set_word(port, PCI_EXP_LNKCTL,
 				 PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE);
@@ -233,7 +230,6 @@  static void pcie_bwnotif_disable(struct pci_dev *port)
 static irqreturn_t pcie_bwnotif_irq(int irq, void *context)
 {
 	struct pcie_device *srv = context;
-	struct pcie_bwctrl_data *data = srv->port->link_bwctrl;
 	struct pci_dev *port = srv->port;
 	u16 link_status, events;
 	int ret;
@@ -247,7 +243,7 @@  static irqreturn_t pcie_bwnotif_irq(int irq, void *context)
 		return IRQ_NONE;
 
 	if (events & PCI_EXP_LNKSTA_LBMS)
-		atomic_inc(&data->lbms_count);
+		set_bit(PCIE_LINK_LBMS_SEEN, &port->priv_flags);
 
 	pcie_capability_write_word(port, PCI_EXP_LNKSTA, events);
 
@@ -262,31 +258,10 @@  static irqreturn_t pcie_bwnotif_irq(int irq, void *context)
 	return IRQ_HANDLED;
 }
 
-void pcie_reset_lbms_count(struct pci_dev *port)
+void pcie_reset_lbms(struct pci_dev *port)
 {
-	struct pcie_bwctrl_data *data;
-
-	guard(rwsem_read)(&pcie_bwctrl_lbms_rwsem);
-	data = port->link_bwctrl;
-	if (data)
-		atomic_set(&data->lbms_count, 0);
-	else
-		pcie_capability_write_word(port, PCI_EXP_LNKSTA,
-					   PCI_EXP_LNKSTA_LBMS);
-}
-
-int pcie_lbms_count(struct pci_dev *port, unsigned long *val)
-{
-	struct pcie_bwctrl_data *data;
-
-	guard(rwsem_read)(&pcie_bwctrl_lbms_rwsem);
-	data = port->link_bwctrl;
-	if (!data)
-		return -ENOTTY;
-
-	*val = atomic_read(&data->lbms_count);
-
-	return 0;
+	clear_bit(PCIE_LINK_LBMS_SEEN, &port->priv_flags);
+	pcie_capability_write_word(port, PCI_EXP_LNKSTA, PCI_EXP_LNKSTA_LBMS);
 }
 
 static int pcie_bwnotif_probe(struct pcie_device *srv)
@@ -308,18 +283,16 @@  static int pcie_bwnotif_probe(struct pcie_device *srv)
 		return ret;
 
 	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem) {
-		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem) {
-			port->link_bwctrl = data;
+		port->link_bwctrl = data;
 
-			ret = request_irq(srv->irq, pcie_bwnotif_irq,
-					  IRQF_SHARED, "PCIe bwctrl", srv);
-			if (ret) {
-				port->link_bwctrl = NULL;
-				return ret;
-			}
-
-			pcie_bwnotif_enable(srv);
+		ret = request_irq(srv->irq, pcie_bwnotif_irq,
+				  IRQF_SHARED, "PCIe bwctrl", srv);
+		if (ret) {
+			port->link_bwctrl = NULL;
+			return ret;
 		}
+
+		pcie_bwnotif_enable(srv);
 	}
 
 	pci_dbg(port, "enabled with IRQ %d\n", srv->irq);
@@ -339,13 +312,11 @@  static void pcie_bwnotif_remove(struct pcie_device *srv)
 	pcie_cooling_device_unregister(data->cdev);
 
 	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem) {
-		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem) {
-			pcie_bwnotif_disable(srv->port);
+		pcie_bwnotif_disable(srv->port);
 
-			free_irq(srv->irq, srv);
+		free_irq(srv->irq, srv);
 
-			srv->port->link_bwctrl = NULL;
-		}
+		srv->port->link_bwctrl = NULL;
 	}
 }
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8d610c17e0f2..f04a7e56872a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -38,14 +38,10 @@ 
 
 static bool pcie_lbms_seen(struct pci_dev *dev, u16 lnksta)
 {
-	unsigned long count;
-	int ret;
-
-	ret = pcie_lbms_count(dev, &count);
-	if (ret < 0)
-		return lnksta & PCI_EXP_LNKSTA_LBMS;
+	if (test_bit(PCIE_LINK_LBMS_SEEN, &dev->priv_flags))
+		return true;
 
-	return count > 0;
+	return lnksta & PCI_EXP_LNKSTA_LBMS;
 }
 
 /*