[PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Masahiro Honda]

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@mechatrax.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);
+}
+
 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 +592,18 @@ 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);
+	irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
+	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) {
+		irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, ad_sd_free_irq, sigma_delta);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Nuno Sá]

On Thu, 2023-04-20 at 19:23 +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 completion
> of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> flag.
> 
> Signed-off-by: Masahiro Honda <honda@mechatrax.com>
> ---

LGTM:

Reviewed-by: Nuno Sá <nuno.sa@analog.com

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Jonathan Cameron]

On Thu, 20 Apr 2023 19:23:16 +0900
Masahiro Honda <honda@mechatrax.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@mechatrax.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
the orginally devm_request_irq call alone.  If it fails, the devm cleanup will
deal with the irq_clear_status_flag for you.

It almost never makes sense for a single devm call to unwind more than one function call in
a driver.

Otherwise this looks fine to me,

Thanks,

Jonathan



> +
>  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 +592,18 @@ 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);
> +	irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> +	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) {
> +		irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ad_sd_free_irq, sigma_delta);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Nuno Sá]

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@mechatrax.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@mechatrax.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á

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Masahiro Honda]

Hi Jonathan,

On Sun, Apr 23, 2023 at 7:59 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Apr 2023 19:23:16 +0900
> Masahiro Honda <honda@mechatrax.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@mechatrax.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
> the orginally devm_request_irq call alone.  If it fails, the devm cleanup will
> deal with the irq_clear_status_flag for you.
>
> It almost never makes sense for a single devm call to unwind more than one function call in
> a driver.
>
> Otherwise this looks fine to me,
>
> Thanks,
>
> Jonathan
>

I understand. I'll fix it.

Thanks,

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Masahiro Honda]

Hi Jonathan,

On Wed, Apr 26, 2023 at 9:02 PM Masahiro Honda <honda@mechatrax.com> wrote:
>
> I understand. I'll fix it.
>

Sorry, I made a mistake.
I would like to wait for a conclusion.

Regards,

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Jonathan Cameron]

On Mon, 24 Apr 2023 11:09:34 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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@mechatrax.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@mechatrax.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.

Ah. I'd missed the ordering.  If that had been valid (and I think you are correct
that it is not required) then a comment to make that clear would be necessary.

Usual case of: When doing something non obvious with ordering, say why.

> 
> 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?

Good question...  Looks to me like a driver shouldn't be clearing this flag
itself, but it's probably harmless in most cases.

I'd drop the clear of the status flag, perhaps adding a comment that
the irq core does it for us.

Jonathan


> 
> - Nuno Sá
> 

Re: [PATCH v3] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

[Masahiro Honda]

Hi all,

On Mon, May 1, 2023 at 2:27 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 24 Apr 2023 11:09:34 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > 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@mechatrax.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@mechatrax.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.
>
> Ah. I'd missed the ordering.  If that had been valid (and I think you are correct
> that it is not required) then a comment to make that clear would be necessary.
>
> Usual case of: When doing something non obvious with ordering, say why.
>
> >
> > 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?
>
> Good question...  Looks to me like a driver shouldn't be clearing this flag
> itself, but it's probably harmless in most cases.
>
> I'd drop the clear of the status flag, perhaps adding a comment that
> the irq core does it for us.
>
> Jonathan
>
>
> >
> > - Nuno Sá
> >
>

I'll remove the callback and just call irq_set_status_flags() with a comment.

Thanks,

Masahiro