[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f8894ec1-0af4-9566-5836-30b23bf40110@alliedtelesis.co.nz>
Date: Mon, 29 May 2023 04:08:13 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Johan Hovold <johan@...nel.org>,
Bartosz Golaszewski <brgl@...ev.pl>
CC: Andy Shevchenko <andy.shevchenko@...il.com>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"maz@...nel.org" <maz@...nel.org>,
"warthog618@...il.com" <warthog618@...il.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] gpiolib: Avoid side effects in gpio_is_visible()
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@...il.com> wrote:
>>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>>> <chris.packham@...iedtelesis.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.
Powered by blists - more mailing lists