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;
MIME-Version: 1.0
References: <TYCP286MB1188B6A0379F7928C63288DF8A21A@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM>
 <ZJk87rWsDj7pWJIP@sol> <CAMRc=McGOV+A8ROXRinUkWG0POTe37RWphRHDW6sy7UMU-e2UA@mail.gmail.com>
 <ZJo+wBMsyHvu/7Mj@sol>
In-Reply-To: <ZJo+wBMsyHvu/7Mj@sol>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Tue, 27 Jun 2023 13:47:16 +0200
Message-ID: <CAMRc=MeyEuZpDXsZHVGdrivZbYjHr1YDiYAbu4A0--Xy9rKVUw@mail.gmail.com>
Subject: Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue
To:     Kent Gibson <warthog618@gmail.com>
Cc:     YE Chengfeng <cyeaa@connect.ust.hk>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "andy@kernel.org" <andy@kernel.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jun 27, 2023 at 3:43=E2=80=AFAM Kent Gibson <warthog618@gmail.com> =
wrote:
>
> On Mon, Jun 26, 2023 at 05:50:47PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jun 26, 2023 at 9:23=E2=80=AFAM Kent Gibson <warthog618@gmail.c=
om> wrote:
> > >
> > >
> > > spin_lock_bh() should be sufficient, given that edge_irq_thread() is =
run
> > > in a softirq?  That is faster and would allow the hard irq handlers t=
o
> > > still run, and timestamp the event, but inhibit the edge_irq_thread()
> > > from being called on that CPU until the lock is released.
> > > (hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it i=
s
> > > never called from hard irq context, so there is a good chance I'm mis=
sing
> > > something here??)
> > > More on spin_lock choice below.
> >
> > Again: this is incorrect - edge_irq_thread() doesn't execute in
> > softirq context which can be verified by calling in_softirq() from it.
> >
>
> Ok, that matches what I had initially thought.  Wading through the kernel
> doc got me thinking the secondary handler was run as a softirq.
> But it is a threaded irq used here, so the thread handler runs in a
> kernel thread, as does the debounce_work_func() and hte thread handler
> process_hw_ts_thread().
> That's a relief.
>
> While we are on the subject of spin_locks, why does
> gpio_desc_to_lineinfo() use spin_lock_irqsave()?
> I assume the _irq is necessary as the desc could be updated at interrupt
> level, but AFAICT gpio_desc_to_lineinfo() is only ever called from proces=
s
> context, so why not just spin_lock_irq()?
>
> Cheers,
> Kent.

Didn't we use an atomic notifier before for some reason? Then it got
changed to blocking but the lock stayed like this? It does look like
spin_lock_irq() would be fine here. On the other hand - if something
isn't broken... :)

Bart