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

Open Source and information security mailing list archives

Message-ID: <20190406230632.GB9140@madhuleo>
Date:   Sat, 6 Apr 2019 18:06:33 -0500
From:   Madhumthia Prabakaran <madhumithabiw@...il.com>
To:     Alex Elder <elder@...aro.org>, dan.carpenter@...cle.com,
        devel@...verdev.osuosl.org, johan@...nel.org,
        linux-kernel@...r.kernel.org, greybus-dev@...ts.linaro.org
Subject: Re: [greybus-dev] [PATCH] Staging: greybus: Fix spinlock_t
 definition without comment

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@...il.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@...ts.linaro.org 
> > https://lists.linaro.org/mailman/listinfo/greybus-dev
> > 
> 

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives