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

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

Hi Krzysztof,
	Thanks for reviewing the code.

On 27-05-2025 02:19 pm, Krzysztof Kozlowski wrote:
> 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.

Will make the change

> 
>>   
>>   /**
>>    * 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.

Shall I combine the both lock invocation together?
> 
> 
> 
> Best regards,
> Krzysztof