| Message ID | 20161113111705.GA2393@box64.home.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
Barry Day <briselec@gmail.com> writes: > The rtl8192e and rtl8723 fail to reconnect to an AP after being > disconnected. Ths patch fixes that without affecting the rtl8192cu. > I don't have a rtl8723 to test but it has been tested on a rtl8192eu. > After going through the orginal realtek code for the rtl8723, I am > confident the patch is applicable to both. > > Signed-off-by: Barry Day <briselec@gmail.com> > --- > rtl8xxxu_core.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) Hi Barry, Thank you for the patch. There are a couple of items which I am not 100% sure about the order of. > diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c > index 04141e5..6ac10d2 100644 > --- a/rtl8xxxu_core.c > +++ b/rtl8xxxu_core.c > @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv, > void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv, > u8 macid, bool connect) > { > + u8 val8; > struct h2c_cmd h2c; > > memset(&h2c, 0, sizeof(struct h2c_cmd)); > > h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT; > - if (connect) > + if (connect) { > h2c.media_status_rpt.parm |= BIT(0); > - else > - h2c.media_status_rpt.parm &= ~BIT(0); > + rtl8xxxu_gen2_h2c_cmd(priv, &h2c, > + sizeof(h2c.media_status_rpt)); > + } else { > + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL); > + val8 &= ~BEACON_FUNCTION_ENABLE; > + > + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00); > + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1))); > + } > > - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt)); > } This only affects 8192eu and not 8192cu - we left RXFLTMAP2 out of here on purpose for monitor mode, but you now disable it for 8192eu/8723bu. > void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) > @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > sgi = 1; > rcu_read_unlock(); > > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); > + > priv->fops->update_rate_mask(priv, ramask, sgi); > > rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); Here you enable RXFLTMAP2 for all devices - this doesn't balance with the above. I am not against the aproach, but I think we need to investigate a little further what is going on. Cheers, Jes
On Mon, Nov 14, 2016 at 08:24:45AM -0500, Jes Sorensen wrote: > Barry Day <briselec@gmail.com> writes: > > The rtl8192e and rtl8723 fail to reconnect to an AP after being > > disconnected. Ths patch fixes that without affecting the rtl8192cu. > > I don't have a rtl8723 to test but it has been tested on a rtl8192eu. > > After going through the orginal realtek code for the rtl8723, I am > > confident the patch is applicable to both. > > > > Signed-off-by: Barry Day <briselec@gmail.com> > > --- > > rtl8xxxu_core.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > Hi Barry, > > Thank you for the patch. There are a couple of items which I am not > 100% sure about the order of. > > > diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c > > index 04141e5..6ac10d2 100644 > > --- a/rtl8xxxu_core.c > > +++ b/rtl8xxxu_core.c > > @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv, > > void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv, > > u8 macid, bool connect) > > { > > + u8 val8; > > struct h2c_cmd h2c; > > > > memset(&h2c, 0, sizeof(struct h2c_cmd)); > > > > h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT; > > - if (connect) > > + if (connect) { > > h2c.media_status_rpt.parm |= BIT(0); > > - else > > - h2c.media_status_rpt.parm &= ~BIT(0); > > + rtl8xxxu_gen2_h2c_cmd(priv, &h2c, > > + sizeof(h2c.media_status_rpt)); > > + } else { > > + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL); > > + val8 &= ~BEACON_FUNCTION_ENABLE; > > + > > + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); > > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00); > > + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1))); > > + } > > > > - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt)); > > } > > This only affects 8192eu and not 8192cu - we left RXFLTMAP2 out of here > on purpose for monitor mode, but you now disable it for 8192eu/8723bu. > Even in monitor mode the interface has to brought up to use it which invokes rtl8xxxu_start which sets it back to accepting frames. > > void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) > > @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > > sgi = 1; > > rcu_read_unlock(); > > > > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); > > + > > priv->fops->update_rate_mask(priv, ramask, sgi); > > > > rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); > > Here you enable RXFLTMAP2 for all devices - this doesn't balance with > the above. > The original realtek code I have for the 8192cu does the disconnect the same way as in this patch. I tested the 8192cu using the patched gen2_report connect and it works. That would make things consistent across all chipsets.
Barry Day <briselec@gmail.com> writes: > On Mon, Nov 14, 2016 at 08:24:45AM -0500, Jes Sorensen wrote: >> Barry Day <briselec@gmail.com> writes: >> > The rtl8192e and rtl8723 fail to reconnect to an AP after being >> > disconnected. Ths patch fixes that without affecting the rtl8192cu. >> > I don't have a rtl8723 to test but it has been tested on a rtl8192eu. >> > After going through the orginal realtek code for the rtl8723, I am >> > confident the patch is applicable to both. >> > >> > Signed-off-by: Barry Day <briselec@gmail.com> >> > --- >> > rtl8xxxu_core.c | 18 ++++++++++++++---- >> > 1 file changed, 14 insertions(+), 4 deletions(-) >> >> Hi Barry, >> >> Thank you for the patch. There are a couple of items which I am not >> 100% sure about the order of. >> >> > diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c >> > index 04141e5..6ac10d2 100644 >> > --- a/rtl8xxxu_core.c >> > +++ b/rtl8xxxu_core.c >> > @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv, >> > void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv, >> > u8 macid, bool connect) >> > { >> > + u8 val8; >> > struct h2c_cmd h2c; >> > >> > memset(&h2c, 0, sizeof(struct h2c_cmd)); >> > >> > h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT; >> > - if (connect) >> > + if (connect) { >> > h2c.media_status_rpt.parm |= BIT(0); >> > - else >> > - h2c.media_status_rpt.parm &= ~BIT(0); >> > + rtl8xxxu_gen2_h2c_cmd(priv, &h2c, >> > + sizeof(h2c.media_status_rpt)); >> > + } else { >> > + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL); >> > + val8 &= ~BEACON_FUNCTION_ENABLE; >> > + >> > + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); >> > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00); >> > + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1))); >> > + } >> > >> > - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt)); >> > } >> >> This only affects 8192eu and not 8192cu - we left RXFLTMAP2 out of here >> on purpose for monitor mode, but you now disable it for 8192eu/8723bu. > > Even in monitor mode the interface has to brought up to use it which invokes > rtl8xxxu_start which sets it back to accepting frames. > > >> > void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) >> > @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, >> > sgi = 1; >> > rcu_read_unlock(); >> > >> > + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); >> > + >> > priv->fops->update_rate_mask(priv, ramask, sgi); >> > >> > rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); >> >> Here you enable RXFLTMAP2 for all devices - this doesn't balance with >> the above. > > The original realtek code I have for the 8192cu does the disconnect the > same way as in this patch. I tested the 8192cu using the patched > gen2_report connect and it works. That would make things consistent across > all chipsets. My concern is that you only set FLATMAP and BEACON_CTRL for gen2 devices, which has zero impact on gen1 devices, such as the 8192cu. If need to do this for gen2 (8723bu/8192eu) I assume we need to do it for gen1 as well. I'd like to see us do a little more investigating on this first. Jes
Jes Sorensen <Jes.Sorensen@redhat.com> writes: > Barry Day <briselec@gmail.com> writes: >> The rtl8192e and rtl8723 fail to reconnect to an AP after being >> disconnected. Ths patch fixes that without affecting the rtl8192cu. >> I don't have a rtl8723 to test but it has been tested on a rtl8192eu. >> After going through the orginal realtek code for the rtl8723, I am >> confident the patch is applicable to both. >> >> Signed-off-by: Barry Day <briselec@gmail.com> >> --- >> rtl8xxxu_core.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) > > Hi Barry, > > Thank you for the patch. There are a couple of items which I am not > 100% sure about the order of. > >> diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c >> index 04141e5..6ac10d2 100644 >> --- a/rtl8xxxu_core.c >> +++ b/rtl8xxxu_core.c >> @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv, >> void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv, >> u8 macid, bool connect) >> { >> + u8 val8; >> struct h2c_cmd h2c; >> >> memset(&h2c, 0, sizeof(struct h2c_cmd)); >> >> h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT; >> - if (connect) >> + if (connect) { >> h2c.media_status_rpt.parm |= BIT(0); >> - else >> - h2c.media_status_rpt.parm &= ~BIT(0); >> + rtl8xxxu_gen2_h2c_cmd(priv, &h2c, >> + sizeof(h2c.media_status_rpt)); >> + } else { >> + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL); >> + val8 &= ~BEACON_FUNCTION_ENABLE; >> + >> + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); >> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00); >> + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1))); >> + } >> >> - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt)); >> } Barry, So looking at this again, I am pretty sure you will break monitor mode with this. By setting REG_RXFLTMAP2 to 0x0000 you stop reception of all data frames. The other thing here is that you change removes the part notifying the firmware that we disconnected since you now only send the media_start_rpt command on connect, but not on disconnect. I am curious if the problem goes away if you simply add the BEACON_CTRL and REG_DUAL_TSF_RST parts? > > This only affects 8192eu and not 8192cu - we left RXFLTMAP2 out of here > on purpose for monitor mode, but you now disable it for 8192eu/8723bu. > >> void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) >> @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, >> sgi = 1; >> rcu_read_unlock(); >> >> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); >> + >> priv->fops->update_rate_mask(priv, ramask, sgi); >> >> rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); I believe this change only matters because you disable RXFLTMAP2 above. If we really have to write to RXFLTMAP2 to make this work, I suspect we need to keep some sort of state information. I would also be curious if RXFLTMAP2 gets reset somehow by the firmware, and we do not account for that. Cheers, Jes
On Mon, Nov 21, 2016 at 11:57:22AM -0500, Jes Sorensen wrote: > Jes Sorensen <Jes.Sorensen@redhat.com> writes: > > Barry Day <briselec@gmail.com> writes: > >> The rtl8192e and rtl8723 fail to reconnect to an AP after being > >> disconnected. Ths patch fixes that without affecting the rtl8192cu. > >> I don't have a rtl8723 to test but it has been tested on a rtl8192eu. > >> After going through the orginal realtek code for the rtl8723, I am > >> confident the patch is applicable to both. > >> > >> Signed-off-by: Barry Day <briselec@gmail.com> > >> --- > >> rtl8xxxu_core.c | 18 ++++++++++++++---- > >> 1 file changed, 14 insertions(+), 4 deletions(-) > > > > Hi Barry, > > > > Thank you for the patch. There are a couple of items which I am not > > 100% sure about the order of. > > > >> diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c > >> index 04141e5..6ac10d2 100644 > >> --- a/rtl8xxxu_core.c > >> +++ b/rtl8xxxu_core.c > >> @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv, > >> void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv, > >> u8 macid, bool connect) > >> { > >> + u8 val8; > >> struct h2c_cmd h2c; > >> > >> memset(&h2c, 0, sizeof(struct h2c_cmd)); > >> > >> h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT; > >> - if (connect) > >> + if (connect) { > >> h2c.media_status_rpt.parm |= BIT(0); > >> - else > >> - h2c.media_status_rpt.parm &= ~BIT(0); > >> + rtl8xxxu_gen2_h2c_cmd(priv, &h2c, > >> + sizeof(h2c.media_status_rpt)); > >> + } else { > >> + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL); > >> + val8 &= ~BEACON_FUNCTION_ENABLE; > >> + > >> + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); > >> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00); > >> + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1))); > >> + } > >> > >> - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt)); > >> } > > Barry, > > So looking at this again, I am pretty sure you will break monitor mode > with this. By setting REG_RXFLTMAP2 to 0x0000 you stop reception of all > data frames. > > The other thing here is that you change removes the part notifying the > firmware that we disconnected since you now only send the > media_start_rpt command on connect, but not on disconnect. > > I am curious if the problem goes away if you simply add the BEACON_CTRL > and REG_DUAL_TSF_RST parts? > > > > > This only affects 8192eu and not 8192cu - we left RXFLTMAP2 out of here > > on purpose for monitor mode, but you now disable it for 8192eu/8723bu. > > > >> void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) > >> @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > >> sgi = 1; > >> rcu_read_unlock(); > >> > >> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); > >> + > >> priv->fops->update_rate_mask(priv, ramask, sgi); > >> > >> rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); > > I believe this change only matters because you disable RXFLTMAP2 > above. If we really have to write to RXFLTMAP2 to make this work, I > suspect we need to keep some sort of state information. > > I would also be curious if RXFLTMAP2 gets reset somehow by the firmware, > and we do not account for that. > > Cheers, > Jes > I'll redo the patch without touching REG_RXFLTMAP2. I don't think it's needed to fix the fail to reconnect issue. I haven't had a proper look at the 8723 chips yet but the vendor drivers for the others don't do a h2c cmd for disconnect but I'll test leaving it in to see if it makes any difference. A 8723bu arrived in the mail today so now I can test it too and I discovered yesterday I have a 8723au but it's in a cheap Android tablet. Barry
Barry Day <briselec@gmail.com> writes: > On Mon, Nov 21, 2016 at 11:57:22AM -0500, Jes Sorensen wrote: >> Jes Sorensen <Jes.Sorensen@redhat.com> writes: >> >> void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) >> >> @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, >> >> sgi = 1; >> >> rcu_read_unlock(); >> >> >> >> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); >> >> + >> >> priv->fops->update_rate_mask(priv, ramask, sgi); >> >> >> >> rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff); >> >> I believe this change only matters because you disable RXFLTMAP2 >> above. If we really have to write to RXFLTMAP2 to make this work, I >> suspect we need to keep some sort of state information. >> >> I would also be curious if RXFLTMAP2 gets reset somehow by the firmware, >> and we do not account for that. >> >> Cheers, >> Jes > > I'll redo the patch without touching REG_RXFLTMAP2. I don't think it's needed > to fix the fail to reconnect issue. > > I haven't had a proper look at the 8723 chips yet but the vendor drivers for > the others don't do a h2c cmd for disconnect but I'll test leaving it in to see > if it makes any difference. A 8723bu arrived in the mail today so now I can > test it too and I discovered yesterday I have a 8723au but it's in a cheap > Android tablet. Let me know what you find out - if the h2c command causes the failure that would be very bizarre but certainly interesting to learn. Cheers, Jes
diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c index 04141e5..6ac10d2 100644 --- a/rtl8xxxu_core.c +++ b/rtl8xxxu_core.c @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv, void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv, u8 macid, bool connect) { + u8 val8; struct h2c_cmd h2c; memset(&h2c, 0, sizeof(struct h2c_cmd)); h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT; - if (connect) + if (connect) { h2c.media_status_rpt.parm |= BIT(0); - else - h2c.media_status_rpt.parm &= ~BIT(0); + rtl8xxxu_gen2_h2c_cmd(priv, &h2c, + sizeof(h2c.media_status_rpt)); + } else { + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL); + val8 &= ~BEACON_FUNCTION_ENABLE; + + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8); + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00); + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1))); + } - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt)); } void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv) @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif, sgi = 1; rcu_read_unlock(); + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff); + priv->fops->update_rate_mask(priv, ramask, sgi); rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff);
The rtl8192e and rtl8723 fail to reconnect to an AP after being disconnected. Ths patch fixes that without affecting the rtl8192cu. I don't have a rtl8723 to test but it has been tested on a rtl8192eu. After going through the orginal realtek code for the rtl8723, I am confident the patch is applicable to both. Signed-off-by: Barry Day <briselec@gmail.com> --- rtl8xxxu_core.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)