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

[Chris Packham]

On 29/05/23 09:21, Chris Packham wrote:
>
> 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
>>
Ironically I tried to revisit my fix but found I was no longer able to 
reproduce the issue. Turns out commit 7dd3d9bd873f ("gpiolib: fix 
allocation of mixed dynamic/static GPIOs") has fixed it for me but I 
don't entirely understand how.