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.
Signed-off-by: Chris Packham <[email protected]>
---
Notes:
Changes in v2:
- expand commit message to (hopefully) better describe the problem and
solution
- drop the inaccurate fixes tag
drivers/gpio/gpiolib-sysfs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 530dfd19d7b5..f859dcd1cbf3 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -362,8 +362,6 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
if (!show_direction)
mode = 0;
} else if (attr == &dev_attr_edge.attr) {
- if (gpiod_to_irq(desc) < 0)
- mode = 0;
if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
mode = 0;
}
--
2.40.1
On Fri, May 19, 2023 at 8:07 AM Chris Packham
<[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Fri, May 19, 2023 at 12:09 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, May 19, 2023 at 8:07 AM Chris Packham
> <[email protected]> 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.
Bart
On Fri, May 26, 2023 at 03:01:01PM +0200, Bartosz Golaszewski wrote:
> On Fri, May 19, 2023 at 12:09 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Fri, May 19, 2023 at 8:07 AM Chris Packham
> > <[email protected]> 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. 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).
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.
Fix the root cause, don't just paper over the symptom.
Johan
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
>> <[email protected]> wrote:
>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>> <[email protected]> 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
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
>>> <[email protected]> wrote:
>>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>>> <[email protected]> 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.