[RFC 0/2] mac80211 offchannel fixes

[RFC 0/2] mac80211 offchannel fixes

[Eliad Peller]

I was working on some roaming scenarios, and encountered some pretty
fundamental offchannel issues. i'm not familiar enough with this
code, so there's a good chance the patches are all wrong.

I'd be glad for your review.
Eliad.

Eliad Peller (2):
  mac80211: fix remain_off_channel regression
  mac80211: config hw when going back on-channel

 net/mac80211/work.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

[RFC 1/2] mac80211: fix remain_off_channel regression

[Eliad Peller]

i'm not familiar enough with the off_channel flow,
but this one looks completely broken - we should
remain_off_channel if the work was started, and
the work's channel and channel_type are the same
as local->tmp_channel and local->tmp_channel_type.

however, if wk->chan_type and local->tmp_channel_type
coexist (e.g. have the same channel type), we won't
remain_off_channel.

this behavior was introduced by commit da2fd1f
("mac80211: Allow work items to use existing
channel type.")

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 net/mac80211/work.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/work.c b/net/mac80211/work.c
index a94b312..3291958 100644
--- a/net/mac80211/work.c
+++ b/net/mac80211/work.c
@@ -1068,8 +1068,8 @@ static void ieee80211_work_work(struct work_struct *work)
 			continue;
 		if (wk->chan != local->tmp_channel)
 			continue;
-		if (ieee80211_work_ct_coexists(wk->chan_type,
-					       local->tmp_channel_type))
+		if (!ieee80211_work_ct_coexists(wk->chan_type,
+						local->tmp_channel_type))
 			continue;
 		remain_off_channel = true;
 	}
-- 
1.7.0.4

[RFC 2/2] mac80211: config hw when going back on-channel

[Eliad Peller]

The hw is currently not configured when going
back on-channel.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 net/mac80211/work.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/work.c b/net/mac80211/work.c
index 3291958..e6abb15 100644
--- a/net/mac80211/work.c
+++ b/net/mac80211/work.c
@@ -1075,7 +1075,6 @@ static void ieee80211_work_work(struct work_struct *work)
 	}
 
 	if (!remain_off_channel && local->tmp_channel) {
-		bool on_oper_chan = ieee80211_cfg_on_oper_channel(local);
 		local->tmp_channel = NULL;
 		/* If tmp_channel wasn't operating channel, then
 		 * we need to go back on-channel.
@@ -1085,7 +1084,7 @@ static void ieee80211_work_work(struct work_struct *work)
 		 * we still need to do a hardware config.  Currently,
 		 * we cannot be here while scanning, however.
 		 */
-		if (ieee80211_cfg_on_oper_channel(local) && !on_oper_chan)
+		if (!ieee80211_cfg_on_oper_channel(local))
 			ieee80211_hw_config(local, 0);
 
 		/* At the least, we need to disable offchannel_ps,
-- 
1.7.0.4

Re: [RFC 1/2] mac80211: fix remain_off_channel regression

[Ben Greear]

On 07/25/2011 08:29 AM, Eliad Peller wrote:
> i'm not familiar enough with the off_channel flow,
> but this one looks completely broken - we should
> remain_off_channel if the work was started, and
> the work's channel and channel_type are the same
> as local->tmp_channel and local->tmp_channel_type.
>
> however, if wk->chan_type and local->tmp_channel_type
> coexist (e.g. have the same channel type), we won't
> remain_off_channel.
>
> this behavior was introduced by commit da2fd1f
> ("mac80211: Allow work items to use existing
> channel type.")

This patch looks correct to me.

Thanks,
Ben

>
> Signed-off-by: Eliad Peller<eliad@wizery.com>
> ---
>   net/mac80211/work.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> index a94b312..3291958 100644
> --- a/net/mac80211/work.c
> +++ b/net/mac80211/work.c
> @@ -1068,8 +1068,8 @@ static void ieee80211_work_work(struct work_struct *work)
>   			continue;
>   		if (wk->chan != local->tmp_channel)
>   			continue;
> -		if (ieee80211_work_ct_coexists(wk->chan_type,
> -					       local->tmp_channel_type))
> +		if (!ieee80211_work_ct_coexists(wk->chan_type,
> +						local->tmp_channel_type))
>   			continue;
>   		remain_off_channel = true;
>   	}


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Ben Greear]

On 07/25/2011 08:29 AM, Eliad Peller wrote:
> The hw is currently not configured when going
> back on-channel.

I am less sure about this patch.  With the existing code,
I think it should catch going from on channel to off
and do the hw config properly.

With your change it will also reconfig the hardware, but it will
reconfig even if we were already on-channel (if, for instance,
local->tmp_channel is oper-channel), right?

Can you please explain in more detail how this code is
broken?

Thanks,
Ben

>
> Signed-off-by: Eliad Peller<eliad@wizery.com>
> ---
>   net/mac80211/work.c |    3 +--
>   1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> index 3291958..e6abb15 100644
> --- a/net/mac80211/work.c
> +++ b/net/mac80211/work.c
> @@ -1075,7 +1075,6 @@ static void ieee80211_work_work(struct work_struct *work)
>   	}
>
>   	if (!remain_off_channel&&  local->tmp_channel) {
> -		bool on_oper_chan = ieee80211_cfg_on_oper_channel(local);
>   		local->tmp_channel = NULL;
>   		/* If tmp_channel wasn't operating channel, then
>   		 * we need to go back on-channel.
> @@ -1085,7 +1084,7 @@ static void ieee80211_work_work(struct work_struct *work)
>   		 * we still need to do a hardware config.  Currently,
>   		 * we cannot be here while scanning, however.
>   		 */
> -		if (ieee80211_cfg_on_oper_channel(local)&&  !on_oper_chan)
> +		if (!ieee80211_cfg_on_oper_channel(local))
>   			ieee80211_hw_config(local, 0);
>
>   		/* At the least, we need to disable offchannel_ps,


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Eliad Peller]

hi Ben,

On Mon, Jul 25, 2011 at 8:18 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>
>> The hw is currently not configured when going
>> back on-channel.
>
> I am less sure about this patch.  With the existing code,
> I think it should catch going from on channel to off
> and do the hw config properly.
>
IIUC, this code is responsible for going back on-channel (if there is
no started work on the tmp_channel).

> With your change it will also reconfig the hardware, but it will
> reconfig even if we were already on-channel (if, for instance,
> local->tmp_channel is oper-channel), right?
>
> Can you please explain in more detail how this code is
> broken?
>
we should reconfigure the hardware iff the hardware is not configured
to the operational channel.
the current code doesn't handle it (e.g. oper_channel=1,
tmp_channel=11, hw_channel=11. since
ieee80211_cfg_on_oper_channel(local) == 0, the hw won't go back
on-channel).

i don't think it will reconfig if we are already on-channel, as in
this case oper_channel == hw_channel.

thanks for your review!
Eliad.

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Ben Greear]

On 07/25/2011 12:16 PM, Eliad Peller wrote:
> hi Ben,
>
> On Mon, Jul 25, 2011 at 8:18 PM, Ben Greear<greearb@candelatech.com>  wrote:
>> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>>
>>> The hw is currently not configured when going
>>> back on-channel.
>>
>> I am less sure about this patch.  With the existing code,
>> I think it should catch going from on channel to off
>> and do the hw config properly.
>>
> IIUC, this code is responsible for going back on-channel (if there is
> no started work on the tmp_channel).
>
>> With your change it will also reconfig the hardware, but it will
>> reconfig even if we were already on-channel (if, for instance,
>> local->tmp_channel is oper-channel), right?
>>
>> Can you please explain in more detail how this code is
>> broken?
>>
> we should reconfigure the hardware iff the hardware is not configured
> to the operational channel.
> the current code doesn't handle it (e.g. oper_channel=1,
> tmp_channel=11, hw_channel=11. since
> ieee80211_cfg_on_oper_channel(local) == 0, the hw won't go back
> on-channel).

If we are off-channel when entering that block of code, then tmp_channel
!= NULL, and on_oper_chan will be false.

Then, we set tmp_channel to NULL, which should make ieee80211_cfg_on_oper_channel
true.

So, the hw_config will happen.

Or am I missing something?

Thanks,
Ben

>
> i don't think it will reconfig if we are already on-channel, as in
> this case oper_channel == hw_channel.
>
> thanks for your review!
> Eliad.


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Eliad Peller]

On Mon, Jul 25, 2011 at 10:56 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 07/25/2011 12:16 PM, Eliad Peller wrote:
>>
>> hi Ben,
>>
>> On Mon, Jul 25, 2011 at 8:18 PM, Ben Greear<greearb@candelatech.com>
>>  wrote:
>>>
>>> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>>>
>>>> The hw is currently not configured when going
>>>> back on-channel.
>>>
>>> I am less sure about this patch.  With the existing code,
>>> I think it should catch going from on channel to off
>>> and do the hw config properly.
>>>
>> IIUC, this code is responsible for going back on-channel (if there is
>> no started work on the tmp_channel).
>>
>>> With your change it will also reconfig the hardware, but it will
>>> reconfig even if we were already on-channel (if, for instance,
>>> local->tmp_channel is oper-channel), right?
>>>
>>> Can you please explain in more detail how this code is
>>> broken?
>>>
>> we should reconfigure the hardware iff the hardware is not configured
>> to the operational channel.
>> the current code doesn't handle it (e.g. oper_channel=1,
>> tmp_channel=11, hw_channel=11. since
>> ieee80211_cfg_on_oper_channel(local) == 0, the hw won't go back
>> on-channel).
>
> If we are off-channel when entering that block of code, then tmp_channel
> != NULL, and on_oper_chan will be false.
>
right.

> Then, we set tmp_channel to NULL, which should make
> ieee80211_cfg_on_oper_channel
> true.
>
tmp_channel is NULL, but ieee80211_cfg_on_oper_channel() also checks for:

	/* Check current hardware-config against oper_channel. */
	if ((local->oper_channel != local->hw.conf.channel) ||
	    (local->_oper_channel_type != local->hw.conf.channel_type))
		return false;


so it will return false, and hw_config won't happen.

Eliad.

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Ben Greear]

