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

[Ilpo Järvinen]

On Thu, 11 May 2023, Bjorn Helgaas wrote:

> On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > A few places write LNKCTL and LNKCTL2 registers without proper
> > concurrency control and this could result in losing the changes
> > one of the writers intended to make.
> > 
> > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > spinlock in the struct pci_dev.
> > 
> > Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> 
> Thanks for raising this issue!  Definitely looks like something that
> needs attention.
> 
> > ---
> >  drivers/pci/access.c | 14 ++++++++++++++
> >  drivers/pci/probe.c  |  1 +
> >  include/linux/pci.h  | 17 +++++++++++++++++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 3c230ca3de58..d92a3daadd0c 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> >  }
> >  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> >  
> > +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.  It would also be nice to preserve the use of
> PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
> need for some of these patches, e.g., the use of
> pcie_capability_clear_word(), where it's not obvious at the call site
> why a change is needed.

There wasn't that big discussion about it (internally). I brought both
alternatives up and Lukas just said he didn't know what's the best 
approach (+ gave a weak nudge towards the separate accessor so I went 
with it to make forward progress). Based on that I don't think he had a 
strong opinion on it.

I'm certainly fine to just use it in the normal accessor functions that 
do RMW and add the locking there. It would certainly have to those good 
sides you mentioned.

-- 
 i.