linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Olivier Moysan <olivier.moysan@st.com>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Lucas Stach <l.stach@pengutronix.de>,
	Holger Assmann <has@pengutronix.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Fabrice Gasnier <fabrice.gasnier@st.com>,
	kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org,
	Alexandre Torgue <alexandre.torgue@st.com>
Subject: Re: [PATCH] iio: adc: stm32-adc: fix erroneous handling of spurious IRQs
Date: Sat, 16 Jan 2021 17:53:33 +0000	[thread overview]
Message-ID: <20210116175333.4d8684c5@archlinux> (raw)
In-Reply-To: <20210112152441.20665-1-a.fatoum@pengutronix.de>

On Tue, 12 Jan 2021 16:24:42 +0100
Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

> 1c6c69525b40 ("genirq: Reject bogus threaded irq requests") makes sure
> that threaded IRQs either
>   - have IRQF_ONESHOT set
>   - don't have the default just return IRQ_WAKE_THREAD primary handler
> 
> This is necessary because level-triggered interrupts need to be masked,
> either at device or irqchip, to avoid an interrupt storm.
> 
> For spurious interrupts, the STM32 ADC driver still does this bogus
> request though:
>   - It doesn't set IRQF_ONESHOT
>   - Its primary handler just returns IRQ_WAKE_THREAD if the interrupt
>     is unexpected, i.e. !(status & enabled_mask)

This seems 'unusual'.  If this is a spurious interrupt we should be
returning IRQ_NONE and letting the spurious interrupt protection
stuff kick in.

The only reason I can see that it does this is print an error message.
I'm not sure why we need to go into the thread to do that given
it's not supposed to happen. If we need that message at all, I'd
suggest doing it in the interrupt handler then return IRQ_NONE;

>   - stm32mp151.dtsi describes the ADC interrupt as level-triggered
> 
> Fix this by setting IRQF_ONESHOT to have the irqchip mask the IRQ
> until the IRQ thread has finished.
> 
> IRQF_ONESHOT also has the effect that the primary handler is no longer
> forced into a thread. This makes the issue with spurious interrupts
> interrupts disappear when reading the ADC on a threadirqs=1 kernel.
> This used to result in following kernel error message:
> 
> 	iio iio:device1: Unexpected IRQ: IER=0x00000000, ISR=0x0000100e
> or
> 	iio iio:device1: Unexpected IRQ: IER=0x00000004, ISR=0x0000100a
> 
> But with this patch applied (or threaded IRQs disabled), this no longer
> occurs.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Reported-by: Holger Assmann <has@pengutronix.de>
> Fixes: 695e2f5c289b ("iio: adc: stm32-adc: fix a regression when using dma and irq")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/iio/adc/stm32-adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index c067c994dae2..7e0e21c79ac8 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -1910,7 +1910,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
>  					stm32_adc_threaded_isr,
> -					0, pdev->name, indio_dev);
> +					IRQF_ONESHOT, pdev->name, indio_dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>  		return ret;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-01-16 17:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-12 15:24 [PATCH] iio: adc: stm32-adc: fix erroneous handling of spurious IRQs Ahmad Fatoum
2021-01-16 17:53 ` Jonathan Cameron [this message]
2021-01-18 11:42   ` Ahmad Fatoum
2021-01-19 17:56     ` [Linux-stm32] " Fabrice Gasnier
2021-01-22 12:18       ` Ahmad Fatoum
2021-01-26 15:52         ` Fabrice Gasnier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210116175333.4d8684c5@archlinux \
    --to=jic23@kernel.org \
    --cc=a.fatoum@pengutronix.de \
    --cc=alexandre.torgue@st.com \
    --cc=fabrice.gasnier@st.com \
    --cc=has@pengutronix.de \
    --cc=kernel@pengutronix.de \
    --cc=l.stach@pengutronix.de \
    --cc=lars@metafoo.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=olivier.moysan@st.com \
    --cc=pmeerw@pmeerw.net \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).