| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Aug 2020 12:45:53 +0000
From: qiuguorui <qiuguorui1@...wei.com>
To: Marc Zyngier <maz@...nel.org>
CC: "tglx@...utronix.de" <tglx@...utronix.de>,
"jason@...edaemon.net" <jason@...edaemon.net>,
"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
"alexandre.torgue@...com" <alexandre.torgue@...com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"ludovic.barre@...com" <ludovic.barre@...com>,
Zengweilin <zengweilin@...wei.com>,
chenjianguo <chenjianguo3@...wei.com>
Subject: Re: [PATCH] irqchip/stm32-exti: avoid interrupts losing due to
clearing pending bit by mistake
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@...wei.com>
>
>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...
>
Powered by blists - more mailing lists