Re: [PATCH 1/5] firmware: stratix10-svc: Add mutex lock and unlock in stratix10 memory allocation/free

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jul 22, 2025 at 11:30:41AM -0500, Dinh Nguyen wrote:
> From: Mahesh Rao <mahesh.rao@altera.com>
> 
> This commit adds a mutex lock to protect the
> stratix10_svc_allocate_memory and
> stratix10_svc_free_memory functions to ensure
> thread safety when allocating and freeing memory.
> This prevents potential race conditions and ensures
> synchronization.

You have 72 columns to write a changelog in, please use it :)

And is this fixing a bug?  If so, shouldn't this be tagged for stable
and add a Fixes: tag?

If this isn't a bug, then why is it needed?  How can these race?

> 
> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
>  drivers/firmware/stratix10-svc.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index e3f990d888d7..73c77b8e9f2b 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) 2017-2018, Intel Corporation
> + * Copyright (C) 2025, Altera Corporation
>   */
>  
>  #include <linux/completion.h>
> @@ -171,6 +172,10 @@ struct stratix10_svc_chan {
>  
>  static LIST_HEAD(svc_ctrl);
>  static LIST_HEAD(svc_data_mem);
> +/* svc_mem_lock protects access to the svc_data_mem list for
> + * concurrent multi-client operations
> + */

Odd coding style, this isn't the network subsystem :(

And what about a lock for svc_ctrl?

> +static DEFINE_MUTEX(svc_mem_lock);
>  
>  /**
>   * svc_pa_to_va() - translate physical address to virtual address
> @@ -182,14 +187,18 @@ static LIST_HEAD(svc_data_mem);
>  static void *svc_pa_to_va(unsigned long addr)
>  {
>  	struct stratix10_svc_data_mem *pmem;
> +	void *ret = NULL;
>  
>  	pr_debug("claim back P-addr=0x%016x\n", (unsigned int)addr);
> +	mutex_lock(&svc_mem_lock);

Why not just use the guard() functionality instead?  Makes for much
simpler code and a smaller patch.  Please do so for all of these.

thanks,

greg k-h