Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] iio: adc: stm32-adc: Add check on overrun interrupt
To:     Jonathan Cameron <jic23@kernel.org>
CC:     <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <mcoquelin.stm32@gmail.com>,
        <alexandre.torgue@st.com>, <linux-iio@vger.kernel.org>,
        <lars@metafoo.de>, <knaack.h@gmx.de>, <pmeerw@pmeerw.net>,
        <linux-stm32@st-md-mailman.stormreply.com>
References: <1574435294-19510-1-git-send-email-fabrice.gasnier@st.com>
 <20191123113623.3e58b2d1@archlinux>
From:   Fabrice Gasnier <fabrice.gasnier@st.com>
Message-ID: <999ccf66-1184-7b9e-309c-8130ad643c82@st.com>
Date:   Mon, 25 Nov 2019 09:42:01 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191123113623.3e58b2d1@archlinux>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/23/19 12:36 PM, Jonathan Cameron wrote:
> On Fri, 22 Nov 2019 16:08:14 +0100
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 
>> Enable overrun interrupt on STM32 ADC. In case data register hasn't been
>> read (by CPU or DMA), overrun condition is detected when there's new
>> conversion data available. Stop grabbing data and log an error message.
>> Use a threaded irq to avoid printing the error message from hard irq
>> context.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> 
> Absolutely makes sense to notify the user this occurred and to stop
> the current process.  It may be useful to give a hint on how
> to restart capture?

Hi Jonathan,

Restarting the capture can simply be done by disabling/re-enabling, e.g.
from userland:
cd /sys/bus/iio/devices/iio:deviceX
echo 0 > buffer/enable
echo 1 > buffer/enable

Do you think such a hint should be added somewhere? (in the commit
message or in a comment...) ?

> 
> Is this a condition that can occur under 'normal conditions' such
> as something else grabbing the CPU for too long?

Indeed, this may happen for instance when DMA isn't used (e.g. no DMA
channel as been assigned to the ADC, so using CPU/interrupts), and CPU
is busy for too long... I don't expect this may happen otherwise.

> 
> I'd just like to understand if we are dealing with a 'fault'
> case of just one where things go wrong under weird conditions.

Of course something weird, wrong, faulty... happening on the DMA side
for example, may cause overrun. But it's quite unlikely.

> 
> Patch itself looks fine.

Please let me know if I need to improve description.

Thanks for reviewing,
Fabrice

