Messages in this thread | | | Date | Fri, 12 May 2023 11:25:32 +0300 (EEST) | From | Ilpo Järvinen <> | Subject | Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants for LNKCTL{,2} |
| |
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. | |