| Message ID | 20230519050702.3681791-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpiolib: Avoid side effects in gpio_is_visible() | expand |
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.
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
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
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
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.
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; }
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(-)