lkml.org 
[lkml]   [2020]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] irqchip/stm32-exti: avoid interrupts losing due to clearing pending bit by mistake
On 2020-08-19 03:39, qiuguorui1 wrote:
> In the previous code, when the eoi handle of the exti clears the
> pending
> bit of the current interrupt, it will first read the values of fpr and
> rpr, then logically OR the corresponding bit of the interrupt number,
> and finally write back to fpr and rpr.
>
> We found through experiments that if two exti interrupts,
> we call them int1/int2, arrive almost at the same time. in our
> scenario,
> the time difference is 30 microseconds, assuming int1 is triggered
> first.
>
> there will be an extreme scenario: both int's pending bit are set to 1,
> the irq handle of int1 is executed first, and eoi handle is then
> executed,
> at this moment, all pending bits are cleared, but the int 2 has not
> finally been reported to the cpu yet, which eventually lost int2.
>
> According to stm32's TRM description about rpr and fpr: Writing a 1 to
> this
> bit will trigger a rising edge event on event x, Writing 0 has no
> effect.
>
> Therefore, when clearing the pending bit, we only need to clear the
> pending bit of the irq.

Interesting findings!

>
> Signed-off-by: qiuguorui1 <qiuguorui1@huawei.com>

This definitely needs a Fixes: tag and a Cc: stable, as lost
interrupts are not fun at all.

> ---
> drivers/irqchip/irq-stm32-exti.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/irqchip/irq-stm32-exti.c
> b/drivers/irqchip/irq-stm32-exti.c
> index 03a36be757d8..ee4faf5c90b8 100644
> --- a/drivers/irqchip/irq-stm32-exti.c
> +++ b/drivers/irqchip/irq-stm32-exti.c
> @@ -26,6 +26,11 @@
>
> #define HWSPNLCK_TIMEOUT 1000 /* usec */
>
> +enum reg_ops {
> + REG_WRITE_ONLY,
> + REG_READ_WRITE
> +};
> +
> struct stm32_exti_bank {
> u32 imr_ofst;
> u32 emr_ofst;
> @@ -416,13 +421,14 @@ static void stm32_irq_ack(struct irq_data *d)
> irq_gc_unlock(gc);
> }
>
> -static inline u32 stm32_exti_set_bit(struct irq_data *d, u32 reg)
> +static inline u32 stm32_exti_set_bit(struct irq_data *d, u32 reg,
> enum reg_ops op)
> {
> struct stm32_exti_chip_data *chip_data =
> irq_data_get_irq_chip_data(d);
> void __iomem *base = chip_data->host_data->base;
> - u32 val;
> + u32 val = 0;
>
> - val = readl_relaxed(base + reg);
> + if (op == REG_READ_WRITE)
> + val = readl_relaxed(base + reg);
> val |= BIT(d->hwirq % IRQS_PER_BANK);
> writel_relaxed(val, base + reg);
>
> @@ -449,9 +455,9 @@ static void stm32_exti_h_eoi(struct irq_data *d)
>
> raw_spin_lock(&chip_data->rlock);
>
> - stm32_exti_set_bit(d, stm32_bank->rpr_ofst);
> + stm32_exti_set_bit(d, stm32_bank->rpr_ofst, REG_WRITE_ONLY);
> if (stm32_bank->fpr_ofst != UNDEF_REG)
> - stm32_exti_set_bit(d, stm32_bank->fpr_ofst);
> + stm32_exti_set_bit(d, stm32_bank->fpr_ofst, REG_WRITE_ONLY);
>
> raw_spin_unlock(&chip_data->rlock);
>
> @@ -478,7 +484,7 @@ static void stm32_exti_h_unmask(struct irq_data *d)
> const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
>
> raw_spin_lock(&chip_data->rlock);
> - chip_data->mask_cache = stm32_exti_set_bit(d, stm32_bank->imr_ofst);
> + chip_data->mask_cache = stm32_exti_set_bit(d, stm32_bank->imr_ofst,
> REG_READ_WRITE);
> raw_spin_unlock(&chip_data->rlock);
>
> if (d->parent_data->chip)

I think this could be made much simpler by simply providing
an accessor that doesn't do a RMW. Something like this (untested):

diff --git a/drivers/irqchip/irq-stm32-exti.c
b/drivers/irqchip/irq-stm32-exti.c
index 03a36be757d8..e35c5561a10d 100644
--- a/drivers/irqchip/irq-stm32-exti.c
+++ b/drivers/irqchip/irq-stm32-exti.c
@@ -416,6 +416,14 @@ static void stm32_irq_ack(struct irq_data *d)
irq_gc_unlock(gc);
}

+static void stm32_exti_write_bit(struct irq_data *d, u32 reg)
+{
+ struct stm32_exti_chip_data *chip_data =
irq_data_get_irq_chip_data(d);
+ void __iomem *base = chip_data->host_data->base;
+
+ writel_relaxed(BIT(d->hwirq % IRQS_PER_BANK), base + reg);
+}
+
static inline u32 stm32_exti_set_bit(struct irq_data *d, u32 reg)
{
struct stm32_exti_chip_data *chip_data =
irq_data_get_irq_chip_data(d);
@@ -449,9 +457,9 @@ static void stm32_exti_h_eoi(struct irq_data *d)

raw_spin_lock(&chip_data->rlock);

- stm32_exti_set_bit(d, stm32_bank->rpr_ofst);
+ stm32_exti_write_bit(d, stm32_bank->rpr_ofst);
if (stm32_bank->fpr_ofst != UNDEF_REG)
- stm32_exti_set_bit(d, stm32_bank->fpr_ofst);
+ stm32_exti_write_bit(d, stm32_bank->fpr_ofst);

raw_spin_unlock(&chip_data->rlock);

Thanks,
M.
--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-08-19 10:25    [W:0.034 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site