Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2}

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

On Thu, 11 May 2023, Lukas Wunner wrote:

> On Thu, May 11, 2023 at 10:55:06AM -0500, Bjorn Helgaas wrote:
> > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > > +					      u16 clear, u16 set)
> > > +{
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> > 
> > I didn't see the prior discussion with Lukas, so maybe this was
> > answered there, but is there any reason not to add locking to
> > pcie_capability_clear_and_set_word() and friends directly?
> > 
> > It would be nice to avoid having to decide whether to use the locked
> > or unlocked versions.
> 
> I think we definitely want to also offer lockless accessors which
> can be used in hotpaths such as interrupt handlers if the accessed
> registers don't need any locking (e.g. because there are no concurrent
> accesses).
> 
> So the relatively lean approach chosen here which limits locking to
> Link Control and Link Control 2, but allows future expansion to other
> registers as well, seemed reasonable to me.

Hi Lukas,

I went through every single use of these functions in the mainline tree 
excluding LNKCTL/LNKCTL2 ones which will be having the lock anyway:

- pcie_capability_clear_and_set_*
- pcie_capability_set_*
- pcie_capability_clear_*

Everything outside of drivers/pci/ is dev init or dev reset related.

Almost all uses inside drivers/pci/ are init/configure/scan/PCI_FIXUP/pci_flr 
or suspend/resume related. With these exceptions:

->set_attention_status() drivers/pci/hotplug/pnv_php.c: pcie_capability_clear_and_set_word(bridge, PCI_EXP_SLTCTL,
spinlock + work (from pme.c) drivers/pci/pci.c: pcie_capability_set_dword(dev, PCI_EXP_RTSTA
spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_set_word(dev, PCI_EXP_RTCTL,
spinlock + irq / work drivers/pci/pcie/pme.c: pcie_capability_clear_word(dev, PCI_EXP_RTCTL,

So the only case which seems relevant to your concern are those in
drivers/pci/pcie/pme.c which already takes a spinlock so it's not lockless 
as is.

What's more important though, isn't it possible that AER and PME RMW
PCI_EXP_RTCTL at the same time so it would need this RMW locking too 
despite the pme internal spinlock?

Do you still feel there's a need to differentiate this per capability 
given all the information above?


There could of course be open-coded capability RMW ops outside of the ones 
I checked but I suspect the conclusion would still remain pretty much the 
same.


-- 
 i.