| Message ID | 6a62a531227cd4f20d77d50cdde60c7a18b9f052.1643625325.git.geert+renesas@glider.be |
|---|---|
| State | Accepted |
| Commit | 2cba05451a6d0c703bb74f1a250691404f27c4f1 |
| Headers | show |
| Series | gpio: aggregator: Fix calling into sleeping GPIO controllers | expand |
Hi Andy, On Tue, Feb 1, 2022 at 9:35 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > If the parent GPIO controller is a sleeping controller (e.g. a GPIO > > controller connected to I2C), getting or setting a GPIO triggers a > > might_sleep() warning. This happens because the GPIO Aggregator takes > > the can_sleep flag into account only for its internal locking, not for > > calling into the parent GPIO controller. > > > > Fix this by using the gpiod_[gs]et*_cansleep() APIs when calling into a > > sleeping GPIO controller. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Thanks! > > +++ b/drivers/gpio/gpio-aggregator.c > > @@ -278,7 +278,8 @@ static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset) > > { > > struct gpiochip_fwd *fwd = gpiochip_get_data(chip); > > > > - return gpiod_get_value(fwd->descs[offset]); > > > + return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offset]) > > + : gpiod_get_value(fwd->descs[offset]); > > This indentation kills the perfectionist in me :-) Why? The above is aligned perfectly ("?" just above ":")? > What about: > > return chip->can_sleep ? > gpiod_get_value_cansleep(fwd->descs[offset]) : > gpiod_get_value(fwd->descs[offset]); > > ? > > Or as variant > > struct gpio_desc *desc = fwd->descs[offset]; > > return chip->can_sleep ? gpiod_get_value_cansleep(desc) : > gpiod_get_value(desc); > > ? IMHO, those are ugly as hell ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Feb 1, 2022 at 9:59 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Feb 1, 2022 at 10:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Feb 1, 2022 at 9:35 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven > > > <geert+renesas@glider.be> wrote: > > ... > > > > > + return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offset]) > > > > + : gpiod_get_value(fwd->descs[offset]); > > > > > > This indentation kills the perfectionist in me :-) > > > > Why? The above is aligned perfectly ("?" just above ":")? > > > > > What about: > > > > > > return chip->can_sleep ? > > > gpiod_get_value_cansleep(fwd->descs[offset]) : > > > gpiod_get_value(fwd->descs[offset]); > > > > > > ? > > > > > > Or as variant > > > > > > struct gpio_desc *desc = fwd->descs[offset]; > > > > > > return chip->can_sleep ? gpiod_get_value_cansleep(desc) : > > > gpiod_get_value(desc); > > > > > > ? > > > > IMHO, those are ugly as hell ;-) > > I have the same opinion about your initial variant. :-) > > So, up to the maintainer(s) what to do. > It's Geert's code so let's keep his version. I like it better myself too. Queued for fixes. Bart
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index 869dc952cf45218b..0cb2664085cf8314 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -278,7 +278,8 @@ static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset) { struct gpiochip_fwd *fwd = gpiochip_get_data(chip); - return gpiod_get_value(fwd->descs[offset]); + return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offset]) + : gpiod_get_value(fwd->descs[offset]); } static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask, @@ -293,7 +294,10 @@ static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask, for_each_set_bit(i, mask, fwd->chip.ngpio) descs[j++] = fwd->descs[i]; - error = gpiod_get_array_value(j, descs, NULL, values); + if (fwd->chip.can_sleep) + error = gpiod_get_array_value_cansleep(j, descs, NULL, values); + else + error = gpiod_get_array_value(j, descs, NULL, values); if (error) return error; @@ -328,7 +332,10 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value) { struct gpiochip_fwd *fwd = gpiochip_get_data(chip); - gpiod_set_value(fwd->descs[offset], value); + if (chip->can_sleep) + gpiod_set_value_cansleep(fwd->descs[offset], value); + else + gpiod_set_value(fwd->descs[offset], value); } static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask, @@ -343,7 +350,10 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask, descs[j++] = fwd->descs[i]; } - gpiod_set_array_value(j, descs, NULL, values); + if (fwd->chip.can_sleep) + gpiod_set_array_value_cansleep(j, descs, NULL, values); + else + gpiod_set_array_value(j, descs, NULL, values); } static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
If the parent GPIO controller is a sleeping controller (e.g. a GPIO controller connected to I2C), getting or setting a GPIO triggers a might_sleep() warning. This happens because the GPIO Aggregator takes the can_sleep flag into account only for its internal locking, not for calling into the parent GPIO controller. Fix this by using the gpiod_[gs]et*_cansleep() APIs when calling into a sleeping GPIO controller. Reported-by: Mikko Salomäki <ms@datarespons.se> Fixes: 828546e24280f721 ("gpio: Add GPIO Aggregator") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- I considered splitting the .[gs]et{_multiple}() callbacks for the sleeping vs. nonsleeping cases, but the code size increase (measured on ARM) would be substantial: +64 bytes for gpio_fwd_[gs]et_cansleep(), +296 bytes for gpio_fwd_[gs]et_multiple_cansleep(). --- drivers/gpio/gpio-aggregator.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)