[PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Bharat Kumar Gogada]

-Realtek 8192CE chipset maintains local irq flags after enabling/disabling
hardware interrupts. 
-Hardware interrupts are enabled before enabling the local irq 
flags(these flags are being checked in interrupt handler), 
leading to race condition on some RP, where the irq line between 
bridge and GIC goes high at ASSERT_INTx and goes low only 
at DEASSERT_INTx. In this kind of RP by the time ASSERT_INTx is seen
irq_enable flag is still set to false, resulting in continuous 
interrupts seen by CPU as DEASSERT_INTx cannot be sent since 
flag is still false and making CPU stall.
-Changing the sequence of setting these irq flags.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
index a47be73..143766c4 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
@@ -1306,9 +1306,9 @@ void rtl92ce_enable_interrupt(struct ieee80211_hw *hw)
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 
+	rtlpci->irq_enabled = true;
 	rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
 	rtl_write_dword(rtlpriv, REG_HIMRE, rtlpci->irq_mask[1] & 0xFFFFFFFF);
-	rtlpci->irq_enabled = true;
 }
 
 void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
@@ -1316,9 +1316,9 @@ void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
 
+	rtlpci->irq_enabled = false;
 	rtl_write_dword(rtlpriv, REG_HIMR, IMR8190_DISABLED);
 	rtl_write_dword(rtlpriv, REG_HIMRE, IMR8190_DISABLED);
-	rtlpci->irq_enabled = false;
 }
 
 static void _rtl92ce_poweroff_adapter(struct ieee80211_hw *hw)
-- 
2.1.1

Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Lino Sanfilippo]

Hi,

altek/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
> index a47be73..143766c4 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
> @@ -1306,9 +1306,9 @@ void rtl92ce_enable_interrupt(struct ieee80211_hw *hw)
>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>  
> +	rtlpci->irq_enabled = true;
>  	rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
>  	rtl_write_dword(rtlpriv, REG_HIMRE, rtlpci->irq_mask[1] & 0xFFFFFFFF);
> -	rtlpci->irq_enabled = true;
>  }
>  
>  void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
> @@ -1316,9 +1316,9 @@ void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>  
> +	rtlpci->irq_enabled = false;
>  	rtl_write_dword(rtlpriv, REG_HIMR, IMR8190_DISABLED);
>  	rtl_write_dword(rtlpriv, REG_HIMRE, IMR8190_DISABLED);
> -	rtlpci->irq_enabled = false;
>  }
>  

AFAIK you also have to use memory barriers here to ensure that
the concerning instructions are not reordered, and both irq handler
and process have a consistent perception of irq_enabled, e.g:

rtlpci->irq_enabled = true;
smp_wmb();
rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);

and in the irq handler

if (rtlpci->irq_enabled == 0) {
        smp_rmb();
	return ret;
}


Regards,
Lino

Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Larry Finger]

On 01/19/2017 08:35 AM, Lino Sanfilippo wrote:
> Hi,
>
> altek/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
>> index a47be73..143766c4 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
>> @@ -1306,9 +1306,9 @@ void rtl92ce_enable_interrupt(struct ieee80211_hw *hw)
>>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
>>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>>
>> +	rtlpci->irq_enabled = true;
>>  	rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
>>  	rtl_write_dword(rtlpriv, REG_HIMRE, rtlpci->irq_mask[1] & 0xFFFFFFFF);
>> -	rtlpci->irq_enabled = true;
>>  }
>>
>>  void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
>> @@ -1316,9 +1316,9 @@ void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
>>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
>>  	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>>
>> +	rtlpci->irq_enabled = false;
>>  	rtl_write_dword(rtlpriv, REG_HIMR, IMR8190_DISABLED);
>>  	rtl_write_dword(rtlpriv, REG_HIMRE, IMR8190_DISABLED);
>> -	rtlpci->irq_enabled = false;
>>  }
>>
>
> AFAIK you also have to use memory barriers here to ensure that
> the concerning instructions are not reordered, and both irq handler
> and process have a consistent perception of irq_enabled, e.g:
>
> rtlpci->irq_enabled = true;
> smp_wmb();
> rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
>
> and in the irq handler
>
> if (rtlpci->irq_enabled == 0) {
>         smp_rmb();
> 	return ret;
> }

I can see the potential race condition between setting interrupts and setting 
the flag, and I will likely accept Bharat's patch after testing.

I am likely displaying my ignorance regarding instruction reordering, but what 
compiler/cpu combination is likely to move a simple set operation after a call 
to an external routine? Is the smp_wmb() operation really needed? I am also 
unsure of the smp_rmb() call in the interrupt handler. Neither instruction 
should cause any problems, but I'm not sure they are needed.

Larry

Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Lino Sanfilippo]

Hi,

On 19.01.2017 19:08, Larry Finger wrote:
> On 01/19/2017 08:35 AM, Lino Sanfilippo wrote:
>> Hi,
>>
>> altek/rtlwifi/rtl8192ce/hw.c 
>> b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
>>> index a47be73..143766c4 100644
>>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
>>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
>>> @@ -1306,9 +1306,9 @@ void rtl92ce_enable_interrupt(struct 
>>> ieee80211_hw *hw)
>>>      struct rtl_priv *rtlpriv = rtl_priv(hw);
>>>      struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>>>
>>> +    rtlpci->irq_enabled = true;
>>>      rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 
>>> 0xFFFFFFFF);
>>>      rtl_write_dword(rtlpriv, REG_HIMRE, rtlpci->irq_mask[1] & 
>>> 0xFFFFFFFF);
>>> -    rtlpci->irq_enabled = true;
>>>  }
>>>
>>>  void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
>>> @@ -1316,9 +1316,9 @@ void rtl92ce_disable_interrupt(struct 
>>> ieee80211_hw *hw)
>>>      struct rtl_priv *rtlpriv = rtl_priv(hw);
>>>      struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>>>
>>> +    rtlpci->irq_enabled = false;
>>>      rtl_write_dword(rtlpriv, REG_HIMR, IMR8190_DISABLED);
>>>      rtl_write_dword(rtlpriv, REG_HIMRE, IMR8190_DISABLED);
>>> -    rtlpci->irq_enabled = false;
>>>  }
>>>
>>
>> AFAIK you also have to use memory barriers here to ensure that
>> the concerning instructions are not reordered, and both irq handler
>> and process have a consistent perception of irq_enabled, e.g:
>>
>> rtlpci->irq_enabled = true;
>> smp_wmb();
>> rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
>>
>> and in the irq handler
>>
>> if (rtlpci->irq_enabled == 0) {
>>         smp_rmb();
>>     return ret;
>> }
>
> I can see the potential race condition between setting interrupts and 
> setting the flag, and I will likely accept Bharat's patch after testing.
>
> I am likely displaying my ignorance regarding instruction reordering, 
> but what compiler/cpu combination is likely to move a simple set 
> operation after a call to an external routine? Is the smp_wmb() 
> operation really needed?

I dont know if and when a specific compiler would actually do such a 
reordering. But the thing is, that you simply cant be sure
that it does not. As I wrote it is also not only reordering that could 
cause trouble, but also a different perception of the
flag value on different CPUs.
We guard against those issues in other drivers, too. See

http://lxr.free-electrons.com/source/drivers/net/ethernet/3com/typhoon.c#L1935

for example.

> I am also unsure of the smp_rmb() call in the interrupt handler. 
> Neither instruction should cause any problems, but I'm not sure they 
> are needed

The smp_rmb() is needed to ensure that the irq handler actually "sees" 
the most recent value of  irq_enabled even if the irq handler is
running on another CPU than the one that set this flag.

Regards,
Lino

Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Larry Finger]

On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
> -Realtek 8192CE chipset maintains local irq flags after enabling/disabling
> hardware interrupts.
> -Hardware interrupts are enabled before enabling the local irq
> flags(these flags are being checked in interrupt handler),
> leading to race condition on some RP, where the irq line between
> bridge and GIC goes high at ASSERT_INTx and goes low only
> at DEASSERT_INTx. In this kind of RP by the time ASSERT_INTx is seen
> irq_enable flag is still set to false, resulting in continuous
> interrupts seen by CPU as DEASSERT_INTx cannot be sent since
> flag is still false and making CPU stall.
> -Changing the sequence of setting these irq flags.
>
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---

This patch should be enhanced with the smb_xx() calls as suggested by by Lino.

The subject should be changed. I would suggest something like "rtlwifi: 
rtl8192ce: Prevent race condition when enabling interrupts", as it explains the 
condition you are preventing.

The other PCI drivers also have the same problem. Do you want to prepare the 
patches, or should I do it?

Larry

Re: Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Lino Sanfilippo]

Hi,

>
> 
> This patch should be enhanced with the smb_xx() calls as suggested by by Lino.
> 

If you do this, please place the smp_rmb() before the if condition in the irq
handler like

smp_rmb();
if (rtlpci->irq_enabled == 0) {
    return ret;


as I think that the suggestion I made before was not correct (sorry for the
confusion). 

Regards,
Lino 
 

RE: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Bharat Kumar Gogada]

 > On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
> > -Realtek 8192CE chipset maintains local irq flags after enabling/disabling
> > hardware interrupts.
> > -Hardware interrupts are enabled before enabling the local irq
> > flags(these flags are being checked in interrupt handler),
> > leading to race condition on some RP, where the irq line between
> > bridge and GIC goes high at ASSERT_INTx and goes low only
> > at DEASSERT_INTx. In this kind of RP by the time ASSERT_INTx is seen
> > irq_enable flag is still set to false, resulting in continuous
> > interrupts seen by CPU as DEASSERT_INTx cannot be sent since
> > flag is still false and making CPU stall.
> > -Changing the sequence of setting these irq flags.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> 
> This patch should be enhanced with the smb_xx() calls as suggested by by Lino.
> 
> The subject should be changed. I would suggest something like "rtlwifi:
> rtl8192ce: Prevent race condition when enabling interrupts", as it explains the
> condition you are preventing.
> 
> The other PCI drivers also have the same problem. Do you want to prepare the
> patches, or should I do it?
> 
Thanks Larry. Please send out the patches adding the above enhancements suggested by Lino.

Bharat

Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Larry Finger]

On 01/20/2017 08:14 AM, Bharat Kumar Gogada wrote:
>  > On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
>>> -Realtek 8192CE chipset maintains local irq flags after enabling/disabling
>>> hardware interrupts.
>>> -Hardware interrupts are enabled before enabling the local irq
>>> flags(these flags are being checked in interrupt handler),
>>> leading to race condition on some RP, where the irq line between
>>> bridge and GIC goes high at ASSERT_INTx and goes low only
>>> at DEASSERT_INTx. In this kind of RP by the time ASSERT_INTx is seen
>>> irq_enable flag is still set to false, resulting in continuous
>>> interrupts seen by CPU as DEASSERT_INTx cannot be sent since
>>> flag is still false and making CPU stall.
>>> -Changing the sequence of setting these irq flags.
>>>
>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>> ---
>>
>> This patch should be enhanced with the smb_xx() calls as suggested by by Lino.
>>
>> The subject should be changed. I would suggest something like "rtlwifi:
>> rtl8192ce: Prevent race condition when enabling interrupts", as it explains the
>> condition you are preventing.
>>
>> The other PCI drivers also have the same problem. Do you want to prepare the
>> patches, or should I do it?
>>
> Thanks Larry. Please send out the patches adding the above enhancements suggested by Lino.

I have prepared a patch fixing all the drivers. By the way, what CPU hardware 
showed this problem?

Larry

RE: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags

[Bharat Kumar Gogada]

> Subject: Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts
> after enabling local irq flags
> 
> On 01/20/2017 08:14 AM, Bharat Kumar Gogada wrote:
> >  > On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
> >>> -Realtek 8192CE chipset maintains local irq flags after
> >>> enabling/disabling hardware interrupts.
> >>> -Hardware interrupts are enabled before enabling the local irq
> >>> flags(these flags are being checked in interrupt handler), leading
> >>> to race condition on some RP, where the irq line between bridge and
> >>> GIC goes high at ASSERT_INTx and goes low only at DEASSERT_INTx. In
> >>> this kind of RP by the time ASSERT_INTx is seen irq_enable flag is
> >>> still set to false, resulting in continuous interrupts seen by CPU
> >>> as DEASSERT_INTx cannot be sent since flag is still false and making
> >>> CPU stall.
> >>> -Changing the sequence of setting these irq flags.
> >>>
> >>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>> ---
> >>
> >> This patch should be enhanced with the smb_xx() calls as suggested by by
> Lino.
> >>
> >> The subject should be changed. I would suggest something like "rtlwifi:
> >> rtl8192ce: Prevent race condition when enabling interrupts", as it
> >> explains the condition you are preventing.
> >>
> >> The other PCI drivers also have the same problem. Do you want to
> >> prepare the patches, or should I do it?
> >>
> > Thanks Larry. Please send out the patches adding the above enhancements
> suggested by Lino.
> 
> I have prepared a patch fixing all the drivers. By the way, what CPU hardware
> showed this problem?
> 
Thanks Larry, it was showed in ZynqUltrascalePlus Root Port hardware.