On 07/25/2011 01:07 PM, Eliad Peller wrote:
> On Mon, Jul 25, 2011 at 10:56 PM, Ben Greear<greearb@candelatech.com>  wrote:
>> On 07/25/2011 12:16 PM, Eliad Peller wrote:
>>>
>>> hi Ben,
>>>
>>> On Mon, Jul 25, 2011 at 8:18 PM, Ben Greear<greearb@candelatech.com>
>>>   wrote:
>>>>
>>>> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>>>>
>>>>> The hw is currently not configured when going
>>>>> back on-channel.
>>>>
>>>> I am less sure about this patch.  With the existing code,
>>>> I think it should catch going from on channel to off
>>>> and do the hw config properly.
>>>>
>>> IIUC, this code is responsible for going back on-channel (if there is
>>> no started work on the tmp_channel).
>>>
>>>> With your change it will also reconfig the hardware, but it will
>>>> reconfig even if we were already on-channel (if, for instance,
>>>> local->tmp_channel is oper-channel), right?
>>>>
>>>> Can you please explain in more detail how this code is
>>>> broken?
>>>>
>>> we should reconfigure the hardware iff the hardware is not configured
>>> to the operational channel.
>>> the current code doesn't handle it (e.g. oper_channel=1,
>>> tmp_channel=11, hw_channel=11. since
>>> ieee80211_cfg_on_oper_channel(local) == 0, the hw won't go back
>>> on-channel).
>>
>> If we are off-channel when entering that block of code, then tmp_channel
>> != NULL, and on_oper_chan will be false.
>>
> right.
>
>> Then, we set tmp_channel to NULL, which should make
>> ieee80211_cfg_on_oper_channel
>> true.
>>
> tmp_channel is NULL, but ieee80211_cfg_on_oper_channel() also checks for:
>
> 	/* Check current hardware-config against oper_channel. */
> 	if ((local->oper_channel != local->hw.conf.channel) ||
> 	    (local->_oper_channel_type != local->hw.conf.channel_type))
> 		return false;
>
>
> so it will return false, and hw_config won't happen.

Ahh, ok, I see your point.

Your fix should be more correct than the current code, but
I think it might still could cause hardware config when not needed.
That isn't really a bug, just less efficient.  And, I'd have to
look at the code in detail to be certain.  I'm trying to be on
vacation this week, but will poke at it when I get a chance.

In the meantime, your patch is probably worth applying, and
should probably go to stable.  Hopefully Johannes can review
it as well, as I obviously didn't get it all right the first time!

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Eliad Peller]

