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