| Message ID | 1445462932-23679-1-git-send-email-br1@einfach.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
Bruno Randolf <br1@einfach.org> writes: > mac80211 documentation says, the ieee80211_ops.start callback "must turn on > frame reception (for possibly enabled monitor interfaces.)". If not a single > monitor interface does not receive data frames. > > Similarly we should not change the data reception based on the association > state. > > Signed-off-by: Bruno Randolf <br1@einfach.org> > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) Bruno, Thanks - I am not 100% convinced about this one. I don't think we should tell the firmware to pass up data frames before we have negotiated the connection. It's true that for monitor mode, we need to enable it if all packets are requested. Looking at iw there is an option where it only requests control packets, and one for all, etc. However for non monitor mode, we shouldn't pass all data packets up to the stack, resulting and have mac80211 parse them all. Cheers, Jes > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c > index 0399b1e..ae490e7 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c > @@ -4475,9 +4475,6 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > > rtl8xxxu_update_rate_mask(priv, ramask, sgi); > > - /* Enable RX of data frames */ > - rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); > - > rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); > > rtl8723a_stop_tx_beacon(priv); > @@ -4492,8 +4489,6 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > val8 |= BEACON_DISABLE_TSF_UPDATE; > rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); > > - /* Disable RX of data frames */ > - rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x0000); > h2c.joinbss.data = H2C_JOIN_BSS_DISCONNECT; > } > h2c.joinbss.cmd = H2C_JOIN_BSS_REPORT; > @@ -5495,12 +5490,9 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw) > } > exit: > /* > - * Disable all data frames > - */ > - rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x0000); > - /* > - * Accept all mgmt frames > + * Accept all data and mgmt frames > */ > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); > rtl8xxxu_write16(priv, REG_RXFLTMAP0, 0xffff); > > rtl8xxxu_write32(priv, REG_OFDM0_XA_AGC_CORE1, 0x6954341e); -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/22/2015 12:13 AM, Jes Sorensen wrote: > Bruno Randolf <br1@einfach.org> writes: >> mac80211 documentation says, the ieee80211_ops.start callback "must turn on >> frame reception (for possibly enabled monitor interfaces.)". If not a single >> monitor interface does not receive data frames. >> >> Similarly we should not change the data reception based on the association >> state. >> >> Signed-off-by: Bruno Randolf <br1@einfach.org> >> --- >> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 12 ++---------- >> 1 file changed, 2 insertions(+), 10 deletions(-) > > Bruno, > > Thanks - I am not 100% convinced about this one. I don't think we should > tell the firmware to pass up data frames before we have negotiated the > connection. > > It's true that for monitor mode, we need to enable it if all packets > are requested. Looking at iw there is an option where it only requests > control packets, and one for all, etc. However for non monitor mode, we > shouldn't pass all data packets up to the stack, resulting and have > mac80211 parse them all. But mac80211 requests us to do so - please see include/net/mac80211.h line 2576 or https://www.kernel.org/doc/htmldocs/80211/API-struct-ieee80211-ops.html I know you are focusing on STA mode at the moment, but enabling/disabling data reception on association is not correct for most other modes. Also don't be afraid of too many frames being passed. In the initial setting (without a monitor interface) the RCR RCR_ACCEPT_AP bit is not set and the RCR_CHECK_BSSID_* bits are set as well. bruno -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bruno Randolf <br1@thinktube.com> writes: > On 10/22/2015 12:13 AM, Jes Sorensen wrote: >> Bruno Randolf <br1@einfach.org> writes: >>> mac80211 documentation says, the ieee80211_ops.start callback "must turn on >>> frame reception (for possibly enabled monitor interfaces.)". If not a single >>> monitor interface does not receive data frames. >>> >>> Similarly we should not change the data reception based on the association >>> state. >>> >>> Signed-off-by: Bruno Randolf <br1@einfach.org> >>> --- >>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 12 ++---------- >>> 1 file changed, 2 insertions(+), 10 deletions(-) >> >> Bruno, >> >> Thanks - I am not 100% convinced about this one. I don't think we should >> tell the firmware to pass up data frames before we have negotiated the >> connection. >> >> It's true that for monitor mode, we need to enable it if all packets >> are requested. Looking at iw there is an option where it only requests >> control packets, and one for all, etc. However for non monitor mode, we >> shouldn't pass all data packets up to the stack, resulting and have >> mac80211 parse them all. > > But mac80211 requests us to do so - please see include/net/mac80211.h > line 2576 or > https://www.kernel.org/doc/htmldocs/80211/API-struct-ieee80211-ops.html > > I know you are focusing on STA mode at the moment, but > enabling/disabling data reception on association is not correct for most > other modes. > > Also don't be afraid of too many frames being passed. In the initial > setting (without a monitor interface) the RCR RCR_ACCEPT_AP bit is not > set and the RCR_CHECK_BSSID_* bits are set as well. I realize the different modes require different behavior, and we obviously need to deal with this. However we shouldn't downgrade STA mode in order to be able to handle other modes. Passing too many frames unncessarily is bad, it adds unnecessary load to the USB bus as well as the stack. Remember that mac80211 is designed to handle completely dumb devices too, where it needs to process everything. So I am not against making changes, I just want them done right. Cheers, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/22/2015 02:10 PM, Jes Sorensen wrote: >>> Thanks - I am not 100% convinced about this one. I don't think we should >>> tell the firmware to pass up data frames before we have negotiated the >>> connection. >>> >>> It's true that for monitor mode, we need to enable it if all packets >>> are requested. Looking at iw there is an option where it only requests >>> control packets, and one for all, etc. However for non monitor mode, we >>> shouldn't pass all data packets up to the stack, resulting and have >>> mac80211 parse them all. >> >> But mac80211 requests us to do so - please see include/net/mac80211.h >> line 2576 or >> https://www.kernel.org/doc/htmldocs/80211/API-struct-ieee80211-ops.html >> >> I know you are focusing on STA mode at the moment, but >> enabling/disabling data reception on association is not correct for most >> other modes. >> >> Also don't be afraid of too many frames being passed. In the initial >> setting (without a monitor interface) the RCR RCR_ACCEPT_AP bit is not >> set and the RCR_CHECK_BSSID_* bits are set as well. > > I realize the different modes require different behavior, and we > obviously need to deal with this. However we shouldn't downgrade STA > mode in order to be able to handle other modes. Passing too many frames > unncessarily is bad, it adds unnecessary load to the USB bus as well as > the stack. > > Remember that mac80211 is designed to handle completely dumb devices > too, where it needs to process everything. > > So I am not against making changes, I just want them done right. Well, I'd say that for a mac80211 driver the right thing to do is to follow the documented specifications of mac80211. Also I don't see how this would downgrade STA mode, and I explained to you above how in STA mode that does not pass more data frames to mac80211 than before. BTW that also fixes the "endless scanning" problem I reported to you before, and makes the driver work with the slightly older mac80211 version of OpenWRT 15.05. Care to explain what's not right with this? But maybe a more credible mac80211 developer can can comment? bruno -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bruno Randolf <br1@einfach.org> writes: > On 10/22/2015 02:10 PM, Jes Sorensen wrote: >>>> Thanks - I am not 100% convinced about this one. I don't think we should >>>> tell the firmware to pass up data frames before we have negotiated the >>>> connection. >>>> >>>> It's true that for monitor mode, we need to enable it if all packets >>>> are requested. Looking at iw there is an option where it only requests >>>> control packets, and one for all, etc. However for non monitor mode, we >>>> shouldn't pass all data packets up to the stack, resulting and have >>>> mac80211 parse them all. >>> >>> But mac80211 requests us to do so - please see include/net/mac80211.h >>> line 2576 or >>> https://www.kernel.org/doc/htmldocs/80211/API-struct-ieee80211-ops.html >>> >>> I know you are focusing on STA mode at the moment, but >>> enabling/disabling data reception on association is not correct for most >>> other modes. >>> >>> Also don't be afraid of too many frames being passed. In the initial >>> setting (without a monitor interface) the RCR RCR_ACCEPT_AP bit is not >>> set and the RCR_CHECK_BSSID_* bits are set as well. >> >> I realize the different modes require different behavior, and we >> obviously need to deal with this. However we shouldn't downgrade STA >> mode in order to be able to handle other modes. Passing too many frames >> unncessarily is bad, it adds unnecessary load to the USB bus as well as >> the stack. >> >> Remember that mac80211 is designed to handle completely dumb devices >> too, where it needs to process everything. >> >> So I am not against making changes, I just want them done right. > > Well, I'd say that for a mac80211 driver the right thing to do is to > follow the documented specifications of mac80211. Also I don't see how > this would downgrade STA mode, and I explained to you above how in STA > mode that does not pass more data frames to mac80211 than before. As I said, I don't mind changing things, but passing frames unnecessarily is to downgrade the STA mode driver's performance. If mac80211 requires this for some reason, then I agree, make the change, otherwise lets find a solution that works for both modes. > BTW that also fixes the "endless scanning" problem I reported to you > before, and makes the driver work with the slightly older mac80211 > version of OpenWRT 15.05. Care to explain what's not right with this? > > But maybe a more credible mac80211 developer can can comment? Making things work against random kernel versions of OpenWRT is not particularly "credible". If the driver needs hacks to work with older kernels, then they should be applied for the older kernels. Linux does not have a history of carrying hacks in current drivers to accommodate older kernels. I am not sure what endless scanning problem you are talking about? I know you had issues with duplicate MAC addresses due to NetworkMangler, but I do not remember issues with endless scanning. Basically we should only be receiving mgmt frames until we are connected, while in STA mode. It reduces power draing and load on the USB bus. In monitor mode we obviously want to receive them all - Johannes Berg just reminded me of @IEEE80211_CONF_MONITOR. We may be able to use this to determine when to enable things early, and when to do it on association. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/22/2015 02:42 PM, Jes Sorensen wrote: > Making things work against random kernel versions of OpenWRT is not > particularly "credible". If the driver needs hacks to work with older > kernels, then they should be applied for the older kernels. Linux does > not have a history of carrying hacks in current drivers to accommodate > older kernels. Well forget OpenWRT. The point is that this is not a hack - it's the behavior that mac80211 expects and documents and moreover it does NOT pass more data frames than before. It does not pass more data frames before association because the RCR RCR_ACCEPT_AP bit is not set and the RCR_CHECK_BSSID_* bits are set. bruno -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/23/2015 03:31 PM, Jes Sorensen wrote: > Bruno Randolf <br1@einfach.org> writes: >> On 10/22/2015 02:42 PM, Jes Sorensen wrote: >>> Making things work against random kernel versions of OpenWRT is not >>> particularly "credible". If the driver needs hacks to work with older >>> kernels, then they should be applied for the older kernels. Linux does >>> not have a history of carrying hacks in current drivers to accommodate >>> older kernels. >> >> Well forget OpenWRT. The point is that this is not a hack - it's the >> behavior that mac80211 expects and documents and moreover it does NOT >> pass more data frames than before. >> >> It does not pass more data frames before association because the RCR >> RCR_ACCEPT_AP bit is not set and the RCR_CHECK_BSSID_* bits are set. > > I have gone back and looked more at the vendor drivers over this issue. > Most versions of the vendor driver consistently mess with both FLTMAP* > and RCR, and disable packets when they go into a non-assoc state, but > non-assoc to the vendor driver also means non-AP, non-monitor, etc. > > I really would like to know more about the difference between RCR and > FLTMAP registers - Larry is there a chance you could try and ping your > Realtek contacts about this? I will try, but please be aware that the USB group generally tends to ignore me. Perhaps the PCI group can help. Larry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bruno Randolf <br1@einfach.org> writes: > On 10/22/2015 02:42 PM, Jes Sorensen wrote: >> Making things work against random kernel versions of OpenWRT is not >> particularly "credible". If the driver needs hacks to work with older >> kernels, then they should be applied for the older kernels. Linux does >> not have a history of carrying hacks in current drivers to accommodate >> older kernels. > > Well forget OpenWRT. The point is that this is not a hack - it's the > behavior that mac80211 expects and documents and moreover it does NOT > pass more data frames than before. > > It does not pass more data frames before association because the RCR > RCR_ACCEPT_AP bit is not set and the RCR_CHECK_BSSID_* bits are set. I have gone back and looked more at the vendor drivers over this issue. Most versions of the vendor driver consistently mess with both FLTMAP* and RCR, and disable packets when they go into a non-assoc state, but non-assoc to the vendor driver also means non-AP, non-monitor, etc. I really would like to know more about the difference between RCR and FLTMAP registers - Larry is there a chance you could try and ping your Realtek contacts about this? For now I have applied your patch and pushed it into both branches. Cheers, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Larry Finger <Larry.Finger@lwfinger.net> writes: > On 10/23/2015 03:31 PM, Jes Sorensen wrote: >> Bruno Randolf <br1@einfach.org> writes: >>> On 10/22/2015 02:42 PM, Jes Sorensen wrote: >>>> Making things work against random kernel versions of OpenWRT is not >>>> particularly "credible". If the driver needs hacks to work with older >>>> kernels, then they should be applied for the older kernels. Linux does >>>> not have a history of carrying hacks in current drivers to accommodate >>>> older kernels. >>> >>> Well forget OpenWRT. The point is that this is not a hack - it's the >>> behavior that mac80211 expects and documents and moreover it does NOT >>> pass more data frames than before. >>> >>> It does not pass more data frames before association because the RCR >>> RCR_ACCEPT_AP bit is not set and the RCR_CHECK_BSSID_* bits are set. >> >> I have gone back and looked more at the vendor drivers over this issue. >> Most versions of the vendor driver consistently mess with both FLTMAP* >> and RCR, and disable packets when they go into a non-assoc state, but >> non-assoc to the vendor driver also means non-AP, non-monitor, etc. >> >> I really would like to know more about the difference between RCR and >> FLTMAP registers - Larry is there a chance you could try and ping your >> Realtek contacts about this? > > I will try, but please be aware that the USB group generally tends to > ignore me. Perhaps the PCI group can help. Larry, Much appreciated - I believe the FLTMAP* and RCR registers are the same on the PCI and USB chips, so any answer should be useful. Cheers, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c index 0399b1e..ae490e7 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c @@ -4475,9 +4475,6 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, rtl8xxxu_update_rate_mask(priv, ramask, sgi); - /* Enable RX of data frames */ - rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); - rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); rtl8723a_stop_tx_beacon(priv); @@ -4492,8 +4489,6 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, val8 |= BEACON_DISABLE_TSF_UPDATE; rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); - /* Disable RX of data frames */ - rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x0000); h2c.joinbss.data = H2C_JOIN_BSS_DISCONNECT; } h2c.joinbss.cmd = H2C_JOIN_BSS_REPORT; @@ -5495,12 +5490,9 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw) } exit: /* - * Disable all data frames - */ - rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x0000); - /* - * Accept all mgmt frames + * Accept all data and mgmt frames */ + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); rtl8xxxu_write16(priv, REG_RXFLTMAP0, 0xffff); rtl8xxxu_write32(priv, REG_OFDM0_XA_AGC_CORE1, 0x6954341e);
mac80211 documentation says, the ieee80211_ops.start callback "must turn on frame reception (for possibly enabled monitor interfaces.)". If not a single monitor interface does not receive data frames. Similarly we should not change the data reception based on the association state. Signed-off-by: Bruno Randolf <br1@einfach.org> --- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)