Re: [PATCH v2 1/1] PCI/bwctrl: Replace lbms_count with PCI_LINK_LBMS_SEEN flag

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Apr 22, 2025 at 02:55:47PM +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.
> 
> 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.
> 
> The priv_flags numbering is not continuous because hotplug code added
> a few flags to fill numbers 4-5 (hotplug and bwctrl changes are routed
> through in different branches).
> 
> 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.
> 
> v2:
> - Change flag value to 6 to make merging with the newly added HP flags easier
> - Renamed flag to PCI_LINK_LBMS_SEEN to match the naming of the new HP flags
> 
>  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(-)

Applied to pci/bwctrl for v6.16, on the assumption that everybody's
happy with this.  Thanks!

> 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..887811fbe722 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 PCI_LINK_LBMS_SEEN	6
>  
>  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..ab0387ff2de2 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(PCI_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(PCI_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(PCI_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..64ac1ee944d3 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(PCI_LINK_LBMS_SEEN, &dev->priv_flags))
> +		return true;
>  
> -	return count > 0;
> +	return lnksta & PCI_EXP_LNKSTA_LBMS;
>  }
>  
>  /*
> 
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> -- 
> 2.39.5
>