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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241017120211.00005b1e@Huawei.com>
Date: Thu, 17 Oct 2024 12:02:11 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-pci@...r.kernel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, "Lorenzo
 Pieralisi" <lorenzo.pieralisi@....com>, Rob Herring <robh@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>, "Maciej W. Rozycki"
	<macro@...am.me.uk>, Lukas Wunner <lukas@...ner.de>, Alexandru Gagniuc
	<mr.nuke.me@...il.com>, Krishna chaitanya chundru <quic_krichai@...cinc.com>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, "Rafael J.
 Wysocki" <rafael@...nel.org>, <linux-pm@...r.kernel.org>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@....com>, <linux-kernel@...r.kernel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>, Amit Kucheria <amitk@...nel.org>,
	Zhang Rui <rui.zhang@...el.com>, Christophe JAILLET
	<christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH v8 6/8] PCI/bwctrl: Add API to set PCIe Link Speed

On Wed,  9 Oct 2024 12:52:21 +0300
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> 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@...ux.intel.com>

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@...wei.com>

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);
>  }
>