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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/05/2025 08:25, Mahesh Rao via B4 Relay wrote:
> From: Mahesh Rao <mahesh.rao@altera.com>
> 
> This commit adds a mutex lock to 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.
> 
> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
> ---
>  drivers/firmware/stratix10-svc.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index e3f990d888d71829f0ab22b8a59aa7af0316bea0..3d42d4b18b7299d0a9e5110159e06253dfeddf88 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,7 @@ struct stratix10_svc_chan {
>  
>  static LIST_HEAD(svc_ctrl);
>  static LIST_HEAD(svc_data_mem);
> +static DEFINE_MUTEX(svc_mem_lock);

You need to document what you are protecting here.

>  
>  /**
>   * svc_pa_to_va() - translate physical address to virtual address
> @@ -182,14 +184,17 @@ 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);
>  	list_for_each_entry(pmem, &svc_data_mem, node)
> -		if (pmem->paddr == addr)
> -			return pmem->vaddr;
> -
> -	/* physical address is not found */
> -	return NULL;
> +		if (pmem->paddr == addr) {
> +			/* physical address is found */
> +			ret = pmem->vaddr;
> +		}
> +	mutex_unlock(&svc_mem_lock);
> +	return ret;
>  }
>  
>  /**
> @@ -990,13 +995,16 @@ int stratix10_svc_send(struct stratix10_svc_chan *chan, void *msg)
>  			p_data->flag = ct->flags;
>  		}
>  	} else {
> +		mutex_lock(&svc_mem_lock);
>  		list_for_each_entry(p_mem, &svc_data_mem, node)
>  			if (p_mem->vaddr == p_msg->payload) {
>  				p_data->paddr = p_mem->paddr;
>  				p_data->size = p_msg->payload_length;
>  				break;
>  			}
> +		mutex_unlock(&svc_mem_lock);
>  		if (p_msg->payload_output) {
> +			mutex_lock(&svc_mem_lock);

Especially that this looks odd.



Best regards,
Krzysztof