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]
Date: Wed, 3 Jan 2024 17:40:06 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Rob Herring <robh@...nel.org>, Krzysztof Wilczy??ski <kw@...ux.com>,
	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,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Alex Deucher <alexdeucher@...il.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Amit Kucheria <amitk@...nel.org>, Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH v3 08/10] PCI/bwctrl: Add "controller" part into PCIe
 bwctrl

On Mon, Jan 01, 2024 at 08:12:43PM +0200, Ilpo Järvinen wrote:
> On Sat, 30 Dec 2023, Lukas Wunner wrote:
> > On Fri, Sep 29, 2023 at 02:57:21PM +0300, Ilpo Järvinen wrote:
> > > --- a/drivers/pci/pcie/bwctrl.c
> > > +static u16 speed2lnkctl2(enum pci_bus_speed speed)
> > > +{
> > > +	static const u16 speed_conv[] = {
> > > +		[PCIE_SPEED_2_5GT] = PCI_EXP_LNKCTL2_TLS_2_5GT,
> > > +		[PCIE_SPEED_5_0GT] = PCI_EXP_LNKCTL2_TLS_5_0GT,
> > > +		[PCIE_SPEED_8_0GT] = PCI_EXP_LNKCTL2_TLS_8_0GT,
> > > +		[PCIE_SPEED_16_0GT] = PCI_EXP_LNKCTL2_TLS_16_0GT,
> > > +		[PCIE_SPEED_32_0GT] = PCI_EXP_LNKCTL2_TLS_32_0GT,
> > > +		[PCIE_SPEED_64_0GT] = PCI_EXP_LNKCTL2_TLS_64_0GT,
> > > +	};
> > 
> > Looks like this could be a u8 array to save a little bit of memory.
> > 
> > Also, this seems to duplicate pcie_link_speed[] defined in probe.c.
> 
> It's not the same. pcie_link_speed[] is indexed by a different thing

Ah, missed that.  Those various conversion functions are confusing.


> > > +	dev_speeds = READ_ONCE(bus->pcie_dev_speeds);
> > 
> > Hm, why is the compiler barrier needed?
> 
> It's probably an overkill but there could be a checker which finds this 
> read is not protected by anything while the value could get updated 
> concurrectly

Why should it be updated?  It's a static value cached on enumeration
and never changed AFAICS.


> If the value selected cannot be set because of endpoint no longer being 
> there, the other parts of the code will detect that.

If the endpoint is hot-unplugged, the link is down, so retraining
the link will fail.

If the device is replaced concurrently to retraining then you may
try to retrain to a speed which is too fast or too slow for the new
device.  Maybe it's necessary to cope with that?  Basically find the
pci_dev on the bus with the device/function id and check if the pci_dev
pointer still points to the same location.  Another idea would be to
hook up bwctrl with pciehp so that pciehp tells bwctrl the device is
gone and any speed changes should be aborted.  We've hooked up DPC
with pciehp in a similar way.


> So if I just add a comment to this line why there's the data race and keep 
> it as is?

I'd just drop the READ_ONCE() here if there's not a good reason for it.

Thanks,

Lukas

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