| 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.
Hi Xu Lu, On 24/08/2023 14:08, Xu Lu wrote: > 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. This got lost but this is still correct and needed. Do you think you can respin a new version? Otherwise, I'll do it and keep you SoB. Thanks, Alex > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Alex, I will send a new version later today. Thanks, Xu Lu On Tue, Jun 18, 2024 at 5:45 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Xu Lu, > > On 24/08/2023 14:08, Xu Lu wrote: > > 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. > > > This got lost but this is still correct and needed. > > Do you think you can respin a new version? Otherwise, I'll do it and > keep you SoB. > > Thanks, > > Alex > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 18/06/2024 12:00, Xu Lu wrote: > Hi Alex, > > I will send a new version later today. Great, thanks, I'll make sure it lands in -fixes. Sorry it did not get merged sooner. Thanks, Alex > > Thanks, > > Xu Lu > > On Tue, Jun 18, 2024 at 5:45 PM Alexandre Ghiti <alex@ghiti.fr> wrote: >> Hi Xu Lu, >> >> On 24/08/2023 14:08, Xu Lu wrote: >>> 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. >> >> This got lost but this is still correct and needed. >> >> Do you think you can respin a new version? Otherwise, I'll do it and >> keep you SoB. >> >> Thanks, >> >> Alex >> >> >>> _______________________________________________ >>> linux-riscv mailing list >>> linux-riscv@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-riscv > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
(merging replies) > > > 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. But doesn't that represent some bogus manipulation of the irq flags? cf. config DEBUG_IRQFLAGS bool "Debug IRQ flag manipulation" help Enables checks for potentially unsafe enabling or disabling of interrupts, such as calling raw_local_irq_restore() when interrupts are enabled. in particular, raw_check_bogus_irq_restore() in raw_local_irq_restore(). > This got lost but this is still correct and needed. You mean because of the mentioned rtl8723e example? are there other such instances? IOW, why do you say that the changes in question are needed? Andrea
Hi Andrea, On 18/06/2024 13:05, Andrea Parri wrote: > (merging replies) > >>>> 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. > But doesn't that represent some bogus manipulation of the irq flags? cf. > > config DEBUG_IRQFLAGS > bool "Debug IRQ flag manipulation" > help > Enables checks for potentially unsafe enabling or disabling of > interrupts, such as calling raw_local_irq_restore() when interrupts > are enabled. > > in particular, raw_check_bogus_irq_restore() in raw_local_irq_restore(). > > >> This got lost but this is still correct and needed. > You mean because of the mentioned rtl8723e example? are there other such > instances? IOW, why do you say that the changes in question are needed? Simply because the scenario in this driver and I looked at the arm64 implementation which restores flags unconditionally. But if that's considered bogus, let's drop this. Sorry Xu for the noise, and thanks Andrea for pointing this. Alex > > Andrea
Hi Alex, On Tue, Jun 18, 2024 at 7:52 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Andrea, > > On 18/06/2024 13:05, Andrea Parri wrote: > > (merging replies) > > > >>>> 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. > > But doesn't that represent some bogus manipulation of the irq flags? cf. > > > > config DEBUG_IRQFLAGS > > bool "Debug IRQ flag manipulation" > > help > > Enables checks for potentially unsafe enabling or disabling of > > interrupts, such as calling raw_local_irq_restore() when interrupts > > are enabled. > > > > in particular, raw_check_bogus_irq_restore() in raw_local_irq_restore(). > > > > > >> This got lost but this is still correct and needed. > > You mean because of the mentioned rtl8723e example? are there other such > > instances? IOW, why do you say that the changes in question are needed? > > > Simply because the scenario in this driver and I looked at the arm64 > implementation which restores flags unconditionally. Indeed, ARM64 implementation restores flags unconditionally. Actually in the beginning I found several drivers use local_irq_restore after local_irq_enable. Such drivers include 'scsi/arm/acornscsi.c', 'net/wireless/realtek/rtlwifi/rtlxxx/hw.c', etc. I wonder whether the semantics of local_irq_restore should be restoring irq flags unconditionally and thus I submitted this patch. > > But if that's considered bogus, let's drop this. Sorry Xu for the noise, > and thanks Andrea for pointing this. It's OK if we think this is not a problem. Just ignore this patch. Thanks! Xu Lu > > Alex > > > > > > Andrea
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(-)