[PATCH] wl12xx: Unset bssid filter, ssid and bssid from firmware on disassoc

[PATCH] wl12xx: Unset bssid filter, ssid and bssid from firmware on disassoc

[juuso.oikarinen]

From: Juuso Oikarinen <juuso.oikarinen@nokia.com>

On the disassociation event from the mac80211, the wl12xx driver does not
clear the chipset configuration related to the AP - i.e. it does not perform
a DISCONNECT and then a JOIN with zero SSID and dummy BSSID. Also, it does not
unset the BSSID filter.

Often this is not a problem, as the above is performed upon entering idle
state. But if a scenario arises where a new association is attempted without
cycling through idle state, the new association will fail.

Fix this by resetting the firmware state on disassociation.

Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
---
 drivers/net/wireless/wl12xx/main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 31f0e2f..c523778 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -2012,6 +2012,10 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
 			/* Disable the keep-alive feature */
 			ret = wl1271_acx_keep_alive_mode(wl, false);
 
+			/* restore the bssid filter and go to dummy bssid */
+			wl1271_handle_idle(wl, true);
+			wl1271_handle_idle(wl, false);
+
 			if (ret < 0)
 				goto out_sleep;
 		}
-- 
1.7.1

Re: [PATCH] wl12xx: Unset bssid filter, ssid and bssid from firmware on disassoc

[Luciano Coelho]

On Wed, 2010-11-17 at 15:29 +0200, juuso.oikarinen@nokia.com wrote:
> From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> 
> On the disassociation event from the mac80211, the wl12xx driver does not
> clear the chipset configuration related to the AP - i.e. it does not perform
> a DISCONNECT and then a JOIN with zero SSID and dummy BSSID. Also, it does not
> unset the BSSID filter.
> 
> Often this is not a problem, as the above is performed upon entering idle
> state. But if a scenario arises where a new association is attempted without
> cycling through idle state, the new association will fail.
> 
> Fix this by resetting the firmware state on disassociation.
> 
> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> ---
>  drivers/net/wireless/wl12xx/main.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 31f0e2f..c523778 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -2012,6 +2012,10 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
>  			/* Disable the keep-alive feature */
>  			ret = wl1271_acx_keep_alive_mode(wl, false);
>  
> +			/* restore the bssid filter and go to dummy bssid */
> +			wl1271_handle_idle(wl, true);
> +			wl1271_handle_idle(wl, false);
> +

Hmmm... This looks quite hacky.  Can 't you create functions with that
"restore the bssid" and "go to dummy bssid" and call them instead? If
you do so, you don't have to artificially go to idle and leave idle
immediately...

-- 
Cheers,
Luca.

Re: [PATCH] wl12xx: Unset bssid filter, ssid and bssid from firmware on disassoc

[Juuso Oikarinen]

On Wed, 2010-11-17 at 17:41 +0200, Luciano Coelho wrote:
> On Wed, 2010-11-17 at 15:29 +0200, juuso.oikarinen@nokia.com wrote:
> > From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > 
> > On the disassociation event from the mac80211, the wl12xx driver does not
> > clear the chipset configuration related to the AP - i.e. it does not perform
> > a DISCONNECT and then a JOIN with zero SSID and dummy BSSID. Also, it does not
> > unset the BSSID filter.
> > 
> > Often this is not a problem, as the above is performed upon entering idle
> > state. But if a scenario arises where a new association is attempted without
> > cycling through idle state, the new association will fail.
> > 
> > Fix this by resetting the firmware state on disassociation.
> > 
> > Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > ---
> >  drivers/net/wireless/wl12xx/main.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > index 31f0e2f..c523778 100644
> > --- a/drivers/net/wireless/wl12xx/main.c
> > +++ b/drivers/net/wireless/wl12xx/main.c
> > @@ -2012,6 +2012,10 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
> >  			/* Disable the keep-alive feature */
> >  			ret = wl1271_acx_keep_alive_mode(wl, false);
> >  
> > +			/* restore the bssid filter and go to dummy bssid */
> > +			wl1271_handle_idle(wl, true);
> > +			wl1271_handle_idle(wl, false);
> > +
> 
> Hmmm... This looks quite hacky.  Can 't you create functions with that
> "restore the bssid" and "go to dummy bssid" and call them instead? If
> you do so, you don't have to artificially go to idle and leave idle
> immediately...
> 

We can, but their content will be the same. Here we are not
"artificially" entering/leaving idle, we're just performing the same
steps towards the firmware as we would on idle entry and idle exit.

If you like, we can rename the handle_idle function to something more
generic.

-Juuso

Re: [PATCH] wl12xx: Unset bssid filter, ssid and bssid from firmware on disassoc

[Luciano Coelho]

