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

[Mahesh Rao]

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@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > Reviewed-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxx>
> > > > Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> > > > ---
> > > >    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