On Thu, May 11, 2023 at 08:35:48PM +0300, Ilpo Järvinen wrote:
> 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.
Let's start with that, then.
Many of these are ASPM-related updates that IMHO should not be in
drivers at all. Drivers should use PCI core interfaces so the core
doesn't get confused.
Bjorn
