Message ID | 20231125-dropcomment-v1-1-15800415aae0@linaro.org |
---|---|
State | New |
Headers | show |
Series | gpiolib: Drop cargo-culted comment | expand |
On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote: > This comment about the gpio_lock is just completely confusing and > misleading. This refers to a gpio_desc that would in 2008 be used > to hold the list of gpio_chips, but nowadays gpio_desc refers to > descriptors of individual GPIO lines and this comment is completely > unparseable. Delete it. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpiolib.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 95d2a7b2ea3e..1c47af866bf6 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -86,10 +86,6 @@ static struct bus_type gpio_bus_type = { > */ > #define FASTPATH_NGPIO CONFIG_GPIOLIB_FASTPATH_LIMIT > > -/* gpio_lock prevents conflicts during gpio_desc[] table updates. > - * While any GPIO is requested, its gpio_chip is not removable; > - * each GPIO's "requested" flag serves as a lock and refcount. > - */ Perhaps provide a comment as to what the gpio_lock DOES cover? > DEFINE_SPINLOCK(gpio_lock); > > static DEFINE_MUTEX(gpio_lookup_lock); > > --- > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86 > change-id: 20231125-dropcomment-89e5b7b4cc3f > > Best regards, > -- > Linus Walleij <linus.walleij@linaro.org> >
On Sat, Nov 25, 2023 at 3:40 AM Kent Gibson <warthog618@gmail.com> wrote: > On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote: > > -/* gpio_lock prevents conflicts during gpio_desc[] table updates. > > - * While any GPIO is requested, its gpio_chip is not removable; > > - * each GPIO's "requested" flag serves as a lock and refcount. > > - */ > > Perhaps provide a comment as to what the gpio_lock DOES cover? Normally yes, but Bartosz just said he is going to replace this spinlock with a mutex so it's better if he adds it then. Yours, Linus Walleij
On Sun, Nov 26, 2023 at 12:05:08AM +0100, Linus Walleij wrote: > On Sat, Nov 25, 2023 at 3:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote: > > > > -/* gpio_lock prevents conflicts during gpio_desc[] table updates. > > > - * While any GPIO is requested, its gpio_chip is not removable; > > > - * each GPIO's "requested" flag serves as a lock and refcount. > > > - */ > > > > Perhaps provide a comment as to what the gpio_lock DOES cover? > > Normally yes, but Bartosz just said he is going to replace this spinlock > with a mutex so it's better if he adds it then. > If that is happening soon then leave it to Bart to change both the comment and lock. If not, then we now have an undocumented lock. If the coverage of the spinlock and proposed mutex are the same why not describe what the lock covers now? Then Bart wont have to update the comment. Cheers, Kent.
On Sun, Nov 26, 2023 at 1:14 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sun, Nov 26, 2023 at 12:05:08AM +0100, Linus Walleij wrote: > > On Sat, Nov 25, 2023 at 3:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > > On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote: > > > > > > -/* gpio_lock prevents conflicts during gpio_desc[] table updates. > > > > - * While any GPIO is requested, its gpio_chip is not removable; > > > > - * each GPIO's "requested" flag serves as a lock and refcount. > > > > - */ > > > > > > Perhaps provide a comment as to what the gpio_lock DOES cover? > > > > Normally yes, but Bartosz just said he is going to replace this spinlock > > with a mutex so it's better if he adds it then. > > > > If that is happening soon then leave it to Bart to change both the > comment and lock. > > If not, then we now have an undocumented lock. If the coverage of the > spinlock and proposed mutex are the same why not describe what the lock > covers now? Then Bart wont have to update the comment. > > Cheers, > Kent. > Yeah, I think we should maybe leave some temporary FIXME comment once the mutex patch is in saying this must go as well but it'll take more time because the problem is quite tricky. Bart
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 95d2a7b2ea3e..1c47af866bf6 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -86,10 +86,6 @@ static struct bus_type gpio_bus_type = { */ #define FASTPATH_NGPIO CONFIG_GPIOLIB_FASTPATH_LIMIT -/* gpio_lock prevents conflicts during gpio_desc[] table updates. - * While any GPIO is requested, its gpio_chip is not removable; - * each GPIO's "requested" flag serves as a lock and refcount. - */ DEFINE_SPINLOCK(gpio_lock); static DEFINE_MUTEX(gpio_lookup_lock);
This comment about the gpio_lock is just completely confusing and misleading. This refers to a gpio_desc that would in 2008 be used to hold the list of gpio_chips, but nowadays gpio_desc refers to descriptors of individual GPIO lines and this comment is completely unparseable. Delete it. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpiolib.c | 4 ---- 1 file changed, 4 deletions(-) --- base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86 change-id: 20231125-dropcomment-89e5b7b4cc3f Best regards,