[PATCH] Staging: greybus: Fix spinlock_t definition without comment

[PATCH] Staging: greybus: Fix spinlock_t definition without comment

[Madhumitha Prabakaran]

Fix spinlock_t definition without comment.

Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
---
 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;
 	struct list_head		operations;
 
-- 
2.17.1

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

[Dan Carpenter]

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

I did glance at the code and it wasn't immediately obvious to me.

regards,
dan carpenter

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

[Alex Elder]

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.

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

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

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

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

[Madhumthia Prabakaran]

On Fri, Apr 05, 2019 at 11:53:04PM +0300, 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>
> > ---
> >  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?

Here, mutexes protect spinlock_t for gb_connection_state fields and
gb_connection_state fields itself in struct gb_connection.

However, I wasn't sure then and now exactly, how to find out which resources in the
structure, the spinlock or mutex is performing kernel synchronization.

> 
> I did glance at the code and it wasn't immediately obvious to me.

Thanks for reviewing it.

> 
> regards,
> dan carpenter
> 

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

[Madhumthia Prabakaran]

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

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

[Johan Hovold]

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

Yeah, I should have documented this at the time.

You're right that the connection spinlock protects the operation list,
and the mutex the connection state, but there are some other
dependencies here and I don't have time to look at this at the moment.

Better to leave as is, I'd say.

Johan