>>>>> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>>>>>
>>>>>> The hw is currently not configured when going
>>>>>> back on-channel.
>>>>>
>>>>> I am less sure about this patch.  With the existing code,
>>>>> I think it should catch going from on channel to off
>>>>> and do the hw config properly.
>>>>>
>>>> IIUC, this code is responsible for going back on-channel (if there is
>>>> no started work on the tmp_channel).
>>>>
>>>>> With your change it will also reconfig the hardware, but it will
>>>>> reconfig even if we were already on-channel (if, for instance,
>>>>> local->tmp_channel is oper-channel), right?
>>>>>
>>>>> Can you please explain in more detail how this code is
>>>>> broken?
>>>>>
>>>> we should reconfigure the hardware iff the hardware is not configured
>>>> to the operational channel.
>>>> the current code doesn't handle it (e.g. oper_channel=1,
>>>> tmp_channel=11, hw_channel=11. since
>>>> ieee80211_cfg_on_oper_channel(local) == 0, the hw won't go back
>>>> on-channel).
>>>
>>> If we are off-channel when entering that block of code, then tmp_channel
>>> != NULL, and on_oper_chan will be false.
>>>
>> right.
>>
>>> Then, we set tmp_channel to NULL, which should make
>>> ieee80211_cfg_on_oper_channel
>>> true.
>>>
>> tmp_channel is NULL, but ieee80211_cfg_on_oper_channel() also checks for:
>>
>>        /* Check current hardware-config against oper_channel. */
>>        if ((local->oper_channel != local->hw.conf.channel) ||
>>            (local->_oper_channel_type != local->hw.conf.channel_type))
>>                return false;
>>
>>
>> so it will return false, and hw_config won't happen.
>
> Ahh, ok, I see your point.
>
> Your fix should be more correct than the current code, but
> I think it might still could cause hardware config when not needed.
> That isn't really a bug, just less efficient.  And, I'd have to
> look at the code in detail to be certain.  I'm trying to be on
> vacation this week, but will poke at it when I get a chance.
>
> In the meantime, your patch is probably worth applying, and
> should probably go to stable.  Hopefully Johannes can review
> it as well, as I obviously didn't get it all right the first time!
>

as i'm not sure in this patch either, i guess we should better wait
for Johannes' review.

thanks,
Eliad.

Re: [RFC 1/2] mac80211: fix remain_off_channel regression

[Johannes Berg]

On Mon, 2011-07-25 at 18:29 +0300, Eliad Peller wrote:
> i'm not familiar enough with the off_channel flow,
> but this one looks completely broken - we should
> remain_off_channel if the work was started, and
> the work's channel and channel_type are the same
> as local->tmp_channel and local->tmp_channel_type.
> 
> however, if wk->chan_type and local->tmp_channel_type
> coexist (e.g. have the same channel type), we won't
> remain_off_channel.
> 
> this behavior was introduced by commit da2fd1f
> ("mac80211: Allow work items to use existing
> channel type.")

Yeah this seems obvious.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

Now that I look at ieee80211_work_ct_coexists() itself again though it
seems to do HT20 wrong?

> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---
>  net/mac80211/work.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> index a94b312..3291958 100644
> --- a/net/mac80211/work.c
> +++ b/net/mac80211/work.c
> @@ -1068,8 +1068,8 @@ static void ieee80211_work_work(struct work_struct *work)
>  			continue;
>  		if (wk->chan != local->tmp_channel)
>  			continue;
> -		if (ieee80211_work_ct_coexists(wk->chan_type,
> -					       local->tmp_channel_type))
> +		if (!ieee80211_work_ct_coexists(wk->chan_type,
> +						local->tmp_channel_type))
>  			continue;
>  		remain_off_channel = true;
>  	}

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Johannes Berg]

On Mon, 2011-07-25 at 10:18 -0700, Ben Greear wrote:
> On 07/25/2011 08:29 AM, Eliad Peller wrote:
> > The hw is currently not configured when going
> > back on-channel.
> 
> I am less sure about this patch.  With the existing code,
> I think it should catch going from on channel to off
> and do the hw config properly.
> 
> With your change it will also reconfig the hardware, but it will
> reconfig even if we were already on-channel (if, for instance,
> local->tmp_channel is oper-channel), right?

I think even if we're already on the same channel, we might still have
the off-channel flag (IEEE80211_CONF_OFFCHANNEL) set? So the patch might
still be needed to clear that?

