linux-kernel - Re: [PATCH v2] 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]
Message-ID: <5c8f68f0157d9ae66e6d88d648fcfd26e68be307.camel@gmail.com>
Date:   Tue, 18 Apr 2023 13:08:54 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Masahiro Honda <honda@...hatrax.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

On Fri, 2023-04-14 at 19:27 +0900, Masahiro Honda 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 complete
> conversion. This patch provides an option to fix the issue by setting
> IRQ_DISABLE_UNLAZY flag.
> 
> Signed-off-by: Masahiro Honda <honda@...hatrax.com>
> ---
> v2:
>  - 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/Kconfig          | 14 ++++++++++++++
>  drivers/iio/adc/ad_sigma_delta.c | 31 ++++++++++++++++++++++++++-----
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 45af2302b..78ab6e2d8 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -21,6 +21,20 @@ config AD_SIGMA_DELTA
>         select IIO_BUFFER
>         select IIO_TRIGGERED_BUFFER
>  
> +if AD_SIGMA_DELTA
> +
> +config AD_SIGMA_DELTA_USE_LAZY_IRQ
> +       bool "Use lazy IRQ for sigma-delta ADCs"
> +       depends on AD_SIGMA_DELTA
> +       default n
> +       help
> +         Some interrupt controllers have data read problem with ADCs depends
> on
> +         AD_SIGMA_DELTA.
> +         Say yes here to avoid the problem at the cost of performance
> overhead.
> +         If unsure, say N (but it's safe to say "Y").
> +
> +endif # if AD_SIGMA_DELTA
> +
>  config AD4130
>         tristate "Analog Device AD4130 ADC Driver"
>         depends on SPI
> diff --git a/drivers/iio/adc/ad_sigma_delta.c
> b/drivers/iio/adc/ad_sigma_delta.c
> index d8570f620..b9eae1e80 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -565,6 +565,16 @@ 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;
> +
> +#ifdef CONFIG_AD_SIGMA_DELTA_USE_LAZY_IRQ
> +       irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> +#endif
> +       free_irq(sigma_delta->spi->irq, sigma_delta);
> +}
> +
>  static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev
> *indio_dev)
>  {
>         struct ad_sigma_delta *sigma_delta =
> iio_device_get_drvdata(indio_dev);
> @@ -584,11 +594,22 @@ static int devm_ad_sd_probe_trigger(struct device *dev,
> struct iio_dev *indio_de
>         init_completion(&sigma_delta->completion);
>  
>         sigma_delta->irq_dis = true;
> -       ret = devm_request_irq(dev, sigma_delta->spi->irq,
> -                              ad_sd_data_rdy_trig_poll,
> -                              sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> -                              indio_dev->name,
> -                              sigma_delta);
> +#ifdef CONFIG_AD_SIGMA_DELTA_USE_LAZY_IRQ
> +       irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> +#endif
> +       ret = request_irq(sigma_delta->spi->irq,
> +                         ad_sd_data_rdy_trig_poll,
> +                         sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> +                         indio_dev->name,
> +                         sigma_delta);
> +       if (ret) {
> +#ifdef CONFIG_AD_SIGMA_DELTA_USE_LAZY_IRQ
> +               irq_clear_status_flags(sigma_delta->spi->irq,
> IRQ_DISABLE_UNLAZY);

I'm not really keen with having the Kconfig option. I'm fairly convinced that
this might be a problem affecting all sigma delta ADCs and even if they aren't
affected, I do not think that setting this IRQ flag will do any arm (other than
less efficiency maybe).


If you really want to be on the safe side, we could add a new boolean to 'struct
ad_sigma_delta_info' that would be enabled for your device and when true, the
LAZY bit is set. Once we prove that all devices might be affected by this issue,
we could remove the boolean and make it a default setting.

- Nuno Sá