linux-kernel - Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Apr 2023 11:09:34 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Masahiro Honda <honda@...hatrax.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

Hi Jonathan,

On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote:
> On Thu, 20 Apr 2023 19:23:16 +0900
> Masahiro Honda <honda@...hatrax.com> wrote:
> 
> > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> > line to indicate the completion of a conversion. However, some devices
> > cannot properly detect the completion of a conversion by an interrupt.
> > This is for the reason mentioned in the following commit.
> > 
> > commit e9849777d0e2 ("genirq: Add flag to force mask in
> >                       disable_irq[_nosync]()")
> > 
> > A read operation is performed by an extra interrupt before the completion
> > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> > flag.
> > 
> > Signed-off-by: Masahiro Honda <honda@...hatrax.com>
> > ---
> > v3:
> >  - Remove the Kconfig option.
> > v2:
> > https://lore.kernel.org/linux-iio/20230414102744.150-1-honda@mechatrax.com/
> >  - Rework commit message.
> >  - Add a new entry in the Kconfig.
> >  - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > IRQ.
> > v1:
> > https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/
> > 
> >  drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > b/drivers/iio/adc/ad_sigma_delta.c
> > index d8570f620..215ecbedb 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev,
> > struct iio_trigger *trig)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
> >  
> > +static void ad_sd_free_irq(void *sd)
> > +{
> > +       struct ad_sigma_delta *sigma_delta = sd;
> > +
> > +       irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> > +       free_irq(sigma_delta->spi->irq, sigma_delta);
> > +}
> 
> Don't fuse the two operations unwinding like this.  Just register a callback
> that only
> does the irq_clear_status_flags immediately after setting them.  Then leave

I was the one to propose fusing them together because I thought that we could
have issues by clearing the flag after calling free_irq(). After looking again
at the IRQ code, I can see that it is not up to free_irq() to free the allocated
irq_descs (that might only happen when unmapping the virq) which means we should
be fine doing the normal way.

That said, looking at the only users that care to clear this flag, it looks like
they do it before calling free_irq(). Hence, I'm not sure if there's anything
subtle going on. In fact, looking at this line:

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909

I'm not so sure we actually need to clear the flag as for these devices, we
should only have one consumer/action per IRQ. Anyways, probably for correctness
we should still explicitly clear it?

- Nuno Sá