Re: [PATCH v8 6/8] PCI/bwctrl: Add API to set PCIe Link Speed

[Jonathan Cameron]

On Wed,  9 Oct 2024 12:52:21 +0300
Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:

> Currently, PCIe Link Speeds are adjusted by custom code rather than in
> a common function provided in PCI core. PCIe bandwidth controller
> (bwctrl) introduces an in-kernel API to set PCIe Link Speed.
> 
> Convert Target Speed quirk to use the new API. The Target Speed quirk
> runs very early when bwctrl is not yet probed for a Port and can also
> run later when bwctrl is already setup for the Port, which requires the
> per port mutex (set_speed_mutex) to be only taken if the bwctrl setup
> is already complete.
> 
> The new API is also intended to be used in an upcoming commit that adds
> a thermal cooling device to throttle PCIe bandwidth when thermal
> thresholds are reached.
> 
> The PCIe bandwidth control procedure is as follows. The highest speed
> supported by the Port and the PCIe device which is not higher than the
> requested speed is selected and written into the Target Link Speed in
> the Link Control 2 Register. Then bandwidth controller retrains the
> PCIe Link.
> 
> Bandwidth Notifications enable the cur_bus_speed in the struct pci_bus
> to keep track PCIe Link Speed changes. While Bandwidth Notifications
> should also be generated when bandwidth controller alters the PCIe Link
> Speed, a few platforms do not deliver LMBS interrupt after Link
> Training as expected. Thus, after changing the Link Speed, bandwidth
> controller makes additional read for the Link Status Register to ensure
> cur_bus_speed is consistent with the new PCIe Link Speed.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>

Trivial stuff inline.  The mutex_destroy discussion is a just a consistency
thing given that call is rarely bothered with but here it might help with
debug.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

Jonathan

> ---
>  drivers/pci/pci.h         |  20 +++++
>  drivers/pci/pcie/bwctrl.c | 161 +++++++++++++++++++++++++++++++++++++-
>  drivers/pci/quirks.c      |  17 +---
>  include/linux/pci.h       |  10 +++
>  4 files changed, 193 insertions(+), 15 deletions(-)
> 



> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
> index 1b11b5da79d4..1d3680ea8e06 100644
> --- a/drivers/pci/pcie/bwctrl.c
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -7,6 +7,11 @@






>  static void pcie_bwnotif_enable(struct pcie_device *srv)
>  {
>  	struct pcie_bwctrl_data *data = get_service_data(srv);
> @@ -135,6 +288,7 @@ static int pcie_bwnotif_probe(struct pcie_device *srv)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	mutex_init(&data->set_speed_mutex);
>  	set_service_data(srv, data);
>  
>  	ret = request_threaded_irq(srv->irq, NULL, pcie_bwnotif_irq_thread,
> @@ -142,8 +296,10 @@ static int pcie_bwnotif_probe(struct pcie_device *srv)
>  	if (ret)
>  		return ret;
>  
> -	port->link_bwctrl = no_free_ptr(data);
> -	pcie_bwnotif_enable(srv);
> +	scoped_guard(rwsem_write, &pcie_bwctrl_remove_rwsem) {

Calling it remove_rwsem and using it to protect against not yet
present seems odd. Maybe rename, pcie_bwctrl_bound_rswem or something like that?

> +		port->link_bwctrl = no_free_ptr(data);
> +		pcie_bwnotif_enable(srv);
> +	}
>  
>  	pci_dbg(port, "enabled with IRQ %d\n", srv->irq);
>  
> @@ -159,6 +315,7 @@ static void pcie_bwnotif_remove(struct pcie_device *srv)
>  		srv->port->link_bwctrl = NULL;
>  
>  	free_irq(srv->irq, srv);
> +	mutex_destroy(&data->set_speed_mutex);
Probably not worth doing.  Also you don't do error handling for this above.
Ideal is use devm_ for data and then devm_mutex_init()


>  	kfree(data);
>  }
>