johannes

Re: [RFC 1/2] mac80211: fix remain_off_channel regression

[Eliad Peller]

On Tue, Aug 9, 2011 at 3:13 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2011-07-25 at 18:29 +0300, Eliad Peller wrote:
>> i'm not familiar enough with the off_channel flow,
>> but this one looks completely broken - we should
>> remain_off_channel if the work was started, and
>> the work's channel and channel_type are the same
>> as local->tmp_channel and local->tmp_channel_type.
>>
>> however, if wk->chan_type and local->tmp_channel_type
>> coexist (e.g. have the same channel type), we won't
>> remain_off_channel.
>>
>> this behavior was introduced by commit da2fd1f
>> ("mac80211: Allow work items to use existing
>> channel type.")
>
> Yeah this seems obvious.
>
> Acked-by: Johannes Berg <johannes@sipsolutions.net>
>
> Now that I look at ieee80211_work_ct_coexists() itself again though it
> seems to do HT20 wrong?
>
hmm... yeah.
and also HT40 seems wrong (e.g. HT40 + NO_HT).

Re: [RFC 1/2] mac80211: fix remain_off_channel regression

[Johannes Berg]

On Tue, 2011-08-09 at 15:48 +0300, Eliad Peller wrote:
> On Tue, Aug 9, 2011 at 3:13 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2011-07-25 at 18:29 +0300, Eliad Peller wrote:
> >> i'm not familiar enough with the off_channel flow,
> >> but this one looks completely broken - we should
> >> remain_off_channel if the work was started, and
> >> the work's channel and channel_type are the same
> >> as local->tmp_channel and local->tmp_channel_type.
> >>
> >> however, if wk->chan_type and local->tmp_channel_type
> >> coexist (e.g. have the same channel type), we won't
> >> remain_off_channel.
> >>
> >> this behavior was introduced by commit da2fd1f
> >> ("mac80211: Allow work items to use existing
> >> channel type.")
> >
> > Yeah this seems obvious.
> >
> > Acked-by: Johannes Berg <johannes@sipsolutions.net>
> >
> > Now that I look at ieee80211_work_ct_coexists() itself again though it
> > seems to do HT20 wrong?
> >
> hmm... yeah.
> and also HT40 seems wrong (e.g. HT40 + NO_HT).

Hm yeah. Well, since wk_ct is always NO_HT right now maybe we should
remove this altogether?

johannes

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Ben Greear]

On 08/09/2011 05:14 AM, Johannes Berg wrote:
> On Mon, 2011-07-25 at 10:18 -0700, Ben Greear wrote:
>> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>> The hw is currently not configured when going
>>> back on-channel.
>>
>> I am less sure about this patch.  With the existing code,
>> I think it should catch going from on channel to off
>> and do the hw config properly.
>>
>> With your change it will also reconfig the hardware, but it will
>> reconfig even if we were already on-channel (if, for instance,
>> local->tmp_channel is oper-channel), right?
>
> I think even if we're already on the same channel, we might still have
> the off-channel flag (IEEE80211_CONF_OFFCHANNEL) set? So the patch might
> still be needed to clear that?

Well, maybe so, but if we are not off-channel and still have the off-channel
flag set, that would seem to be another bug.

Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Johannes Berg]

On Tue, 2011-08-09 at 06:27 -0700, Ben Greear wrote:
> On 08/09/2011 05:14 AM, Johannes Berg wrote:
> > On Mon, 2011-07-25 at 10:18 -0700, Ben Greear wrote:
> >> On 07/25/2011 08:29 AM, Eliad Peller wrote:
> >>> The hw is currently not configured when going
> >>> back on-channel.
> >>
> >> I am less sure about this patch.  With the existing code,
> >> I think it should catch going from on channel to off
> >> and do the hw config properly.
> >>
> >> With your change it will also reconfig the hardware, but it will
> >> reconfig even if we were already on-channel (if, for instance,
> >> local->tmp_channel is oper-channel), right?
> >
> > I think even if we're already on the same channel, we might still have
> > the off-channel flag (IEEE80211_CONF_OFFCHANNEL) set? So the patch might
> > still be needed to clear that?
> 
> Well, maybe so, but if we are not off-channel and still have the off-channel
> flag set, that would seem to be another bug.

Hm, true. Not sure about this then.

johannes

