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

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On 27/05/23 01:23, Johan Hovold wrote:
> On Fri, May 26, 2023 at 03:01:01PM +0200, Bartosz Golaszewski wrote:
>> On Fri, May 19, 2023 at 12:09 PM Andy Shevchenko
>> <andy.shevchenko@xxxxxxxxx> wrote:
>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>> <chris.packham@xxxxxxxxxxxxxxxxxxx> wrote:
>>>> On a system with pca9555 GPIOs that have been exported via sysfs the
>>>> following warning could be triggered on kexec().
>>>>
>>>>    WARNING: CPU: 0 PID: 265 at drivers/gpio/gpiolib.c:3411 gpiochip_disable_irq
>>>>    Call trace:
>>>>     gpiochip_disable_irq
>>>>     machine_crash_shutdown
>>>>     __crash_kexec
>>>>     panic
>>>>     sysrq_reset_seq_param_set
>>>>     __handle_sysrq
>>>>     write_sysrq_trigger
>>>>
>>>> The warning is triggered because there is an irq_desc for the GPIO but
>>>> it does not have the FLAG_USED_AS_IRQ set. This is because when the GPIO
>>>> is exported via gpiod_export(), gpio_is_visible() is used to determine
>>>> if the "edge" attribute should be provided but in doing so it ends up
>>>> calling gpiochip_to_irq() which creates the irq_desc.
>>>>
>>>> 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.
>>> To me it still sounds like a hack and the real solution should be done
>>> differently/elsewhere.
>>>
>>> Also I'm worrying that not having this file visible or not may affect
>>> existing user space custom scripts we will never hear about.
>>>
>>> P.S. TBH, I don't care much about sysfs, so if this patch finds its
>>> way upstream, I won't be unhappy.
>>>
>> Same. Which is why - if there'll be no more objections, I will apply it.
> I don't think this should be applied.
>
> It's still not clear from the commit message why gpiochip_disable_irq()
> is called for a line which has not been requested.

The code that does the calling is in machine_kexec_mask_interrupts(). 
The problem is that for some irq_chips irq_mask is set to the disable 
function. The disable call immediately after the mask call does check to 
see if the irq is not already disabled.

>   That seems like what
> should be fixed, not changing some behaviour in the gpio sysfs interface
> which has been there since forever (e.g. do not create the edge
> attributes for gpios that cannot be used as interrupts).

I don't disagree with the sentiment. The problem is there doesn't appear 
to be an API that can tell if a GPIO pin is capable of being an irq 
without actually converting it into one.

> There are other ways that mappings can be created (e.g. a gpio that
> requested as as interrupt and then released) which would trigger the
> same warning it seems.
I've tried a few of those cases and haven't been able to provoke the 
same warning. gpio_sysfs_free_irq() seems to clear whatever flags 
gpiochip_disable_irq() is complaining about.
> Fix the root cause, don't just paper over the symptom.
I think maybe there is a compromise where I do something in 
gpiochip_to_irq() instead of gpio_is_visible(). I'm not entirely sure 
what that something is
>
> Johan