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

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

On 22-08-2025 05:19 pm, Greg KH wrote:
> On Fri, Aug 22, 2025 at 03:17:54PM +0530, Mahesh Rao wrote:
>> 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.
> 
> Then that needs to be said here :)
> 
> Also, what is causing these operations to be performed serially if there
> is no locking?

Use case wise, currently we are only accessing this serially. But I 
agree that there is a scenario where parallel access is possible. So I 
shall add fixes tag in the next revision.

> 
>>
>>>
>>>>
>>>> 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.
> 
> But don't you have multiple places that list can be accessed now at the
> same time?
> 
> In other words, what is changing to require one list to require it but
> not the other?  Is there some other lock for that?

This is just a context controller having only 1 instance as of now and 
this context object is only updated during driver init and read in 
another function.

> 
> thanks,
> 
> greg k-h

Thanks
Mahesh