Messages in this thread Patch in this message | | | Date | Wed, 19 Aug 2020 09:24:14 +0100 | From | Marc Zyngier <> | Subject | Re: [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...
| |