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.
