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

[Greg KH <gregkh@linuxfoundation.org>]

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?

> 
> > 
> > > 
> > > 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?

thanks,

greg k-h