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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 15 Apr 2019 14:59:49 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Alex Elder <elder@...aro.org>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        Madhumitha Prabakaran <madhumithabiw@...il.com>,
        devel@...verdev.osuosl.org, elder@...nel.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.
> 
> 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

Powered by blists - more mailing lists