| Message ID | 1484820854-16719-1-git-send-email-bharatku@xilinx.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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
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
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/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
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
> 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.
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)
-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(-)