Re: [PATCH] iio: adc: stm32: fix sleep inside atomic section when using DMA

[Mukesh Ojha]

On 3/27/2019 6:21 PM, Mukesh Ojha wrote:
> On 3/26/2019 6:14 PM, Fabrice Gasnier wrote:
> > Enabling CONFIG_DEBUG_ATOMIC_SLEEP=y triggers this BUG message:
> > BUG: sleeping function called from invalid context at 
> > kernel/irq/chip.c...
> >
> > Call stack is as follows:
> > - __might_sleep
> > - handle_nested_irqÂÂÂÂÂÂÂÂÂ <-- Expects threaded irq
> > - iio_trigger_poll_chained
> > - stm32_adc_dma_buffer_done
> > - vchan_complete
> > - tasklet_action_common
> > - tasklet_action
> > - __do_softirqÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <-- DMA completion raises a tasklet
> > - irq_exit
> > - __handle_domain_irqÂÂÂÂÂÂÂ <-- DMA IRQ
> > - gic_handle_irq
> >
> > IIO expects threaded interrupt context when calling:
> > - iio_trigger_poll_chained()
> > Or it expects interrupt context when calling:
> > - iio_trigger_poll()
> >
> > This patch triggers an IRQ upon stm32_adc_dma_buffer_done() DMA callback
> > call, so the IIO trigger poll API gets called from IRQ context.
> >
> > Fixes: 2763ea0585c9 ("iio: adc: stm32: add optional dma support")
> >
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
> Reviewed-by: Mukesh Ojha <mojha@xxxxxxxxxxxxxx>
>
> -Mukesh
>
>
> > ---
> > Â drivers/iio/adc/KconfigÂÂÂÂ |Â 1 +
> > Â drivers/iio/adc/stm32-adc.c | 32 ++++++++++++++++++++++++++++++--
> > Â 2 files changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 76db6e5..059407a 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -775,6 +775,7 @@ config STM32_ADC_CORE
> > ÂÂÂÂÂ select MFD_STM32_TIMERS
> > ÂÂÂÂÂ select IIO_STM32_TIMER_TRIGGER
> > ÂÂÂÂÂ select IIO_TRIGGERED_BUFFER
> > +ÂÂÂ select IRQ_WORK
> > ÂÂÂÂÂ help
> > ÂÂÂÂÂÂÂ Select this option to enable the core driver for 
> > STMicroelectronics
> > ÂÂÂÂÂÂÂ STM32 analog-to-digital converter (ADC).
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 205e169..1aa3189 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -20,6 +20,7 @@
> > Â #include <linux/interrupt.h>
> > Â #include <linux/io.h>
> > Â #include <linux/iopoll.h>
> > +#include <linux/irq_work.h>
> > Â #include <linux/module.h>
> > Â #include <linux/platform_device.h>
> > Â #include <linux/pm_runtime.h>
> > @@ -297,6 +298,7 @@ struct stm32_adc_cfg {
> > ÂÂ * @smpr_val:ÂÂÂÂÂÂÂ sampling time settings (e.g. smpr1 / smpr2)
> > ÂÂ * @cal:ÂÂÂÂÂÂÂ optional calibration data on some devices
> > ÂÂ * @chan_name:ÂÂÂÂÂÂÂ channel name array
> > + * @work:ÂÂÂÂÂÂÂ irq work used to call trigger poll routine
> > ÂÂ */
> > Â struct stm32_adc {
> > ÂÂÂÂÂ struct stm32_adc_commonÂÂÂ *common;
> > @@ -320,6 +322,7 @@ struct stm32_adc {
> > ÂÂÂÂÂ u32ÂÂÂÂÂÂÂÂÂÂÂ smpr_val[2];
> > ÂÂÂÂÂ struct stm32_adc_calibÂÂÂ cal;
> > ÂÂÂÂÂ char chan_name[STM32_ADC_CH_MAX][STM32_ADC_CH_SZ];
> > +ÂÂÂ struct irq_workÂÂÂÂÂÂÂ work;
> > Â };
> > Â Â struct stm32_adc_diff_channel {
> > @@ -1473,11 +1476,32 @@ static unsigned int 
> > stm32_adc_dma_residue(struct stm32_adc *adc)
> > ÂÂÂÂÂ return 0;
> > Â }
> > Â +static void stm32_adc_dma_irq_work(struct irq_work *work)
> > +{
> > +ÂÂÂ struct stm32_adc *adc = container_of(work, struct stm32_adc, work);
> > +ÂÂÂ struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > +
> > +ÂÂÂ /*
> > +ÂÂÂÂ * iio_trigger_poll calls generic_handle_irq(). So, it requires 
> > hard
> > +ÂÂÂÂ * irq context, and cannot be called directly from dma callback,
> > +ÂÂÂÂ * dma cb has to schedule this work instead.
> > +ÂÂÂÂ */
> > +ÂÂÂ iio_trigger_poll(indio_dev->trig);
> > +}
> > +
> > Â static void stm32_adc_dma_buffer_done(void *data)
> > Â {
> > ÂÂÂÂÂ struct iio_dev *indio_dev = data;
> > +ÂÂÂ struct stm32_adc *adc = iio_priv(indio_dev);
> > Â -ÂÂÂ iio_trigger_poll_chained(indio_dev->trig);
> > +ÂÂÂ /*
> > +ÂÂÂÂ * Invoques iio_trigger_poll() from hard irq context: We can't


s/Invoques/invoke


overlooked this last time.
Take mine Review tag if you make above change.
Reviewed-by: Mukesh Ojha <mojha@xxxxxxxxxxxxxx>

-Mukesh


> > +ÂÂÂÂ * call iio_trigger_poll() nor iio_trigger_poll_chained()
> > +ÂÂÂÂ * directly from DMA callback (under tasklet e.g. softirq).
> > +ÂÂÂÂ * They require respectively HW IRQ and threaded IRQ context
> > +ÂÂÂÂ * as it might sleep.
> > +ÂÂÂÂ */
> > +ÂÂÂ irq_work_queue(&adc->work);
> > Â }
> > Â Â static int stm32_adc_dma_start(struct iio_dev *indio_dev)
> > @@ -1585,8 +1609,10 @@ static void 
> > __stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
> > ÂÂÂÂÂ if (!adc->dma_chan)
> > ÂÂÂÂÂÂÂÂÂ stm32_adc_conv_irq_disable(adc);
> > Â -ÂÂÂ if (adc->dma_chan)
> > +ÂÂÂ if (adc->dma_chan) {
> > ÂÂÂÂÂÂÂÂÂ dmaengine_terminate_all(adc->dma_chan);
> > +ÂÂÂÂÂÂÂ irq_work_sync(&adc->work);
> > +ÂÂÂ }
> > Â ÂÂÂÂÂ if (stm32_adc_set_trig(indio_dev, NULL))
> > ÂÂÂÂÂÂÂÂÂ dev_err(&indio_dev->dev, "Can't clear trigger\n");
> > @@ -1872,6 +1898,8 @@ static int stm32_adc_dma_request(struct iio_dev 
> > *indio_dev)
> > ÂÂÂÂÂ if (ret)
> > ÂÂÂÂÂÂÂÂÂ goto err_free;
> > Â +ÂÂÂ init_irq_work(&adc->work, stm32_adc_dma_irq_work);
> > +
> > ÂÂÂÂÂ return 0;
> > Â Â err_free: