Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()

[Chris Packham]

On 12/05/23 19:56, Linus Walleij wrote:
> On Fri, May 12, 2023 at 6:28 AM Chris Packham
> <chris.packham@xxxxxxxxxxxxxxxxxxx> wrote:
>
>> Calling gpiod_to_irq() creates an irq_desc for the GPIO.
> Normally gpiod_to_irq() should not have side effects, it's just
> a helper function that is there to translate a descriptor to the
> corresponding IRQ number.
>
>> This is not
>> something that should happen when just exporting the GPIO via sysfs. The
>> effect of this was observed as triggering a warning in
>> gpiochip_disable_irq() when kexec()ing after exporting a GPIO.

For clarity this is the problem I see on kexec

WARNING: CPU: 1 PID: 235 at drivers/gpio/gpiolib.c:3440 
gpiochip_disable_irq+0x64/0x6c
Call trace:
  gpiochip_disable_irq+0x64/0x6c
  machine_crash_shutdown+0xac/0x114
  __crash_kexec+0x74/0x154
  panic+0x300/0x370
  sysrq_reset_seq_param_set+0x0/0x8c
  __handle_sysrq+0xb8/0x1b8
  write_sysrq_trigger+0x74/0xa0
  proc_reg_write+0x9c/0xf0
  vfs_write+0xc4/0x298
  ksys_write+0x68/0xf4
  __arm64_sys_write+0x1c/0x28
  invoke_syscall+0x48/0x114
  el0_svc_common.constprop.0+0x44/0xf4
  do_el0_svc+0x3c/0xa8
  el0_svc+0x2c/0x84
  el0t_64_sync_handler+0xbc/0x138
  el0t_64_sync+0x190/0x194

>> Remove the call to gpiod_to_irq() from gpio_is_visible(). The actual
>> intended creation of the irq_desc comes via edge_store() when requested
>> by the user.
>>
>> Fixes: ebbeba120ab2 ("gpio: sysfs: fix gpio attribute-creation race")
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> I have a hard time understanding this fix.

The problem (as I far as I've been able to determine). Is that 
gpio_is_visible() uses gpiod_to_irq() to decide whether to provide the 
"edge" attribute. The problem is that instead of just looking up the 
irq_desc from the GPIO pin gpiod_to_irq() actually creates the irq_desc 
(via  gpiochip_to_irq()).

Because gpio_sysfs_request_irq() calls gpiochip_to_irq() anyway to 
create the irq_desc I thought removing gpiochip_to_irq() from 
gpio_is_visible() should be safe.

>
> The problem is rather this see gpiolib.c:
>
> int gpiod_to_irq(const struct gpio_desc *desc)
> {
>          struct gpio_chip *gc;
>          int offset;
>
>          /*
>           * Cannot VALIDATE_DESC() here as gpiod_to_irq() consumer semantics
>           * requires this function to not return zero on an invalid descriptor
>           * but rather a negative error number.
>           */
>          if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
>                  return -EINVAL;
>
>          gc = desc->gdev->chip;
>          offset = gpio_chip_hwgpio(desc);
>          if (gc->to_irq) {
>                  int retirq = gc->to_irq(gc, offset);
>
> Here gc->to_irq() is called unconditionally.
>
> Since this is using gpiolib_irqchip this ->to_irq() will be
> gpiochip_to_irq() and that finally ends in the call:
>
> return irq_create_mapping(domain, offset);
>
> which seems to be the problem here. Why is this a problem?
> The IRQ mappings are dynamic, meaning they are created
> on-demand, but for this hardware it should be fine to essentially
> just call irq_create_mapping() on all of them as the device
> is created, we just don't do it in order to save space.

In my original case which is a kernel module that exports a GPIO for 
userspace using gpiod_export() it's inappropriate because the GPIO in 
question is configured as an output but gpio_is_visible() still causes 
an irq_desc to be created even though the very next line of code knows 
that it should not make the edge attribute visible.

The manually exporting things via sysfs case is a bit more murky because 
it's not known if the GPIO is an input or output so the code must assume 
both are possible (obviously this is one thing libgpio fixes). I still 
think creating the irq_desc on export is wrong, it seems unnecessary as 
it'll happen if an interrupt is actually requested via edge_store().

> I don't understand why calling irq_create_mapping(domain, offset);
> creates a problem? It's just a mapping between a GPIO and the
> corresponding IRQ. What am I missing here?

The crux of the problem is that the irq_desc is created when it hasn't 
been requested. In some cases we know the GPIO pin is an output so we 
could avoid it, in others we could delay the creation until an interrupt 
is actually requested (which is what I'm attempting to do).

>
> Yours,
> Linus Walleij