Re: [PATCH] irqchip/stm32-exti: avoid interrupts losing due to clearing pending bit by mistake

[qiuguorui]

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 <qiuguorui1@xxxxxxxxxx>
>
>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...
>