On Thu, 2010-11-18 at 07:27 +0200, Juuso Oikarinen wrote:
> On Wed, 2010-11-17 at 17:41 +0200, Luciano Coelho wrote:
> > On Wed, 2010-11-17 at 15:29 +0200, juuso.oikarinen@nokia.com wrote:
> > > From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > > 
> > > On the disassociation event from the mac80211, the wl12xx driver does not
> > > clear the chipset configuration related to the AP - i.e. it does not perform
> > > a DISCONNECT and then a JOIN with zero SSID and dummy BSSID. Also, it does not
> > > unset the BSSID filter.
> > > 
> > > Often this is not a problem, as the above is performed upon entering idle
> > > state. But if a scenario arises where a new association is attempted without
> > > cycling through idle state, the new association will fail.
> > > 
> > > Fix this by resetting the firmware state on disassociation.
> > > 
> > > Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > > ---
> > >  drivers/net/wireless/wl12xx/main.c |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > > index 31f0e2f..c523778 100644
> > > --- a/drivers/net/wireless/wl12xx/main.c
> > > +++ b/drivers/net/wireless/wl12xx/main.c
> > > @@ -2012,6 +2012,10 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
> > >  			/* Disable the keep-alive feature */
> > >  			ret = wl1271_acx_keep_alive_mode(wl, false);
> > >  
> > > +			/* restore the bssid filter and go to dummy bssid */
> > > +			wl1271_handle_idle(wl, true);
> > > +			wl1271_handle_idle(wl, false);
> > > +
> > 
> > Hmmm... This looks quite hacky.  Can 't you create functions with that
> > "restore the bssid" and "go to dummy bssid" and call them instead? If
> > you do so, you don't have to artificially go to idle and leave idle
> > immediately...
> > 
> 
> We can, but their content will be the same. Here we are not
> "artificially" entering/leaving idle, we're just performing the same
> steps towards the firmware as we would on idle entry and idle exit.
> 
> If you like, we can rename the handle_idle function to something more
> generic.

No, I meant that you could create functions with names that mirror the
things you want to do and call them here *and* in wl1271_handle_idle(),
so you won't have duplicate code and it will be quite clear what you
want to do here.  What I mean by artificially entering idle is that in
the wl1271_handle_idle() function you even set the IDLE bit and so on.
In this case you don't need that.

This is really a small thing, so I won't fight much for it, but I think
it could be clearer...

-- 
Cheers,
Luca.

Re: [PATCH] wl12xx: Unset bssid filter, ssid and bssid from firmware on disassoc

[Juuso Oikarinen]

On Thu, 2010-11-18 at 08:14 +0200, Luciano Coelho wrote:
> On Thu, 2010-11-18 at 07:27 +0200, Juuso Oikarinen wrote:
> > On Wed, 2010-11-17 at 17:41 +0200, Luciano Coelho wrote:
> > > On Wed, 2010-11-17 at 15:29 +0200, juuso.oikarinen@nokia.com wrote:
> > > > From: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > > > 
> > > > On the disassociation event from the mac80211, the wl12xx driver does not
> > > > clear the chipset configuration related to the AP - i.e. it does not perform
> > > > a DISCONNECT and then a JOIN with zero SSID and dummy BSSID. Also, it does not
> > > > unset the BSSID filter.
> > > > 
> > > > Often this is not a problem, as the above is performed upon entering idle
> > > > state. But if a scenario arises where a new association is attempted without
> > > > cycling through idle state, the new association will fail.
> > > > 
> > > > Fix this by resetting the firmware state on disassociation.
> > > > 
> > > > Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > > > ---
> > > >  drivers/net/wireless/wl12xx/main.c |    4 ++++
> > > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > > > index 31f0e2f..c523778 100644
> > > > --- a/drivers/net/wireless/wl12xx/main.c
> > > > +++ b/drivers/net/wireless/wl12xx/main.c
> > > > @@ -2012,6 +2012,10 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
> > > >  			/* Disable the keep-alive feature */
> > > >  			ret = wl1271_acx_keep_alive_mode(wl, false);
> > > >  
> > > > +			/* restore the bssid filter and go to dummy bssid */
> > > > +			wl1271_handle_idle(wl, true);
> > > > +			wl1271_handle_idle(wl, false);
> > > > +
> > > 
> > > Hmmm... This looks quite hacky.  Can 't you create functions with that
> > > "restore the bssid" and "go to dummy bssid" and call them instead? If
> > > you do so, you don't have to artificially go to idle and leave idle
> > > immediately...
> > > 
> > 
> > We can, but their content will be the same. Here we are not
> > "artificially" entering/leaving idle, we're just performing the same
> > steps towards the firmware as we would on idle entry and idle exit.
> > 
> > If you like, we can rename the handle_idle function to something more
> > generic.
> 
> No, I meant that you could create functions with names that mirror the
> things you want to do and call them here *and* in wl1271_handle_idle(),
> so you won't have duplicate code and it will be quite clear what you
> want to do here.  What I mean by artificially entering idle is that in
> the wl1271_handle_idle() function you even set the IDLE bit and so on.
> In this case you don't need that.
> 
> This is really a small thing, so I won't fight much for it, but I think
> it could be clearer...
> 

Ok. But in my book it essentially means renaming the current handle_idle
function to something more generic, because the content (with maybe the
exception of the IDLE bit) will be what it's now.

I'll submit v2 shortly.

-Juuso