-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 <[email protected]>
---
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
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 <[email protected]>
> ---
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
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
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
> Subject: Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware i=
nterrupts
> after enabling local irq flags
>=20
> 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 <[email protected]>
> >>> ---
> >>
> >> 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.
>=20
> I have prepared a patch fixing all the drivers. By the way, what CPU hard=
ware
> showed this problem?
>=20
Thanks Larry, it was showed in ZynqUltrascalePlus Root Port hardware.
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 <[email protected]>
>>> ---
>>
>> 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
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
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
> On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
> > -Realtek 8192CE chipset maintains local irq flags after enabling/disabl=
ing
> > 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 <[email protected]>
> > ---
>=20
> This patch should be enhanced with the smb_xx() calls as suggested by by =
Lino.
>=20
> The subject should be changed. I would suggest something like "rtlwifi:
> rtl8192ce: Prevent race condition when enabling interrupts", as it explai=
ns the
> condition you are preventing.
>=20
> The other PCI drivers also have the same problem. Do you want to prepare =
the
> patches, or should I do it?
>=20
Thanks Larry. Please send out the patches adding the above enhancements sug=
gested by Lino.
Bharat