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 <[email protected]>
Fixes: 828546e24280f721 ("gpio: Add GPIO Aggregator")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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
Hi Andy,
On Tue, Feb 1, 2022 at 9:35 PM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven
> <[email protected]> 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 <[email protected]>
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 -- [email protected]
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 10:09 PM Geert Uytterhoeven
<[email protected]> 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 <[email protected]>
One nit-pick below, though.
> Reported-by: Mikko Salomäki <[email protected]>
> Fixes: 828546e24280f721 ("gpio: Add GPIO Aggregator")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> 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
On Tue, Feb 1, 2022 at 10:54 PM Geert Uytterhoeven <[email protected]> wrote:
> On Tue, Feb 1, 2022 at 9:35 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven
> > <[email protected]> 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
On Tue, Feb 1, 2022 at 9:59 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Feb 1, 2022 at 10:54 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Tue, Feb 1, 2022 at 9:35 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven
> > > <[email protected]> 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