* [PATCH] iio: adc: stm32-adc: fix a race when using several adcs with dma and irq
@ 2019-09-13 13:21 Fabrice Gasnier
2019-09-15 10:05 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Fabrice Gasnier @ 2019-09-13 13:21 UTC (permalink / raw)
To: jic23
Cc: linux-arm-kernel, linux-kernel, mcoquelin.stm32,
alexandre.torgue, fabrice.gasnier, linux-iio, lars, knaack.h,
pmeerw, linux-stm32
End of conversion may be handled by using IRQ or DMA. There may be a
race when two conversions complete at the same time on several ADCs.
EOC can be read as 'set' for several ADCs, with:
- an ADC configured to use IRQs. EOCIE bit is set. The handler is normally
called in this case.
- an ADC configured to use DMA. EOCIE bit isn't set. EOC triggers the DMA
request instead. It's then automatically cleared by DMA read. But the
handler gets called due to status bit is temporarily set (IRQ triggered
by the other ADC).
So both EOC status bit in CSR and EOCIE control bit must be checked
before invoking the interrupt handler (e.g. call ISR only for
IRQ-enabled ADCs).
Fixes: 2763ea0585c9 ("iio: adc: stm32: add optional dma support")
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
drivers/iio/adc/stm32-adc-core.c | 43 +++++++++++++++++++++++++++++++++++++---
drivers/iio/adc/stm32-adc-core.h | 13 ++++++++++++
drivers/iio/adc/stm32-adc.c | 6 ------
3 files changed, 53 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
index 9b85fef..7297396 100644
--- a/drivers/iio/adc/stm32-adc-core.c
+++ b/drivers/iio/adc/stm32-adc-core.c
@@ -71,6 +71,8 @@
* @eoc1: adc1 end of conversion flag in @csr
* @eoc2: adc2 end of conversion flag in @csr
* @eoc3: adc3 end of conversion flag in @csr
+ * @ier: interrupt enable register offset for each adc
+ * @eocie_msk: end of conversion interrupt enable mask in @ier
*/
struct stm32_adc_common_regs {
u32 csr;
@@ -78,6 +80,8 @@ struct stm32_adc_common_regs {
u32 eoc1_msk;
u32 eoc2_msk;
u32 eoc3_msk;
+ u32 ier;
+ u32 eocie_msk;
};
struct stm32_adc_priv;
@@ -303,6 +307,8 @@ static const struct stm32_adc_common_regs stm32f4_adc_common_regs = {
.eoc1_msk = STM32F4_EOC1,
.eoc2_msk = STM32F4_EOC2,
.eoc3_msk = STM32F4_EOC3,
+ .ier = STM32F4_ADC_CR1,
+ .eocie_msk = STM32F4_EOCIE,
};
/* STM32H7 common registers definitions */
@@ -311,8 +317,24 @@ static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
.ccr = STM32H7_ADC_CCR,
.eoc1_msk = STM32H7_EOC_MST,
.eoc2_msk = STM32H7_EOC_SLV,
+ .ier = STM32H7_ADC_IER,
+ .eocie_msk = STM32H7_EOCIE,
};
+static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = {
+ 0, STM32_ADC_OFFSET, STM32_ADC_OFFSET * 2,
+};
+
+static unsigned int stm32_adc_eoc_enabled(struct stm32_adc_priv *priv,
+ unsigned int adc)
+{
+ u32 ier, offset = stm32_adc_offset[adc];
+
+ ier = readl_relaxed(priv->common.base + offset + priv->cfg->regs->ier);
+
+ return ier & priv->cfg->regs->eocie_msk;
+}
+
/* ADC common interrupt for all instances */
static void stm32_adc_irq_handler(struct irq_desc *desc)
{
@@ -323,13 +345,28 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
chained_irq_enter(chip, desc);
status = readl_relaxed(priv->common.base + priv->cfg->regs->csr);
- if (status & priv->cfg->regs->eoc1_msk)
+ /*
+ * End of conversion may be handled by using IRQ or DMA. There may be a
+ * race here when two conversions complete at the same time on several
+ * ADCs. EOC may be read 'set' for several ADCs, with:
+ * - an ADC configured to use DMA (EOC triggers the DMA request, and
+ * is then automatically cleared by DR read in hardware)
+ * - an ADC configured to use IRQs (EOCIE bit is set. The handler must
+ * be called in this case)
+ * So both EOC status bit in CSR and EOCIE control bit must be checked
+ * before invoking the interrupt handler (e.g. call ISR only for
+ * IRQ-enabled ADCs).
+ */
+ if (status & priv->cfg->regs->eoc1_msk &&
+ stm32_adc_eoc_enabled(priv, 0))
generic_handle_irq(irq_find_mapping(priv->domain, 0));
- if (status & priv->cfg->regs->eoc2_msk)
+ if (status & priv->cfg->regs->eoc2_msk &&
+ stm32_adc_eoc_enabled(priv, 1))
generic_handle_irq(irq_find_mapping(priv->domain, 1));
- if (status & priv->cfg->regs->eoc3_msk)
+ if (status & priv->cfg->regs->eoc3_msk &&
+ stm32_adc_eoc_enabled(priv, 2))
generic_handle_irq(irq_find_mapping(priv->domain, 2));
chained_irq_exit(chip, desc);
diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
index 8af507b..8dc936b 100644
--- a/drivers/iio/adc/stm32-adc-core.h
+++ b/drivers/iio/adc/stm32-adc-core.h
@@ -25,8 +25,21 @@
* --------------------------------------------------------
*/
#define STM32_ADC_MAX_ADCS 3
+#define STM32_ADC_OFFSET 0x100
#define STM32_ADCX_COMN_OFFSET 0x300
+/* STM32F4 - registers for each ADC instance */
+#define STM32F4_ADC_CR1 0x04
+
+/* STM32F4_ADC_CR1 - bit fields */
+#define STM32F4_EOCIE BIT(5)
+
+/* STM32H7 - registers for each instance */
+#define STM32H7_ADC_IER 0x04
+
+/* STM32H7_ADC_IER - bit fields */
+#define STM32H7_EOCIE BIT(2)
+
/**
* struct stm32_adc_common - stm32 ADC driver common data (for all instances)
* @base: control registers base cpu addr
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 6a7dd08..3c9f456 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -30,7 +30,6 @@
/* STM32F4 - Registers for each ADC instance */
#define STM32F4_ADC_SR 0x00
-#define STM32F4_ADC_CR1 0x04
#define STM32F4_ADC_CR2 0x08
#define STM32F4_ADC_SMPR1 0x0C
#define STM32F4_ADC_SMPR2 0x10
@@ -54,7 +53,6 @@
#define STM32F4_RES_SHIFT 24
#define STM32F4_RES_MASK GENMASK(25, 24)
#define STM32F4_SCAN BIT(8)
-#define STM32F4_EOCIE BIT(5)
/* STM32F4_ADC_CR2 - bit fields */
#define STM32F4_SWSTART BIT(30)
@@ -69,7 +67,6 @@
/* STM32H7 - Registers for each ADC instance */
#define STM32H7_ADC_ISR 0x00
-#define STM32H7_ADC_IER 0x04
#define STM32H7_ADC_CR 0x08
#define STM32H7_ADC_CFGR 0x0C
#define STM32H7_ADC_SMPR1 0x14
@@ -89,9 +86,6 @@
#define STM32H7_EOC BIT(2)
#define STM32H7_ADRDY BIT(0)
-/* STM32H7_ADC_IER - bit fields */
-#define STM32H7_EOCIE STM32H7_EOC
-
/* STM32H7_ADC_CR - bit fields */
#define STM32H7_ADCAL BIT(31)
#define STM32H7_ADCALDIF BIT(30)
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: adc: stm32-adc: fix a race when using several adcs with dma and irq
2019-09-13 13:21 [PATCH] iio: adc: stm32-adc: fix a race when using several adcs with dma and irq Fabrice Gasnier
@ 2019-09-15 10:05 ` Jonathan Cameron
2019-09-16 11:47 ` Fabrice Gasnier
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2019-09-15 10:05 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: linux-arm-kernel, linux-kernel, mcoquelin.stm32,
alexandre.torgue, linux-iio, lars, knaack.h, pmeerw, linux-stm32
On Fri, 13 Sep 2019 15:21:30 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> End of conversion may be handled by using IRQ or DMA. There may be a
> race when two conversions complete at the same time on several ADCs.
> EOC can be read as 'set' for several ADCs, with:
> - an ADC configured to use IRQs. EOCIE bit is set. The handler is normally
> called in this case.
> - an ADC configured to use DMA. EOCIE bit isn't set. EOC triggers the DMA
> request instead. It's then automatically cleared by DMA read. But the
> handler gets called due to status bit is temporarily set (IRQ triggered
> by the other ADC).
> So both EOC status bit in CSR and EOCIE control bit must be checked
> before invoking the interrupt handler (e.g. call ISR only for
> IRQ-enabled ADCs).
>
> Fixes: 2763ea0585c9 ("iio: adc: stm32: add optional dma support")
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Fix looks fine to me, but I'm not keen on splitting out individual bits from
register defines. That's a long term readability nightmare.
See below,
Jonathan
> ---
> drivers/iio/adc/stm32-adc-core.c | 43 +++++++++++++++++++++++++++++++++++++---
> drivers/iio/adc/stm32-adc-core.h | 13 ++++++++++++
> drivers/iio/adc/stm32-adc.c | 6 ------
> 3 files changed, 53 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> index 9b85fef..7297396 100644
> --- a/drivers/iio/adc/stm32-adc-core.c
> +++ b/drivers/iio/adc/stm32-adc-core.c
> @@ -71,6 +71,8 @@
> * @eoc1: adc1 end of conversion flag in @csr
> * @eoc2: adc2 end of conversion flag in @csr
> * @eoc3: adc3 end of conversion flag in @csr
> + * @ier: interrupt enable register offset for each adc
> + * @eocie_msk: end of conversion interrupt enable mask in @ier
> */
> struct stm32_adc_common_regs {
> u32 csr;
> @@ -78,6 +80,8 @@ struct stm32_adc_common_regs {
> u32 eoc1_msk;
> u32 eoc2_msk;
> u32 eoc3_msk;
> + u32 ier;
> + u32 eocie_msk;
> };
>
> struct stm32_adc_priv;
> @@ -303,6 +307,8 @@ static const struct stm32_adc_common_regs stm32f4_adc_common_regs = {
> .eoc1_msk = STM32F4_EOC1,
> .eoc2_msk = STM32F4_EOC2,
> .eoc3_msk = STM32F4_EOC3,
> + .ier = STM32F4_ADC_CR1,
> + .eocie_msk = STM32F4_EOCIE,
> };
>
> /* STM32H7 common registers definitions */
> @@ -311,8 +317,24 @@ static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
> .ccr = STM32H7_ADC_CCR,
> .eoc1_msk = STM32H7_EOC_MST,
> .eoc2_msk = STM32H7_EOC_SLV,
> + .ier = STM32H7_ADC_IER,
> + .eocie_msk = STM32H7_EOCIE,
> };
>
> +static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = {
> + 0, STM32_ADC_OFFSET, STM32_ADC_OFFSET * 2,
> +};
> +
> +static unsigned int stm32_adc_eoc_enabled(struct stm32_adc_priv *priv,
> + unsigned int adc)
> +{
> + u32 ier, offset = stm32_adc_offset[adc];
> +
> + ier = readl_relaxed(priv->common.base + offset + priv->cfg->regs->ier);
> +
> + return ier & priv->cfg->regs->eocie_msk;
> +}
> +
> /* ADC common interrupt for all instances */
> static void stm32_adc_irq_handler(struct irq_desc *desc)
> {
> @@ -323,13 +345,28 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
> chained_irq_enter(chip, desc);
> status = readl_relaxed(priv->common.base + priv->cfg->regs->csr);
>
> - if (status & priv->cfg->regs->eoc1_msk)
> + /*
> + * End of conversion may be handled by using IRQ or DMA. There may be a
> + * race here when two conversions complete at the same time on several
> + * ADCs. EOC may be read 'set' for several ADCs, with:
> + * - an ADC configured to use DMA (EOC triggers the DMA request, and
> + * is then automatically cleared by DR read in hardware)
> + * - an ADC configured to use IRQs (EOCIE bit is set. The handler must
> + * be called in this case)
> + * So both EOC status bit in CSR and EOCIE control bit must be checked
> + * before invoking the interrupt handler (e.g. call ISR only for
> + * IRQ-enabled ADCs).
> + */
> + if (status & priv->cfg->regs->eoc1_msk &&
> + stm32_adc_eoc_enabled(priv, 0))
> generic_handle_irq(irq_find_mapping(priv->domain, 0));
>
> - if (status & priv->cfg->regs->eoc2_msk)
> + if (status & priv->cfg->regs->eoc2_msk &&
> + stm32_adc_eoc_enabled(priv, 1))
> generic_handle_irq(irq_find_mapping(priv->domain, 1));
>
> - if (status & priv->cfg->regs->eoc3_msk)
> + if (status & priv->cfg->regs->eoc3_msk &&
> + stm32_adc_eoc_enabled(priv, 2))
> generic_handle_irq(irq_find_mapping(priv->domain, 2));
>
> chained_irq_exit(chip, desc);
> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> index 8af507b..8dc936b 100644
> --- a/drivers/iio/adc/stm32-adc-core.h
> +++ b/drivers/iio/adc/stm32-adc-core.h
> @@ -25,8 +25,21 @@
> * --------------------------------------------------------
> */
> #define STM32_ADC_MAX_ADCS 3
> +#define STM32_ADC_OFFSET 0x100
> #define STM32_ADCX_COMN_OFFSET 0x300
>
> +/* STM32F4 - registers for each ADC instance */
> +#define STM32F4_ADC_CR1 0x04
> +
> +/* STM32F4_ADC_CR1 - bit fields */
> +#define STM32F4_EOCIE BIT(5)
> +
> +/* STM32H7 - registers for each instance */
> +#define STM32H7_ADC_IER 0x04
> +
> +/* STM32H7_ADC_IER - bit fields */
> +#define STM32H7_EOCIE BIT(2)
> +
> /**
> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
> * @base: control registers base cpu addr
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 6a7dd08..3c9f456 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -30,7 +30,6 @@
>
> /* STM32F4 - Registers for each ADC instance */
> #define STM32F4_ADC_SR 0x00
> -#define STM32F4_ADC_CR1 0x04
> #define STM32F4_ADC_CR2 0x08
> #define STM32F4_ADC_SMPR1 0x0C
> #define STM32F4_ADC_SMPR2 0x10
> @@ -54,7 +53,6 @@
> #define STM32F4_RES_SHIFT 24
> #define STM32F4_RES_MASK GENMASK(25, 24)
> #define STM32F4_SCAN BIT(8)
> -#define STM32F4_EOCIE BIT(5)
Hmm. This is breaking up the definitions of bits in a single register.
That is rather nasty from a code readability point of view.
I am as keen as the next person on only exposing definitions where
we need to, but in this case we either need to provide an access path
to it here, or we need to move the whole block to the header.
>
> /* STM32F4_ADC_CR2 - bit fields */
> #define STM32F4_SWSTART BIT(30)
> @@ -69,7 +67,6 @@
>
> /* STM32H7 - Registers for each ADC instance */
> #define STM32H7_ADC_ISR 0x00
> -#define STM32H7_ADC_IER 0x04
> #define STM32H7_ADC_CR 0x08
> #define STM32H7_ADC_CFGR 0x0C
> #define STM32H7_ADC_SMPR1 0x14
> @@ -89,9 +86,6 @@
> #define STM32H7_EOC BIT(2)
> #define STM32H7_ADRDY BIT(0)
>
> -/* STM32H7_ADC_IER - bit fields */
> -#define STM32H7_EOCIE STM32H7_EOC
> -
> /* STM32H7_ADC_CR - bit fields */
> #define STM32H7_ADCAL BIT(31)
> #define STM32H7_ADCALDIF BIT(30)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: adc: stm32-adc: fix a race when using several adcs with dma and irq
2019-09-15 10:05 ` Jonathan Cameron
@ 2019-09-16 11:47 ` Fabrice Gasnier
2019-09-17 10:11 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Fabrice Gasnier @ 2019-09-16 11:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-arm-kernel, linux-kernel, mcoquelin.stm32,
alexandre.torgue, linux-iio, lars, knaack.h, pmeerw, linux-stm32
On 9/15/19 12:05 PM, Jonathan Cameron wrote:
> On Fri, 13 Sep 2019 15:21:30 +0200
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
>
>> End of conversion may be handled by using IRQ or DMA. There may be a
>> race when two conversions complete at the same time on several ADCs.
>> EOC can be read as 'set' for several ADCs, with:
>> - an ADC configured to use IRQs. EOCIE bit is set. The handler is normally
>> called in this case.
>> - an ADC configured to use DMA. EOCIE bit isn't set. EOC triggers the DMA
>> request instead. It's then automatically cleared by DMA read. But the
>> handler gets called due to status bit is temporarily set (IRQ triggered
>> by the other ADC).
>> So both EOC status bit in CSR and EOCIE control bit must be checked
>> before invoking the interrupt handler (e.g. call ISR only for
>> IRQ-enabled ADCs).
>>
>> Fixes: 2763ea0585c9 ("iio: adc: stm32: add optional dma support")
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Fix looks fine to me, but I'm not keen on splitting out individual bits from
> register defines. That's a long term readability nightmare.
>
> See below,
>
> Jonathan
>
>> ---
>> drivers/iio/adc/stm32-adc-core.c | 43 +++++++++++++++++++++++++++++++++++++---
>> drivers/iio/adc/stm32-adc-core.h | 13 ++++++++++++
>> drivers/iio/adc/stm32-adc.c | 6 ------
>> 3 files changed, 53 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
>> index 9b85fef..7297396 100644
>> --- a/drivers/iio/adc/stm32-adc-core.c
>> +++ b/drivers/iio/adc/stm32-adc-core.c
>> @@ -71,6 +71,8 @@
>> * @eoc1: adc1 end of conversion flag in @csr
>> * @eoc2: adc2 end of conversion flag in @csr
>> * @eoc3: adc3 end of conversion flag in @csr
>> + * @ier: interrupt enable register offset for each adc
>> + * @eocie_msk: end of conversion interrupt enable mask in @ier
>> */
>> struct stm32_adc_common_regs {
>> u32 csr;
>> @@ -78,6 +80,8 @@ struct stm32_adc_common_regs {
>> u32 eoc1_msk;
>> u32 eoc2_msk;
>> u32 eoc3_msk;
>> + u32 ier;
>> + u32 eocie_msk;
>> };
>>
>> struct stm32_adc_priv;
>> @@ -303,6 +307,8 @@ static const struct stm32_adc_common_regs stm32f4_adc_common_regs = {
>> .eoc1_msk = STM32F4_EOC1,
>> .eoc2_msk = STM32F4_EOC2,
>> .eoc3_msk = STM32F4_EOC3,
>> + .ier = STM32F4_ADC_CR1,
>> + .eocie_msk = STM32F4_EOCIE,
>> };
>>
>> /* STM32H7 common registers definitions */
>> @@ -311,8 +317,24 @@ static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
>> .ccr = STM32H7_ADC_CCR,
>> .eoc1_msk = STM32H7_EOC_MST,
>> .eoc2_msk = STM32H7_EOC_SLV,
>> + .ier = STM32H7_ADC_IER,
>> + .eocie_msk = STM32H7_EOCIE,
>> };
>>
>> +static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = {
>> + 0, STM32_ADC_OFFSET, STM32_ADC_OFFSET * 2,
>> +};
>> +
>> +static unsigned int stm32_adc_eoc_enabled(struct stm32_adc_priv *priv,
>> + unsigned int adc)
>> +{
>> + u32 ier, offset = stm32_adc_offset[adc];
>> +
>> + ier = readl_relaxed(priv->common.base + offset + priv->cfg->regs->ier);
>> +
>> + return ier & priv->cfg->regs->eocie_msk;
>> +}
>> +
>> /* ADC common interrupt for all instances */
>> static void stm32_adc_irq_handler(struct irq_desc *desc)
>> {
>> @@ -323,13 +345,28 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
>> chained_irq_enter(chip, desc);
>> status = readl_relaxed(priv->common.base + priv->cfg->regs->csr);
>>
>> - if (status & priv->cfg->regs->eoc1_msk)
>> + /*
>> + * End of conversion may be handled by using IRQ or DMA. There may be a
>> + * race here when two conversions complete at the same time on several
>> + * ADCs. EOC may be read 'set' for several ADCs, with:
>> + * - an ADC configured to use DMA (EOC triggers the DMA request, and
>> + * is then automatically cleared by DR read in hardware)
>> + * - an ADC configured to use IRQs (EOCIE bit is set. The handler must
>> + * be called in this case)
>> + * So both EOC status bit in CSR and EOCIE control bit must be checked
>> + * before invoking the interrupt handler (e.g. call ISR only for
>> + * IRQ-enabled ADCs).
>> + */
>> + if (status & priv->cfg->regs->eoc1_msk &&
>> + stm32_adc_eoc_enabled(priv, 0))
>> generic_handle_irq(irq_find_mapping(priv->domain, 0));
>>
>> - if (status & priv->cfg->regs->eoc2_msk)
>> + if (status & priv->cfg->regs->eoc2_msk &&
>> + stm32_adc_eoc_enabled(priv, 1))
>> generic_handle_irq(irq_find_mapping(priv->domain, 1));
>>
>> - if (status & priv->cfg->regs->eoc3_msk)
>> + if (status & priv->cfg->regs->eoc3_msk &&
>> + stm32_adc_eoc_enabled(priv, 2))
>> generic_handle_irq(irq_find_mapping(priv->domain, 2));
>>
>> chained_irq_exit(chip, desc);
>> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
>> index 8af507b..8dc936b 100644
>> --- a/drivers/iio/adc/stm32-adc-core.h
>> +++ b/drivers/iio/adc/stm32-adc-core.h
>> @@ -25,8 +25,21 @@
>> * --------------------------------------------------------
>> */
>> #define STM32_ADC_MAX_ADCS 3
>> +#define STM32_ADC_OFFSET 0x100
>> #define STM32_ADCX_COMN_OFFSET 0x300
>>
>> +/* STM32F4 - registers for each ADC instance */
>> +#define STM32F4_ADC_CR1 0x04
>> +
>> +/* STM32F4_ADC_CR1 - bit fields */
>> +#define STM32F4_EOCIE BIT(5)
>> +
>> +/* STM32H7 - registers for each instance */
>> +#define STM32H7_ADC_IER 0x04
>> +
>> +/* STM32H7_ADC_IER - bit fields */
>> +#define STM32H7_EOCIE BIT(2)
>> +
>> /**
>> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
>> * @base: control registers base cpu addr
>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>> index 6a7dd08..3c9f456 100644
>> --- a/drivers/iio/adc/stm32-adc.c
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -30,7 +30,6 @@
>>
>> /* STM32F4 - Registers for each ADC instance */
>> #define STM32F4_ADC_SR 0x00
>> -#define STM32F4_ADC_CR1 0x04
>> #define STM32F4_ADC_CR2 0x08
>> #define STM32F4_ADC_SMPR1 0x0C
>> #define STM32F4_ADC_SMPR2 0x10
>> @@ -54,7 +53,6 @@
>> #define STM32F4_RES_SHIFT 24
>> #define STM32F4_RES_MASK GENMASK(25, 24)
>> #define STM32F4_SCAN BIT(8)
>> -#define STM32F4_EOCIE BIT(5)
> Hmm. This is breaking up the definitions of bits in a single register.
> That is rather nasty from a code readability point of view.
>
> I am as keen as the next person on only exposing definitions where
> we need to, but in this case we either need to provide an access path
> to it here, or we need to move the whole block to the header.
Hi Jonathan,
I think I'll add a precursor patch in v2 to move the whole block to the
header file. This way, the access path is easy (e.g. readl).
I'm only wondering about the Fixes tag... this will probably not be
straight forward to apply the fix on the maintenance releases ?
Or do I need to add it to the precursor patch as well ?
Thanks for reviewing,
Best regards,
Fabrice
>
>>
>> /* STM32F4_ADC_CR2 - bit fields */
>> #define STM32F4_SWSTART BIT(30)
>> @@ -69,7 +67,6 @@
>>
>> /* STM32H7 - Registers for each ADC instance */
>> #define STM32H7_ADC_ISR 0x00
>> -#define STM32H7_ADC_IER 0x04
>> #define STM32H7_ADC_CR 0x08
>> #define STM32H7_ADC_CFGR 0x0C
>> #define STM32H7_ADC_SMPR1 0x14
>> @@ -89,9 +86,6 @@
>> #define STM32H7_EOC BIT(2)
>> #define STM32H7_ADRDY BIT(0)
>>
>> -/* STM32H7_ADC_IER - bit fields */
>> -#define STM32H7_EOCIE STM32H7_EOC
>> -
>> /* STM32H7_ADC_CR - bit fields */
>> #define STM32H7_ADCAL BIT(31)
>> #define STM32H7_ADCALDIF BIT(30)
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: adc: stm32-adc: fix a race when using several adcs with dma and irq
2019-09-16 11:47 ` Fabrice Gasnier
@ 2019-09-17 10:11 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2019-09-17 10:11 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: Jonathan Cameron, lars, alexandre.torgue, linux-iio, pmeerw,
linux-kernel, mcoquelin.stm32, knaack.h, linux-stm32,
linux-arm-kernel
On Mon, 16 Sep 2019 13:47:34 +0200
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> On 9/15/19 12:05 PM, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2019 15:21:30 +0200
> > Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> >
> >> End of conversion may be handled by using IRQ or DMA. There may be a
> >> race when two conversions complete at the same time on several ADCs.
> >> EOC can be read as 'set' for several ADCs, with:
> >> - an ADC configured to use IRQs. EOCIE bit is set. The handler is normally
> >> called in this case.
> >> - an ADC configured to use DMA. EOCIE bit isn't set. EOC triggers the DMA
> >> request instead. It's then automatically cleared by DMA read. But the
> >> handler gets called due to status bit is temporarily set (IRQ triggered
> >> by the other ADC).
> >> So both EOC status bit in CSR and EOCIE control bit must be checked
> >> before invoking the interrupt handler (e.g. call ISR only for
> >> IRQ-enabled ADCs).
> >>
> >> Fixes: 2763ea0585c9 ("iio: adc: stm32: add optional dma support")
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > Fix looks fine to me, but I'm not keen on splitting out individual bits from
> > register defines. That's a long term readability nightmare.
> >
> > See below,
> >
> > Jonathan
> >
> >> ---
> >> drivers/iio/adc/stm32-adc-core.c | 43 +++++++++++++++++++++++++++++++++++++---
> >> drivers/iio/adc/stm32-adc-core.h | 13 ++++++++++++
> >> drivers/iio/adc/stm32-adc.c | 6 ------
> >> 3 files changed, 53 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> >> index 9b85fef..7297396 100644
> >> --- a/drivers/iio/adc/stm32-adc-core.c
> >> +++ b/drivers/iio/adc/stm32-adc-core.c
> >> @@ -71,6 +71,8 @@
> >> * @eoc1: adc1 end of conversion flag in @csr
> >> * @eoc2: adc2 end of conversion flag in @csr
> >> * @eoc3: adc3 end of conversion flag in @csr
> >> + * @ier: interrupt enable register offset for each adc
> >> + * @eocie_msk: end of conversion interrupt enable mask in @ier
> >> */
> >> struct stm32_adc_common_regs {
> >> u32 csr;
> >> @@ -78,6 +80,8 @@ struct stm32_adc_common_regs {
> >> u32 eoc1_msk;
> >> u32 eoc2_msk;
> >> u32 eoc3_msk;
> >> + u32 ier;
> >> + u32 eocie_msk;
> >> };
> >>
> >> struct stm32_adc_priv;
> >> @@ -303,6 +307,8 @@ static const struct stm32_adc_common_regs stm32f4_adc_common_regs = {
> >> .eoc1_msk = STM32F4_EOC1,
> >> .eoc2_msk = STM32F4_EOC2,
> >> .eoc3_msk = STM32F4_EOC3,
> >> + .ier = STM32F4_ADC_CR1,
> >> + .eocie_msk = STM32F4_EOCIE,
> >> };
> >>
> >> /* STM32H7 common registers definitions */
> >> @@ -311,8 +317,24 @@ static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
> >> .ccr = STM32H7_ADC_CCR,
> >> .eoc1_msk = STM32H7_EOC_MST,
> >> .eoc2_msk = STM32H7_EOC_SLV,
> >> + .ier = STM32H7_ADC_IER,
> >> + .eocie_msk = STM32H7_EOCIE,
> >> };
> >>
> >> +static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = {
> >> + 0, STM32_ADC_OFFSET, STM32_ADC_OFFSET * 2,
> >> +};
> >> +
> >> +static unsigned int stm32_adc_eoc_enabled(struct stm32_adc_priv *priv,
> >> + unsigned int adc)
> >> +{
> >> + u32 ier, offset = stm32_adc_offset[adc];
> >> +
> >> + ier = readl_relaxed(priv->common.base + offset + priv->cfg->regs->ier);
> >> +
> >> + return ier & priv->cfg->regs->eocie_msk;
> >> +}
> >> +
> >> /* ADC common interrupt for all instances */
> >> static void stm32_adc_irq_handler(struct irq_desc *desc)
> >> {
> >> @@ -323,13 +345,28 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
> >> chained_irq_enter(chip, desc);
> >> status = readl_relaxed(priv->common.base + priv->cfg->regs->csr);
> >>
> >> - if (status & priv->cfg->regs->eoc1_msk)
> >> + /*
> >> + * End of conversion may be handled by using IRQ or DMA. There may be a
> >> + * race here when two conversions complete at the same time on several
> >> + * ADCs. EOC may be read 'set' for several ADCs, with:
> >> + * - an ADC configured to use DMA (EOC triggers the DMA request, and
> >> + * is then automatically cleared by DR read in hardware)
> >> + * - an ADC configured to use IRQs (EOCIE bit is set. The handler must
> >> + * be called in this case)
> >> + * So both EOC status bit in CSR and EOCIE control bit must be checked
> >> + * before invoking the interrupt handler (e.g. call ISR only for
> >> + * IRQ-enabled ADCs).
> >> + */
> >> + if (status & priv->cfg->regs->eoc1_msk &&
> >> + stm32_adc_eoc_enabled(priv, 0))
> >> generic_handle_irq(irq_find_mapping(priv->domain, 0));
> >>
> >> - if (status & priv->cfg->regs->eoc2_msk)
> >> + if (status & priv->cfg->regs->eoc2_msk &&
> >> + stm32_adc_eoc_enabled(priv, 1))
> >> generic_handle_irq(irq_find_mapping(priv->domain, 1));
> >>
> >> - if (status & priv->cfg->regs->eoc3_msk)
> >> + if (status & priv->cfg->regs->eoc3_msk &&
> >> + stm32_adc_eoc_enabled(priv, 2))
> >> generic_handle_irq(irq_find_mapping(priv->domain, 2));
> >>
> >> chained_irq_exit(chip, desc);
> >> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> >> index 8af507b..8dc936b 100644
> >> --- a/drivers/iio/adc/stm32-adc-core.h
> >> +++ b/drivers/iio/adc/stm32-adc-core.h
> >> @@ -25,8 +25,21 @@
> >> * --------------------------------------------------------
> >> */
> >> #define STM32_ADC_MAX_ADCS 3
> >> +#define STM32_ADC_OFFSET 0x100
> >> #define STM32_ADCX_COMN_OFFSET 0x300
> >>
> >> +/* STM32F4 - registers for each ADC instance */
> >> +#define STM32F4_ADC_CR1 0x04
> >> +
> >> +/* STM32F4_ADC_CR1 - bit fields */
> >> +#define STM32F4_EOCIE BIT(5)
> >> +
> >> +/* STM32H7 - registers for each instance */
> >> +#define STM32H7_ADC_IER 0x04
> >> +
> >> +/* STM32H7_ADC_IER - bit fields */
> >> +#define STM32H7_EOCIE BIT(2)
> >> +
> >> /**
> >> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
> >> * @base: control registers base cpu addr
> >> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> >> index 6a7dd08..3c9f456 100644
> >> --- a/drivers/iio/adc/stm32-adc.c
> >> +++ b/drivers/iio/adc/stm32-adc.c
> >> @@ -30,7 +30,6 @@
> >>
> >> /* STM32F4 - Registers for each ADC instance */
> >> #define STM32F4_ADC_SR 0x00
> >> -#define STM32F4_ADC_CR1 0x04
> >> #define STM32F4_ADC_CR2 0x08
> >> #define STM32F4_ADC_SMPR1 0x0C
> >> #define STM32F4_ADC_SMPR2 0x10
> >> @@ -54,7 +53,6 @@
> >> #define STM32F4_RES_SHIFT 24
> >> #define STM32F4_RES_MASK GENMASK(25, 24)
> >> #define STM32F4_SCAN BIT(8)
> >> -#define STM32F4_EOCIE BIT(5)
> > Hmm. This is breaking up the definitions of bits in a single register.
> > That is rather nasty from a code readability point of view.
> >
> > I am as keen as the next person on only exposing definitions where
> > we need to, but in this case we either need to provide an access path
> > to it here, or we need to move the whole block to the header.
>
> Hi Jonathan,
>
> I think I'll add a precursor patch in v2 to move the whole block to the
> header file. This way, the access path is easy (e.g. readl).
> I'm only wondering about the Fixes tag... this will probably not be
> straight forward to apply the fix on the maintenance releases ?
> Or do I need to add it to the precursor patch as well ?
The precursor is a simple move of definitions. Even if it's large, I don't
think it will be a problem applying it to stable.
Just make it clear in the patch description why it is needed for the fix.
Thanks,
Jonathan
>
> Thanks for reviewing,
> Best regards,
> Fabrice
>
> >
> >>
> >> /* STM32F4_ADC_CR2 - bit fields */
> >> #define STM32F4_SWSTART BIT(30)
> >> @@ -69,7 +67,6 @@
> >>
> >> /* STM32H7 - Registers for each ADC instance */
> >> #define STM32H7_ADC_ISR 0x00
> >> -#define STM32H7_ADC_IER 0x04
> >> #define STM32H7_ADC_CR 0x08
> >> #define STM32H7_ADC_CFGR 0x0C
> >> #define STM32H7_ADC_SMPR1 0x14
> >> @@ -89,9 +86,6 @@
> >> #define STM32H7_EOC BIT(2)
> >> #define STM32H7_ADRDY BIT(0)
> >>
> >> -/* STM32H7_ADC_IER - bit fields */
> >> -#define STM32H7_EOCIE STM32H7_EOC
> >> -
> >> /* STM32H7_ADC_CR - bit fields */
> >> #define STM32H7_ADCAL BIT(31)
> >> #define STM32H7_ADCALDIF BIT(30)
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-17 10:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-13 13:21 [PATCH] iio: adc: stm32-adc: fix a race when using several adcs with dma and irq Fabrice Gasnier
2019-09-15 10:05 ` Jonathan Cameron
2019-09-16 11:47 ` Fabrice Gasnier
2019-09-17 10:11 ` Jonathan Cameron
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).