Re: [RFC 2/2] mac80211: config hw when going back on-channel

[Ben Greear]

On 08/09/2011 06:28 AM, Johannes Berg wrote:
> On Tue, 2011-08-09 at 06:27 -0700, Ben Greear wrote:
>> On 08/09/2011 05:14 AM, Johannes Berg wrote:
>>> On Mon, 2011-07-25 at 10:18 -0700, Ben Greear wrote:
>>>> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>>>> The hw is currently not configured when going
>>>>> back on-channel.
>>>>
>>>> I am less sure about this patch.  With the existing code,
>>>> I think it should catch going from on channel to off
>>>> and do the hw config properly.
>>>>
>>>> With your change it will also reconfig the hardware, but it will
>>>> reconfig even if we were already on-channel (if, for instance,
>>>> local->tmp_channel is oper-channel), right?
>>>
>>> I think even if we're already on the same channel, we might still have
>>> the off-channel flag (IEEE80211_CONF_OFFCHANNEL) set? So the patch might
>>> still be needed to clear that?
>>
>> Well, maybe so, but if we are not off-channel and still have the off-channel
>> flag set, that would seem to be another bug.
>
> Hm, true. Not sure about this then.

When I was looking at his patches, I eventually thought that it was better
than current code, but I think it might still have some room for improvement
because I think it could, in some cases, cause a re-config when not needed.

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 1/2] mac80211: fix remain_off_channel regression

[Ben Greear]

On 07/25/2011 08:29 AM, Eliad Peller wrote:
> i'm not familiar enough with the off_channel flow,
> but this one looks completely broken - we should
> remain_off_channel if the work was started, and
> the work's channel and channel_type are the same
> as local->tmp_channel and local->tmp_channel_type.
>
> however, if wk->chan_type and local->tmp_channel_type
> coexist (e.g. have the same channel type), we won't
> remain_off_channel.
>
> this behavior was introduced by commit da2fd1f
> ("mac80211: Allow work items to use existing
> channel type.")
>
> Signed-off-by: Eliad Peller<eliad@wizery.com>

Both Johannes and I agreed with this patch shortly after it
was posted, and I have just done a quick test with multiple
stations and it appears to work fine.

Eliad:  Please re-send this patch w/out the RFC, and you
can add:

Tested-by: Ben Greear <greearb@candelatech.com>

Thanks,
Ben



> ---
>   net/mac80211/work.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> index a94b312..3291958 100644
> --- a/net/mac80211/work.c
> +++ b/net/mac80211/work.c
> @@ -1068,8 +1068,8 @@ static void ieee80211_work_work(struct work_struct *work)
>   			continue;
>   		if (wk->chan != local->tmp_channel)
>   			continue;
> -		if (ieee80211_work_ct_coexists(wk->chan_type,
> -					       local->tmp_channel_type))
> +		if (!ieee80211_work_ct_coexists(wk->chan_type,
> +						local->tmp_channel_type))
>   			continue;
>   		remain_off_channel = true;
>   	}


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 1/2] mac80211: fix remain_off_channel regression

[Eliad Peller]

hi Ben,

On Wed, Oct 19, 2011 at 8:03 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 07/25/2011 08:29 AM, Eliad Peller wrote:
>>
>> i'm not familiar enough with the off_channel flow,
>> but this one looks completely broken - we should
>> remain_off_channel if the work was started, and
>> the work's channel and channel_type are the same
>> as local->tmp_channel and local->tmp_channel_type.
>>
>> however, if wk->chan_type and local->tmp_channel_type
>> coexist (e.g. have the same channel type), we won't
>> remain_off_channel.
>>
>> this behavior was introduced by commit da2fd1f
>> ("mac80211: Allow work items to use existing
>> channel type.")
>>
>> Signed-off-by: Eliad Peller<eliad@wizery.com>
>
> Both Johannes and I agreed with this patch shortly after it
> was posted, and I have just done a quick test with multiple
> stations and it appears to work fine.
>
> Eliad:  Please re-send this patch w/out the RFC, and you
> can add:
>
> Tested-by: Ben Greear <greearb@candelatech.com>
>
it seems that i forgot to resubmit the RFCs as patches.
thanks for testing, i'll submit them shortly.

(John - i see there is some ongoing discussion regarding similar
patches, so feel free to take only some of them, according to the
conclusion)

Eliad.