[PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[Geert Uytterhoeven]

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(-)

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,
-- 
2.25.1

Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[Andy Shevchenko]

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>
One nit-pick below, though.

> 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(-)
>
> 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]);

This indentation kills the perfectionist in me :-)
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);

?

>  }
>
>  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,
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[Geert Uytterhoeven]

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

Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[Bartosz Golaszewski]

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