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

[Mahesh Rao <mahesh.rao@altera.com>]

Hi Matthew,

On 10-06-2025 09:36 pm, Matthew Gerlach wrote:
> 
> 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

Will make the change.
> 
>> +        }
>> +    mutex_unlock(&svc_mem_lock);
>> +    return ret;
>>   }
>>   /**
>> @@ -990,6 +998,7 @@ int stratix10_svc_send(struct stratix10_svc_chan