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