On 2020-08-19 16:24, Marc Zyngier wrote:
>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!
Thanks!
>
>>
>> Signed-off-by: qiuguorui1 <[email protected]>
>
>This definitely needs a Fixes: tag and a Cc: stable, as lost interrupts are
>not fun at all.
OK.I will do this in the 2nd version of patch.
>
>> ---
>> 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):
Thanks for your advice! I will optimize in the 2nd version of patch.
>
>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...
>
