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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

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@infradead.org/T/#u
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   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;
>