> 
> Thanks,
> 
> Jonathan
> 
> 
>> ---
>>  drivers/iio/adc/stm32-adc-core.c | 14 +++++------
>>  drivers/iio/adc/stm32-adc-core.h |  9 +++++++
>>  drivers/iio/adc/stm32-adc.c      | 53 ++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 67 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
>> index 6537f4f..97655d7 100644
>> --- a/drivers/iio/adc/stm32-adc-core.c
>> +++ b/drivers/iio/adc/stm32-adc-core.c
>> @@ -280,21 +280,21 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev,
>>  static const struct stm32_adc_common_regs stm32f4_adc_common_regs = {
>>  	.csr = STM32F4_ADC_CSR,
>>  	.ccr = STM32F4_ADC_CCR,
>> -	.eoc1_msk = STM32F4_EOC1,
>> -	.eoc2_msk = STM32F4_EOC2,
>> -	.eoc3_msk = STM32F4_EOC3,
>> +	.eoc1_msk = STM32F4_EOC1 | STM32F4_OVR1,
>> +	.eoc2_msk = STM32F4_EOC2 | STM32F4_OVR2,
>> +	.eoc3_msk = STM32F4_EOC3 | STM32F4_OVR3,
>>  	.ier = STM32F4_ADC_CR1,
>> -	.eocie_msk = STM32F4_EOCIE,
>> +	.eocie_msk = STM32F4_EOCIE | STM32F4_OVRIE,
>>  };
>>  
>>  /* STM32H7 common registers definitions */
>>  static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
>>  	.csr = STM32H7_ADC_CSR,
>>  	.ccr = STM32H7_ADC_CCR,
>> -	.eoc1_msk = STM32H7_EOC_MST,
>> -	.eoc2_msk = STM32H7_EOC_SLV,
>> +	.eoc1_msk = STM32H7_EOC_MST | STM32H7_OVR_MST,
>> +	.eoc2_msk = STM32H7_EOC_SLV | STM32H7_OVR_SLV,
>>  	.ier = STM32H7_ADC_IER,
>> -	.eocie_msk = STM32H7_EOCIE,
>> +	.eocie_msk = STM32H7_EOCIE | STM32H7_OVRIE,
>>  };
>>  
>>  static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = {
>> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
>> index 2579d51..2322809 100644
>> --- a/drivers/iio/adc/stm32-adc-core.h
>> +++ b/drivers/iio/adc/stm32-adc-core.h
>> @@ -51,10 +51,12 @@
>>  #define STM32F4_ADC_CCR			(STM32_ADCX_COMN_OFFSET + 0x04)
>>  
>>  /* STM32F4_ADC_SR - bit fields */
>> +#define STM32F4_OVR			BIT(5)
>>  #define STM32F4_STRT			BIT(4)
>>  #define STM32F4_EOC			BIT(1)
>>  
>>  /* STM32F4_ADC_CR1 - bit fields */
>> +#define STM32F4_OVRIE			BIT(26)
>>  #define STM32F4_RES_SHIFT		24
>>  #define STM32F4_RES_MASK		GENMASK(25, 24)
>>  #define STM32F4_SCAN			BIT(8)
>> @@ -72,8 +74,11 @@
>>  #define STM32F4_ADON			BIT(0)
>>  
>>  /* STM32F4_ADC_CSR - bit fields */
>> +#define STM32F4_OVR3			BIT(21)
>>  #define STM32F4_EOC3			BIT(17)
>> +#define STM32F4_OVR2			BIT(13)
>>  #define STM32F4_EOC2			BIT(9)
>> +#define STM32F4_OVR1			BIT(5)
>>  #define STM32F4_EOC1			BIT(1)
>>  
>>  /* STM32F4_ADC_CCR - bit fields */
>> @@ -103,10 +108,12 @@
>>  
>>  /* STM32H7_ADC_ISR - bit fields */
>>  #define STM32MP1_VREGREADY		BIT(12)
>> +#define STM32H7_OVR			BIT(4)
>>  #define STM32H7_EOC			BIT(2)
>>  #define STM32H7_ADRDY			BIT(0)
>>  
>>  /* STM32H7_ADC_IER - bit fields */
>> +#define STM32H7_OVRIE			STM32H7_OVR
>>  #define STM32H7_EOCIE			STM32H7_EOC
>>  
>>  /* STM32H7_ADC_CR - bit fields */
>> @@ -155,7 +162,9 @@ enum stm32h7_adc_dmngt {
>>  #define STM32H7_LINCALFACT_MASK		GENMASK(29, 0)
>>  
>>  /* STM32H7_ADC_CSR - bit fields */
>> +#define STM32H7_OVR_SLV			BIT(20)
>>  #define STM32H7_EOC_SLV			BIT(18)
>> +#define STM32H7_OVR_MST			BIT(4)
>>  #define STM32H7_EOC_MST			BIT(2)
>>  
>>  /* STM32H7_ADC_CCR - bit fields */
>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>> index 3b291d7..9361f92 100644
>> --- a/drivers/iio/adc/stm32-adc.c
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -117,7 +117,9 @@ struct stm32_adc_regs {
>>   * struct stm32_adc_regspec - stm32 registers definition
>>   * @dr:			data register offset
>>   * @ier_eoc:		interrupt enable register & eocie bitfield
>> + * @ier_ovr:		interrupt enable register & overrun bitfield
>>   * @isr_eoc:		interrupt status register & eoc bitfield
>> + * @isr_ovr:		interrupt status register & overrun bitfield
>>   * @sqr:		reference to sequence registers array
>>   * @exten:		trigger control register & bitfield
>>   * @extsel:		trigger selection register & bitfield
>> @@ -128,7 +130,9 @@ struct stm32_adc_regs {
>>  struct stm32_adc_regspec {
>>  	const u32 dr;
>>  	const struct stm32_adc_regs ier_eoc;
>> +	const struct stm32_adc_regs ier_ovr;
>>  	const struct stm32_adc_regs isr_eoc;
>> +	const struct stm32_adc_regs isr_ovr;
>>  	const struct stm32_adc_regs *sqr;
>>  	const struct stm32_adc_regs exten;
>>  	const struct stm32_adc_regs extsel;
>> @@ -337,7 +341,9 @@ static const unsigned int stm32f4_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = {
>>  static const struct stm32_adc_regspec stm32f4_adc_regspec = {
>>  	.dr = STM32F4_ADC_DR,
>>  	.ier_eoc = { STM32F4_ADC_CR1, STM32F4_EOCIE },
>> +	.ier_ovr = { STM32F4_ADC_CR1, STM32F4_OVRIE },
>>  	.isr_eoc = { STM32F4_ADC_SR, STM32F4_EOC },
>> +	.isr_ovr = { STM32F4_ADC_SR, STM32F4_OVR },
>>  	.sqr = stm32f4_sq,
>>  	.exten = { STM32F4_ADC_CR2, STM32F4_EXTEN_MASK, STM32F4_EXTEN_SHIFT },
>>  	.extsel = { STM32F4_ADC_CR2, STM32F4_EXTSEL_MASK,
>> @@ -429,7 +435,9 @@ static const unsigned int stm32h7_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = {
>>  static const struct stm32_adc_regspec stm32h7_adc_regspec = {
>>  	.dr = STM32H7_ADC_DR,
>>  	.ier_eoc = { STM32H7_ADC_IER, STM32H7_EOCIE },
>> +	.ier_ovr = { STM32H7_ADC_IER, STM32H7_OVRIE },
>>  	.isr_eoc = { STM32H7_ADC_ISR, STM32H7_EOC },
>> +	.isr_ovr = { STM32H7_ADC_ISR, STM32H7_OVR },
>>  	.sqr = stm32h7_sq,
>>  	.exten = { STM32H7_ADC_CFGR, STM32H7_EXTEN_MASK, STM32H7_EXTEN_SHIFT },
>>  	.extsel = { STM32H7_ADC_CFGR, STM32H7_EXTSEL_MASK,
>> @@ -506,6 +514,18 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>>  			   adc->cfg->regs->ier_eoc.mask);
>>  }
>>  
>> +static void stm32_adc_ovr_irq_enable(struct stm32_adc *adc)
>> +{
>> +	stm32_adc_set_bits(adc, adc->cfg->regs->ier_ovr.reg,
>> +			   adc->cfg->regs->ier_ovr.mask);
>> +}
>> +
>> +static void stm32_adc_ovr_irq_disable(struct stm32_adc *adc)
>> +{
>> +	stm32_adc_clr_bits(adc, adc->cfg->regs->ier_ovr.reg,
>> +			   adc->cfg->regs->ier_ovr.mask);
>> +}
>> +
>>  static void stm32_adc_set_res(struct stm32_adc *adc)
>>  {
>>  	const struct stm32_adc_regs *res = &adc->cfg->regs->res;
>> @@ -1205,6 +1225,19 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
>>  	}
>>  }
>>  
>> +static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
>> +{
>> +	struct stm32_adc *adc = data;
>> +	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
>> +	const struct stm32_adc_regspec *regs = adc->cfg->regs;
>> +	u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
>> +
>> +	if (status & regs->isr_ovr.mask)
>> +		dev_err(&indio_dev->dev, "Overrun interrupt, stopping.\n");
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static irqreturn_t stm32_adc_isr(int irq, void *data)
>>  {
>>  	struct stm32_adc *adc = data;
>> @@ -1212,6 +1245,17 @@ static irqreturn_t stm32_adc_isr(int irq, void *data)
>>  	const struct stm32_adc_regspec *regs = adc->cfg->regs;
>>  	u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
>>  
>> +	if (status & regs->isr_ovr.mask) {
>> +		/*
>> +		 * Overrun occurred on regular conversions: data for wrong
>> +		 * channel may be read. Unconditionally disable interrupts
>> +		 * to stop processing data and print error message.
>> +		 */
>> +		stm32_adc_ovr_irq_disable(adc);
>> +		stm32_adc_conv_irq_disable(adc);
>> +		return IRQ_WAKE_THREAD;
>> +	}
>> +
>>  	if (status & regs->isr_eoc.mask) {
>>  		/* Reading DR also clears EOC status flag */
>>  		adc->buffer[adc->bufi] = stm32_adc_readw(adc, regs->dr);
>> @@ -1441,6 +1485,8 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>>  	/* Reset adc buffer index */
>>  	adc->bufi = 0;
>>  
>> +	stm32_adc_ovr_irq_enable(adc);
>> +
>>  	if (!adc->dma_chan)
>>  		stm32_adc_conv_irq_enable(adc);
>>  
>> @@ -1481,6 +1527,8 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
>>  	if (!adc->dma_chan)
>>  		stm32_adc_conv_irq_disable(adc);
>>  
>> +	stm32_adc_ovr_irq_disable(adc);
>> +
>>  	if (adc->dma_chan)
>>  		dmaengine_terminate_sync(adc->dma_chan);
>>  
>> @@ -1818,8 +1866,9 @@ static int stm32_adc_probe(struct platform_device *pdev)
>>  	if (adc->irq < 0)
>>  		return adc->irq;
>>  
>> -	ret = devm_request_irq(&pdev->dev, adc->irq, stm32_adc_isr,
>> -			       0, pdev->name, adc);
>> +	ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
>> +					stm32_adc_threaded_isr,
>> +					0, pdev->name, adc);
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>>  		return ret;
>