Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 May 2023 15:23:32 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Bartosz Golaszewski <brgl@bgdev.pl>
Cc:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Chris Packham <chris.packham@alliedtelesis.co.nz>,
        linus.walleij@linaro.org, maz@kernel.org, warthog618@gmail.com,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] gpiolib: Avoid side effects in gpio_is_visible()
Message-ID: <ZHCy1PhyNAOCsalJ@hovoldconsulting.com>
References: <20230519050702.3681791-1-chris.packham@alliedtelesis.co.nz>
 <CAHp75Vcd8Q+-XMyfg3Y_hv_AL00PGgqg0jo7Yd7TTC4GrxPOuQ@mail.gmail.com>
 <CAMRc=MdHMiqhcpd2rFwjfKvwMWtTeTxG4fK+7zbzgSq9MHmGew@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRc=MdHMiqhcpd2rFwjfKvwMWtTeTxG4fK+7zbzgSq9MHmGew@mail.gmail.com>
Precedence: bulk

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