Messages in this thread | | | From | Xu Lu <> | Date | Thu, 24 Aug 2023 20:08:02 +0800 | Subject | Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled |
| |
On Wed, Aug 9, 2023 at 2:58 PM 旭路 <luxu.kernel@bytedance.com> wrote: > > 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 */
A gentle ping.
| |