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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d6999d74-45f6-413a-8881-90473b322dfa@linux.ibm.com>
Date: Fri, 14 Mar 2025 15:00:17 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: ajd@...ux.ibm.com
Cc: npiggin@...il.com, christophe.leroy@...roup.eu, mpe@...erman.id.au,
        peterz@...radead.org, fbarrat@...ux.ibm.com, mahesh@...ux.ibm.com,
        oohall@...il.com, hbathini@...ux.ibm.com, dhowells@...hat.com,
        haren@...ux.ibm.com, linux-kernel@...r.kernel.org, maddy@...ux.ibm.com,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH 5/6] powerpc: powenv: oxcl: use lock guard for mutex



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@...ux.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.

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