[PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Bruno Randolf]

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

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);
-- 
1.9.1

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Jes Sorensen]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Bruno Randolf]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Jes Sorensen]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Bruno Randolf]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Jes Sorensen]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Bruno Randolf]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Jes Sorensen]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Larry Finger]

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

Re: [PATCH] rtl8xxxu: Enable data frame reception in rtl8xxxu_start

[Jes Sorensen]

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