Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex

[Shrikanth Hegde]

On 3/14/25 15:00, Shrikanth Hegde wrote:
> On 3/14/25 11:15, Shrikanth Hegde wrote:
> > use guard(mutex) for scope based resource management of mutex.
> > This would make the code simpler and easier to maintain.
> >
> > More details on lock guards can be found at
> > https://lore.kernel.org/all/20230612093537.614161713@xxxxxxxxxxxxx/T/#u
> >
> > Signed-off-by: Shrikanth Hegde <sshegde@xxxxxxxxxxxxx>
> > ---
> >   arch/powerpc/platforms/powernv/ocxl.c | 12 +++---------
> >   1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/ 
> > platforms/powernv/ocxl.c
> > index 64a9c7125c29..f8139948348e 100644
> > --- a/arch/powerpc/platforms/powernv/ocxl.c
> > +++ b/arch/powerpc/platforms/powernv/ocxl.c
> > @@ -172,12 +172,11 @@ static void pnv_ocxl_fixup_actag(struct pci_dev 
> > *dev)
> >       if (phb->type != PNV_PHB_NPU_OCAPI)
> >           return;
> > -    mutex_lock(&links_list_lock);
> > +    guard(mutex)(&links_list_lock);
> >       link = find_link(dev);
> >       if (!link) {
> >           dev_warn(&dev->dev, "couldn't update actag information\n");
> > -        mutex_unlock(&links_list_lock);
> >           return;
> >       }
> > @@ -206,7 +205,6 @@ static void pnv_ocxl_fixup_actag(struct pci_dev *dev)
> >       dev_dbg(&dev->dev, "total actags for function: %d\n",
> >           link->fn_desired_actags[PCI_FUNC(dev->devfn)]);
> > -    mutex_unlock(&links_list_lock);
> >   }
> >   DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pnv_ocxl_fixup_actag);
> > @@ -253,12 +251,11 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 
> > *base, u16 *enabled,
> >   {
> >       struct npu_link *link;
> > -    mutex_lock(&links_list_lock);
> > +    guard(mutex)(&links_list_lock);
> >       link = find_link(dev);
> >       if (!link) {
> >           dev_err(&dev->dev, "actag information not found\n");
> > -        mutex_unlock(&links_list_lock);
> >           return -ENODEV;
> >       }
> >       /*
> > @@ -274,7 +271,6 @@ int pnv_ocxl_get_actag(struct pci_dev *dev, u16 
> > *base, u16 *enabled,
> >       *enabled   = link->fn_actags[PCI_FUNC(dev->devfn)].count;
> >       *supported = link->fn_desired_actags[PCI_FUNC(dev->devfn)];
> > -    mutex_unlock(&links_list_lock);
> >       return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(pnv_ocxl_get_actag);
> > @@ -293,12 +289,11 @@ int pnv_ocxl_get_pasid_count(struct pci_dev 
> > *dev, int *count)
> >        *
> >        * We only support one AFU-carrying function for now.
> >        */
> > -    mutex_lock(&links_list_lock);
> > +    guard(mutex)(&links_list_lock);
> >       link = find_link(dev);
> >       if (!link) {
> >           dev_err(&dev->dev, "actag information not found\n");
> > -        mutex_unlock(&links_list_lock);
> >           return -ENODEV;
> >       }
> > @@ -309,7 +304,6 @@ int pnv_ocxl_get_pasid_count(struct pci_dev *dev, 
> > int *count)
> >               break;
> >           }
> > -    mutex_unlock(&links_list_lock);
>
> Hi. Andrew,
>
> After this change below dev_dbg will be called with mutex held still. Is 
> that a concern? I don't see the mutex being used in that path.
>
> Since using scoped_guard cause more code churn here, I would prefer not 
> use it.

I see current code in pnv_ocxl_fixup_actag calls dev_dbg with mutex 
held. So likely not a concern of using just guard in 
pnv_ocxl_get_pasid_count as well.

Assuming that, let me send out v2 with corrected commit subject. :w

> >       dev_dbg(&dev->dev, "%d PASIDs available for function\n",
> >           rc ? 0 : *count);
> >       return rc;