gpio irqchip initialization race

Commit Message

Linus Walleij April 1, 2016, 8:43 a.m. UTC

On Fri, Apr 1, 2016 at 10:28 AM, Alexander Stein
<[e-mail]> wrote:

> I suspect this depends on the time when gpio-keys is being probed. It will not
> be probed (or actually deferred) before mcp23s08_probe_one calls
> gpiochip_add_data as the parent GPIo controller to buttons is not present yet.
> I guess those error messages raise when gpio-keys is probed after
> gpiochip_add_data but before gpiochip_irqchip_add or
> gpiochip_set_chained_irqchip (in mcp23s08_irq_setup).
>
> The comment to _gpiochip_irqchip_add states the gpiochip has to be registered
> when calling. So to me this opens a rather small window where childs to
> gpiochip can be probed (parent is registered) but registering an interrupt
> will fail as the parent IRQ setup has not finished yet.
> Any suggestions how to fix that?

If that is what is happening it is indeed a very annoying race condition.
Strange we haven't seen it before!

So the problem doesn't exist when just using the gpiolib API or
the irqchip side independently, as we have been careful to make sure
that at least in modern drivers, these two are orthogonal.

I think the problem is that gpio-keys is calling gpio_to_irq(), and at that
point between gpiochip_add_data() but before gpiochip_irqchip_add()
it gets a bogus IRQ when it should be getting -EPROBE_DEFER.

I think we need to make sure that for drivers using gpiolib_irqchip_add()
-EPROBE_DEFER is returned for gpio[d]_to_irq() for this interrim
period.

What happens if you just apply this oneliner?


Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Walleij April 1, 2016, 8:47 a.m. UTC | #1

On Fri, Apr 1, 2016 at 10:43 AM, Linus Walleij <[e-mail]> wrote:

> I think the problem is that gpio-keys is calling gpio_to_irq(), and at that
> point between gpiochip_add_data() but before gpiochip_irqchip_add()
> it gets a bogus IRQ when it should be getting -EPROBE_DEFER.

Or not a bogus irqnumber really, it get -ENXIO (-6)
as in your example:

> gpio-keys user_sw: Unable to get irq number for GPIO 376, error -6

And in this case (if gpio_keys handle -EPROBE_DEFER gracefully)
all should be fine with my oneliner patch.

I am more uncertain about the -EINVAL (-22) we might need some
more analysis there.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexander Stein April 1, 2016, 8:56 a.m. UTC | #2

On Friday 01 April 2016 10:47:42, Linus Walleij wrote:
> On Fri, Apr 1, 2016 at 10:43 AM, Linus Walleij <[e-mail]> 
wrote:
> > I think the problem is that gpio-keys is calling gpio_to_irq(), and at
> > that
> > point between gpiochip_add_data() but before gpiochip_irqchip_add()
> > it gets a bogus IRQ when it should be getting -EPROBE_DEFER.
> 
> Or not a bogus irqnumber really, it get -ENXIO (-6)
> 
> as in your example:
> > gpio-keys user_sw: Unable to get irq number for GPIO 376, error -6
> 
> And in this case (if gpio_keys handle -EPROBE_DEFER gracefully)
> all should be fine with my oneliner patch.
> 
> I am more uncertain about the -EINVAL (-22) we might need some
> more analysis there.

I did 10 runs and got the following results:
> 3x  gpio-keys user_sw: Unable to claim irq 0; error -22
> 2x  gpio-keys user_sw: Unable to get irq number for GPIO 376, error -517
> 5x  ok

So, for one gpio-keys seems not to handle -EPROBE_DEFER gracefully and it seem 
still possible to get an invalid irq number.

Best regards,
Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij April 1, 2016, 11:29 a.m. UTC | #3

On Fri, Apr 1, 2016 at 10:56 AM, Alexander Stein
<[e-mail]> wrote:
> [Me]
>> And in this case (if gpio_keys handle -EPROBE_DEFER gracefully)
>> all should be fine with my oneliner patch.
>>
>> I am more uncertain about the -EINVAL (-22) we might need some
>> more analysis there.
>
> I did 10 runs and got the following results:
>> 3x  gpio-keys user_sw: Unable to claim irq 0; error -22
>> 2x  gpio-keys user_sw: Unable to get irq number for GPIO 376, error -517
>> 5x  ok
>
> So, for one gpio-keys seems not to handle -EPROBE_DEFER gracefully

After reading the code the only problem seems to be that it prints that
error. A small patch to silence that print should fix it then, can you
confirm that in these cases the gpio-keys are retried later?

> and it seem
> still possible to get an invalid irq number.

I think this is because gpiolib initialize .to_irq() before setting up the
irqdomain.

I'm making a combined patch fixing both issues, send out in a few sec.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexander Stein April 1, 2016, 12:45 p.m. UTC | #4

On Friday 01 April 2016 13:29:14, Linus Walleij wrote:
> On Fri, Apr 1, 2016 at 10:56 AM, Alexander Stein
> 
> <[e-mail]> wrote:
> > [Me]
> > 
> >> And in this case (if gpio_keys handle -EPROBE_DEFER gracefully)
> >> all should be fine with my oneliner patch.
> >> 
> >> I am more uncertain about the -EINVAL (-22) we might need some
> >> more analysis there.
> > 
> > I did 10 runs and got the following results:
> >> 3x  gpio-keys user_sw: Unable to claim irq 0; error -22
> >> 2x  gpio-keys user_sw: Unable to get irq number for GPIO 376, error -517
> >> 5x  ok
> > 
> > So, for one gpio-keys seems not to handle -EPROBE_DEFER gracefully
> 
> After reading the code the only problem seems to be that it prints that
> error. A small patch to silence that print should fix it then, can you
> confirm that in these cases the gpio-keys are retried later?

Ah, yes. You're right. I just checked for the error messages, but not if the 
device are actually absent. I just had a start where a (different) gpio-keys 
device failed with error -517 but is available when loggin in. So it really is 
just fix to silence on -DEFER_PROBE.

Best regards,
Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b747c76fd2b1..838643d2d976 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2004,7 +2004,7 @@  int gpiod_to_irq(const struct gpio_desc *desc)
        VALIDATE_DESC(desc);
        chip = desc->gdev->chip;
        offset = gpio_chip_hwgpio(desc);
-       return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO;
+       return chip->to_irq ? chip->to_irq(chip, offset) : -EPROBE_DEFER;
 }
 EXPORT_SYMBOL_GPL(gpiod_to_irq);