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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux