[PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[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)


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[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)

Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Palmer Dabbelt]

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 */

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Palmer Dabbelt]

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 */

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 */

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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 */

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Xu Lu]

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Xu Lu]

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.

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Alexandre Ghiti]

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

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Alexandre Ghiti]

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

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Xu Lu]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Xu Lu]

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

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Alexandre Ghiti]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Alexandre Ghiti]

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

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Andrea Parri]

(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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Andrea Parri]

(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

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Alexandre Ghiti]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Alexandre Ghiti]

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

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Xu Lu]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv: Fix local irq restore when flags indicates irq disabled

[Xu Lu]

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