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

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

Hi Greg,
	thanks for reviewing the code.

On 19-08-2025 04:36 pm, Greg KH wrote:
> 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?

In the current implementation, all operations were performed serially, 
eliminating the need for protection mechanisms. However, with this patch 
set, we are introducing parallel access and communication with the SDM 
across multiple client drivers. This change may lead to race conditions 
involving the svc_data_mem list.

> 
>>
>> 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 :(

Ok sure, will change

> 
> And what about a lock for svc_ctrl?

There is only one instance of svc_ctrl and there is no parallel access 
to it.so a lock is not required as of now.


> 
>> +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.

sure we will add this.
> 
> thanks,
> 
> greg k-h