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

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

On Wed,  9 Oct 2024 12:52:20 +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

cached in struct pci_bus updated.

>    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).
> 
> 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>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

A couple of comments inline, but they are things that I plan
to tackle for other service drivers as part of the first stage
of the rework discussed at LPC.

I don't mind just including patches for this code in there
it you'd prefer to minimize changes at this point.

Jonathan

> +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_dbg(port, "enabled with IRQ %d\n", srv->irq);
> +
> +	return 0;
> +}
> +
> +static void pcie_bwnotif_remove(struct pcie_device *srv)
> +{
> +	struct pcie_bwctrl_data *data = get_service_data(srv);

this is the same as srv->port->link_bwctrl I think
Can we kill of the service data use?  That's one of the early
changes I have in cleaning up portdrv in general.  Aim
is to make it all look much more like you have here with
explicit pointers in port.

If not I'll just sweep that up in the same cleanup set.
(I'm hoping this merges soon to make that exercise easier!)

> +
> +	pcie_bwnotif_disable(srv->port);
> +	scoped_guard(rwsem_write, &pcie_bwctrl_remove_rwsem)
> +		srv->port->link_bwctrl = NULL;
> +
> +	free_irq(srv->irq, srv);
> +	kfree(data);

Also on that cleanup plan is using devm_ for all this
stuff in the portdrv service drivers (squashing them
into one driver at the same time).
Maybe worth doing at least the data and irq handling now
but I'm also fine just rolling it into that mega exercise.


> +}