Messages in this thread | | | From | 旭路 <> | Date | Wed, 9 Aug 2023 14:58:11 +0800 | Subject | Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled |
| |
On Wed, Aug 9, 2023 at 2:05 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Tue, 25 Jul 2023 00:05:49 PDT (-0700), luxu.kernel@bytedance.com wrote: > > When arch_local_irq_restore() is called with flags indicating irqs > > disabled, we need to clear SR_IE bit in CSR_STATUS, whereas current > > implementation based on csr_set() function only sets SR_IE bit of > > CSR_STATUS when SR_IE bit of flags is high and does nothing when > > SR_IE bit of flags is low. > > > > This commit supplies csr clear operation when calling irq restore > > function with flags indicating irq disabled. > > > > Fixes: 6d60b6ee0c97 ("RISC-V: Device, timer, IRQs, and the SBI") > > Signed-off-by: Xu Lu <luxu.kernel@bytedance.com> > > --- > > arch/riscv/include/asm/irqflags.h | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/irqflags.h b/arch/riscv/include/asm/irqflags.h > > index 08d4d6a5b7e9..7c31fc3c3559 100644 > > --- a/arch/riscv/include/asm/irqflags.h > > +++ b/arch/riscv/include/asm/irqflags.h > > @@ -49,7 +49,10 @@ static inline int arch_irqs_disabled(void) > > /* set interrupt enabled status */ > > static inline void arch_local_irq_restore(unsigned long flags) > > { > > - csr_set(CSR_STATUS, flags & SR_IE); > > + if (flags & SR_IE) > > + csr_set(CSR_STATUS, SR_IE); > > + else > > + csr_clear(CSR_STATUS, SR_IE); > > Unless I'm missing something, the original version is correct: > local_irq_restore() must be paired with a local_irq_save(), so we can > only get here with interrupts disabled.
Yes, it is correct if local_irq_save() is called when irq is enabled before. The flags returned will be SR_IE. And it is safe to call local_irq_restore() with flag SR_IE in any case.
However, if local_irq_save() is called when irq is disabled. The SR_IE bit of flag returned is clear. If some code between local_irq_save() and local_irq_restore() reenables irq, causing the SR_IE bit of CSR_STATUS back to 1, then local_irq_restore() can not restore irq status back to disabled.
Here is an example in existing driver (may not belong to riscv arch) in drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c. The pseudo code behaves like:
int rtl8723e_hw_init(struct ieee80211_hw *hw) { int err; unsigned long flags; ... local_irq_save_flags(flags); local_irq_enable(); ... local_irq_restore(flags); ... return err; }
> > > } > > > > #endif /* _ASM_RISCV_IRQFLAGS_H */
| |