Re: [greybus-dev] [PATCH] Staging: greybus: Fix spinlock_t definition without comment

[Madhumthia Prabakaran <madhumithabiw@gmail.com>]

On Fri, Apr 05, 2019 at 05:50:10PM -0500, Alex Elder wrote:
> On 4/5/19 3:53 PM, Dan Carpenter wrote:
> > On Fri, Apr 05, 2019 at 03:00:46PM -0500, Madhumitha Prabakaran
> > wrote:
> >> Fix spinlock_t definition without comment.
> >> 
> >> Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> 
> Madhumitha, the reason one would want a comment associated with
> a lock field in a structure is to get some understanding of why
> it's needed.  Saying "protect structure fields" is not enough,
> because that can pretty much be assumed, so a comment like that
> adds no value.

That's true. It doesn't make much sense.

> 
> Looking at the code, you can see the lock field protects the
> connection's operations list.  It also appears to be needed
> for accessing the state field (reading or updating).
> 

Along with that, in some cases the spinlocks are protecting hd_links and
bundle_links list.

Lines : drivers/staging/greybus/connection.c#895 #896


> Given that, a better comment might be:
> 
>         spinlock_t                      lock;	/* operations list and state */
> 
> >> --- drivers/staging/greybus/connection.h | 2 +- 1 file changed, 1
> >> insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/staging/greybus/connection.h
> >> b/drivers/staging/greybus/connection.h index
> >> 5ca3befc0636..0aedd246e94a 100644 ---
> >> a/drivers/staging/greybus/connection.h +++
> >> b/drivers/staging/greybus/connection.h @@ -47,7 +47,7 @@ struct
> >> gb_connection { unsigned long			flags;
> >> 
> >> struct mutex			mutex; -	spinlock_t			lock; +	spinlock_t			lock; /*
> >> Protect structure fields */ enum gb_connection_state	state;
> > 
> > What does the mutex do then?  Why can't we just use the spinlock for 
> > everything?
> 
> The mutex needs to be held during enable and disable of a connection.
> Johan might be able to give you a more complete answer but these
> operations (or at least the enable) need to block, so can't hold a
> spinlock.
> 
> 					-Alex

Thanks for explaining it. Checking on the code, mutexes protect spinlock_t
for gb_connection_state fields and gb_connection_state fields itself in
struct gb_connection.

> 
> > I did glance at the code and it wasn't immediately obvious to me.
> > 
> > regards, dan carpenter
> > 
> > _______________________________________________ greybus-dev mailing
> > list greybus-dev@lists.linaro.org 
> > https://lists.linaro.org/mailman/listinfo/greybus-dev
> > 
>