| Message ID | 20230725070549.89810-1-luxu.kernel@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Fix local irq restore when flags indicates irq disabled | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be fixes at HEAD ab2dbc7acced |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 2810 this patch: 2810 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | fail | Errors and warnings before: 15877 this patch: 15878 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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. > } > > #endif /* _ASM_RISCV_IRQFLAGS_H */
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 */
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.
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); } #endif /* _ASM_RISCV_IRQFLAGS_H */
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(-)