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 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 6 Apr 2019 18:06:33 -0500 From: Madhumthia Prabakaran <madhumithabiw@...il.com> To: Alex Elder <elder@...aro.org>, dan.carpenter@...cle.com, devel@...verdev.osuosl.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. 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@...ts.linaro.org > > https://lists.linaro.org/mailman/listinfo/greybus-dev > > >
Powered by blists - more mailing lists