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: <20241017114812.00005e67@Huawei.com>
Date: Thu, 17 Oct 2024 11:48:12 +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 5/8] PCI/bwctrl: Re-add BW notification portdrv as
 PCIe BW controller

On Wed,  9 Oct 2024 12:52:20 +0300
Ilpo Järvinen <ilpo.jarvinen@...ux.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@...ner.de> # Building bwctrl on top of bwnotif
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.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.


> +}


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