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

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

[Chris Packham]

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 <chris.packham@alliedtelesis.co.nz>
---

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

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

[Andy Shevchenko]

On Fri, May 19, 2023 at 8:07 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> 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

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

[Bartosz Golaszewski]

On Fri, May 19, 2023 at 12:09 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, May 19, 2023 at 8:07 AM Chris Packham
> <chris.packham@alliedtelesis.co.nz> 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

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

[Johan Hovold]

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@gmail.com> wrote:
> >
> > On Fri, May 19, 2023 at 8:07 AM Chris Packham
> > <chris.packham@alliedtelesis.co.nz> 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

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

[Chris Packham]

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@gmail.com> wrote:
>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>> <chris.packham@alliedtelesis.co.nz> 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

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@gmail.com> wrote:
>>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>>> <chris.packham@alliedtelesis.co.nz> 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.