| Message ID | 1553604244-10922-1-git-send-email-fabrice.gasnier@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: stm32: fix sleep inside atomic section when using DMA | expand |
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@st.com> Reviewed-by: Mukesh Ojha <mojha@codeaurora.org> -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 > + * 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:
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@st.com> > Reviewed-by: Mukesh Ojha <mojha@codeaurora.org> > > -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@codeaurora.org> -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:
On Tue, 26 Mar 2019 13:44:04 +0100 Fabrice Gasnier <fabrice.gasnier@st.com> 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@st.com> An irq_work is a very heavy weight solution. Perhaps we need a iio_trigger_poll* that can be called from a tasklet? Or indeed a generic irq function that can be. Hmm. This isn't actually a 'real' trigger (I think) anyway as it's only ever calling it's own irq thread. In this case we could just bypass and call that function directly. Sometimes the trigger infrastructure of IIO is just not suited to how a particular device works! We need to maintain the trigger infrastructure for it's ability to select the trigger, but not necessarily push the data through it. So I think it would be safe to just call the block for dma_chan being set in _adc_trigger_handler directly. If you take this approach, make sure there are comments saying why. I don't want people to cut and paste it into new driver unless they understand the reasoning! We have had drivers do this in the past, though I can't find one right now (am335x used to do this for example, but got reworked to suport the touchscreen at the same time and this path went away). Jonathan > --- > 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 > + * 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:
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 + * 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:
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@st.com> --- drivers/iio/adc/Kconfig | 1 + drivers/iio/adc/stm32-adc.c | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-)