Re: [PATCH v3 08/10] PCI/bwctrl: Add "controller" part into PCIe bwctrl

[Lukas Wunner <lukas@wunner.de>]

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