Re: [PATCH v5 5/8] PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed,  8 May 2024 16:47:41 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> This mostly reverts the commit b4c7d2076b4e ("PCI/LINK: Remove
> bandwidth notification"). An upcoming commit extends this driver
> building PCIe bandwidth controller on top of it.
> 
> The PCIe bandwidth notification were first added in the commit
> e8303bb7a75c ("PCI/LINK: Report degraded links via link bandwidth
> notification") but later had to be removed. The significant changes
> compared with the old bandwidth notification driver include:
> 
> 1) Don't print the notifications into kernel log, just keep the Link
>    Speed cached into the struct pci_bus updated. While somewhat
>    unfortunate, the log spam was the source of complaints that
>    eventually lead to the removal of the bandwidth notifications driver
>    (see the links below for further information).
> 
> 2) Besides the Link Bandwidth Management Interrupt, enable also Link
>    Autonomous Bandwidth Interrupt to cover the other source of
>    bandwidth changes.
> 
> 3) Use threaded IRQ with IRQF_ONESHOT to handle Bandwidth Notification
>    Interrupts to address the problem fixed in the commit 3e82a7f9031f
>    ("PCI/LINK: Supply IRQ handler so level-triggered IRQs are acked")).
> 
> 4) Handle Link Speed updates robustly. Refresh the cached Link Speed
>    when enabling Bandwidth Notification Interrupts, and solve the race
>    between Link Speed read and LBMS/LABS update in
>    pcie_bwnotif_irq_thread().
> 
> 5) Use concurrency safe LNKCTL RMW operations.
> 
> 6) The driver is now called PCIe bwctrl (bandwidth controller) instead
>    of just bandwidth notifications because of increased scope and
>    functionality within the driver.
> 
> 7) Coexist with the Target Link Speed quirk in
>    pcie_failed_link_retrain(). Provide LBMS counting API for it.
> 
> 8) Tweaks to variable/functions names for consistency and length
>    reasons.
> 
> Bandwidth Notifications enable the cur_bus_speed in the struct pci_bus
> to keep track PCIe Link Speed changes.
> 
> Link: https://lore.kernel.org/all/20190429185611.121751-1-helgaas@kernel.org/
> Link: https://lore.kernel.org/linux-pci/20190501142942.26972-1-keith.busch@intel.com/
> Link: https://lore.kernel.org/linux-pci/20200115221008.GA191037@google.com/
> Suggested-by: Lukas Wunner <lukas@wunner.de> # Building bwctrl on top of bwnotif
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

A few trivial things inline. Either way LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 6461aa93fe76..6357bc219632 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -12,4 +12,5 @@ obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
>  obj-$(CONFIG_PCIE_PME)		+= pme.o
>  obj-$(CONFIG_PCIE_DPC)		+= dpc.o
>  obj-$(CONFIG_PCIE_PTM)		+= ptm.o
> +obj-$(CONFIG_PCIE_BWCTRL)	+= bwctrl.o
>  obj-$(CONFIG_PCIE_EDR)		+= edr.o
> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
> new file mode 100644
> index 000000000000..5afc533dd0a9
> --- /dev/null
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -0,0 +1,185 @@

> +
> +static irqreturn_t pcie_bwnotif_irq_thread(int irq, void *context)
> +{
> +	struct pcie_device *srv = context;
> +	struct pcie_bwctrl_data *data = get_service_data(srv);
> +	struct pci_dev *port = srv->port;
> +	u16 link_status, events;
> +	int ret;
> +
> +	ret = pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status);
> +	events = link_status & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS);
> +
> +	if (ret != PCIBIOS_SUCCESSFUL || !events)
> +		return IRQ_NONE;

Trivial, but nicer to not use link_status if it is garbage (even briefly)
Only a couple of lines more to keep it clean.

	ret = pcie...
	if (ret != PCI_BIOS_SUCCESSFUL)
		return IRQ_NONE;

	events = ...
	if (!events)
		return IRQ_NONE;

> +
> +	if (events & PCI_EXP_LNKSTA_LBMS)
> +		atomic_inc(&data->lbms_count);
> +
> +	pcie_capability_write_word(port, PCI_EXP_LNKSTA, events);
> +
> +	/*
> +	 * Interrupts will not be triggered from any further Link Speed
> +	 * change until LBMS is cleared by the write. Therefore, re-read the
> +	 * speed (inside pcie_update_link_speed()) after LBMS has been
> +	 * cleared to avoid missing link speed changes.
> +	 */
> +	pcie_update_link_speed(port->subordinate);
> +
> +	return IRQ_HANDLED;
> +}

> +
> +static int pcie_bwnotif_probe(struct pcie_device *srv)
> +{
> +	struct pci_dev *port = srv->port;
> +	int ret;
> +
> +	struct pcie_bwctrl_data *data __free(kfree) =
> +				kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	set_service_data(srv, data);
> +
> +	ret = request_threaded_irq(srv->irq, NULL, pcie_bwnotif_irq_thread,
> +				   IRQF_SHARED | IRQF_ONESHOT, "PCIe bwctrl", srv);
> +	if (ret)
> +		return ret;
> +
> +	port->link_bwctrl = no_free_ptr(data);
> +	pcie_bwnotif_enable(srv);
> +	pci_info(port, "enabled with IRQ %d\n", srv->irq);

Rather noisy given this is easy enough to establish via other paths.
pci_dbg() maybe?

> +
> +	return 0;
> +}
> +
> +static void pcie_bwnotif_remove(struct pcie_device *srv)
> +{
> +	struct pcie_bwctrl_data *data = get_service_data(srv);
> +
> +	scoped_guard(rwsem_write, &pcie_bwctrl_remove_rwsem)
> +		srv->port->link_bwctrl = NULL;
> +
> +	pcie_bwnotif_disable(srv->port);

Trivial but I'd like a comment to say why this needs to be done after
the link_bwctrl = NULL above (or if not, move it before that.
That puts the tear down slightly out of order vs set up.

> +	free_irq(srv->irq, srv);
> +	kfree(data);
> +}
> +