Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 6 Apr 2019 18:06:33 -0500
From:   Madhumthia Prabakaran <madhumithabiw@gmail.com>
To:     Alex Elder <elder@linaro.org>, dan.carpenter@oracle.com,
        devel@driverdev.osuosl.org, johan@kernel.org,
        linux-kernel@vger.kernel.org, greybus-dev@lists.linaro.org
Subject: Re: [greybus-dev] [PATCH] Staging: greybus: Fix spinlock_t
 definition without comment
Message-ID: <20190406230632.GB9140@madhuleo>
References: <20190405200046.20600-1-madhumithabiw@gmail.com>
 <20190405205304.GS32590@kadam>
 <fd59f7d5-84a0-35ca-b096-a1d2c7e30336@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fd59f7d5-84a0-35ca-b096-a1d2c7e30336@linaro.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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