* [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
@ 2023-07-25 7:05 Xu Lu
2023-08-09 6:05 ` Palmer Dabbelt
0 siblings, 1 reply; 10+ messages in thread
From: Xu Lu @ 2023-07-25 7:05 UTC (permalink / raw)
To: paul.walmsley, palmer, aou; +Cc: linux-riscv, linux-kernel, Xu Lu
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);
}
#endif /* _ASM_RISCV_IRQFLAGS_H */
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2023-07-25 7:05 [PATCH] riscv: Fix local irq restore when flags indicates irq disabled Xu Lu
@ 2023-08-09 6:05 ` Palmer Dabbelt
2023-08-09 6:58 ` [External] " 旭路
0 siblings, 1 reply; 10+ messages in thread
From: Palmer Dabbelt @ 2023-08-09 6:05 UTC (permalink / raw)
To: luxu.kernel; +Cc: Paul Walmsley, aou, linux-riscv, linux-kernel, luxu.kernel
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 */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2023-08-09 6:05 ` Palmer Dabbelt
@ 2023-08-09 6:58 ` 旭路
2023-08-24 12:08 ` Xu Lu
0 siblings, 1 reply; 10+ messages in thread
From: 旭路 @ 2023-08-09 6:58 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: Paul Walmsley, aou, linux-riscv, linux-kernel
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 */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2023-08-09 6:58 ` [External] " 旭路
@ 2023-08-24 12:08 ` Xu Lu
2024-06-18 9:45 ` Alexandre Ghiti
0 siblings, 1 reply; 10+ messages in thread
From: Xu Lu @ 2023-08-24 12:08 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: Paul Walmsley, aou, linux-riscv, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2023-08-24 12:08 ` Xu Lu
@ 2024-06-18 9:45 ` Alexandre Ghiti
2024-06-18 10:00 ` Xu Lu
2024-06-18 11:05 ` Andrea Parri
0 siblings, 2 replies; 10+ messages in thread
From: Alexandre Ghiti @ 2024-06-18 9:45 UTC (permalink / raw)
To: Xu Lu, Palmer Dabbelt; +Cc: Paul Walmsley, aou, linux-riscv, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2024-06-18 9:45 ` Alexandre Ghiti
@ 2024-06-18 10:00 ` Xu Lu
2024-06-18 10:02 ` Alexandre Ghiti
2024-06-18 11:05 ` Andrea Parri
1 sibling, 1 reply; 10+ messages in thread
From: Xu Lu @ 2024-06-18 10:00 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Paul Walmsley, aou, linux-riscv, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2024-06-18 10:00 ` Xu Lu
@ 2024-06-18 10:02 ` Alexandre Ghiti
0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Ghiti @ 2024-06-18 10:02 UTC (permalink / raw)
To: Xu Lu; +Cc: Palmer Dabbelt, Paul Walmsley, aou, linux-riscv, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2024-06-18 9:45 ` Alexandre Ghiti
2024-06-18 10:00 ` Xu Lu
@ 2024-06-18 11:05 ` Andrea Parri
2024-06-18 11:52 ` Alexandre Ghiti
1 sibling, 1 reply; 10+ messages in thread
From: Andrea Parri @ 2024-06-18 11:05 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Xu Lu, Palmer Dabbelt, Paul Walmsley, aou, linux-riscv,
linux-kernel
(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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2024-06-18 11:05 ` Andrea Parri
@ 2024-06-18 11:52 ` Alexandre Ghiti
2024-06-19 3:53 ` Xu Lu
0 siblings, 1 reply; 10+ messages in thread
From: Alexandre Ghiti @ 2024-06-18 11:52 UTC (permalink / raw)
To: Andrea Parri
Cc: Xu Lu, Palmer Dabbelt, Paul Walmsley, aou, linux-riscv,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled
2024-06-18 11:52 ` Alexandre Ghiti
@ 2024-06-19 3:53 ` Xu Lu
0 siblings, 0 replies; 10+ messages in thread
From: Xu Lu @ 2024-06-19 3:53 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Andrea Parri, Palmer Dabbelt, Paul Walmsley, aou, linux-riscv,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-06-19 3:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-25 7:05 [PATCH] riscv: Fix local irq restore when flags indicates irq disabled Xu Lu
2023-08-09 6:05 ` Palmer Dabbelt
2023-08-09 6:58 ` [External] " 旭路
2023-08-24 12:08 ` Xu Lu
2024-06-18 9:45 ` Alexandre Ghiti
2024-06-18 10:00 ` Xu Lu
2024-06-18 10:02 ` Alexandre Ghiti
2024-06-18 11:05 ` Andrea Parri
2024-06-18 11:52 ` Alexandre Ghiti
2024-06-19 3:53 ` Xu Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox