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

[Matthew Gerlach <matthew.gerlach@altera.com>]

On 6/10/25 8:37 AM, Mahesh Rao via B4 Relay 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.
>
> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
> ---
>   drivers/firmware/stratix10-svc.c | 30 +++++++++++++++++++++++-------
>   1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index e3f990d888d71829f0ab22b8a59aa7af0316bea0..955468555738b2031dfcf6dc4db7dbf11ccc482c 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
> + */
> +static DEFINE_MUTEX(svc_mem_lock);
>   
>   /**
>    * svc_pa_to_va() - translate physical address to virtual address
> @@ -182,14 +187,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;

I think you want a "break;" here.


Matthew Geralch

> +		}
> +	mutex_unlock(&svc_mem_lock);
> +	return ret;
>   }
>   
>   /**
> @@ -990,6 +998,7 @@ 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;
> @@ -1006,6 +1015,7 @@ int stratix10_svc_send(struct stratix10_svc_chan *chan, void *msg)
>   					break;
>   				}
>   		}
> +		mutex_unlock(&svc_mem_lock);
>   	}
>   
>   	p_data->command = p_msg->command;
> @@ -1072,9 +1082,12 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>   	if (!pmem)
>   		return ERR_PTR(-ENOMEM);
>   
> +	mutex_lock(&svc_mem_lock);
>   	va = gen_pool_alloc(genpool, s);
> -	if (!va)
> +	if (!va) {
> +		mutex_unlock(&svc_mem_lock);
>   		return ERR_PTR(-ENOMEM);
> +	}
>   
>   	memset((void *)va, 0, s);
>   	pa = gen_pool_virt_to_phys(genpool, va);
> @@ -1086,6 +1099,7 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>   	pr_debug("%s: va=%p, pa=0x%016x\n", __func__,
>   		 pmem->vaddr, (unsigned int)pmem->paddr);
>   
> +	mutex_unlock(&svc_mem_lock);
>   	return (void *)va;
>   }
>   EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
> @@ -1100,6 +1114,7 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>   void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>   {
>   	struct stratix10_svc_data_mem *pmem;
> +	mutex_lock(&svc_mem_lock);
>   
>   	list_for_each_entry(pmem, &svc_data_mem, node)
>   		if (pmem->vaddr == kaddr) {
> @@ -1107,9 +1122,10 @@ void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>   				       (unsigned long)kaddr, pmem->size);
>   			pmem->vaddr = NULL;
>   			list_del(&pmem->node);
> +			mutex_unlock(&svc_mem_lock);
>   			return;
>   		}
> -
> +	mutex_unlock(&svc_mem_lock);
>   	list_del(&svc_data_mem);
>   }
>   EXPORT_SYMBOL_GPL(stratix10_svc_free_memory);
>