* [PATCH] mac80211: Fix off-channel problem in work task.
@ 2011-10-19 18:44 greearb
2011-10-20 14:58 ` Stanislaw Gruszka
0 siblings, 1 reply; 5+ messages in thread
From: greearb @ 2011-10-19 18:44 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
The ieee80211_cfg_on_oper_channel method compared the
current hardware config as well as the desired hardware
config. In most cases, this is proper, but when deciding
whether to go back on-channel, if the hardware is not
configured on-channel, but logically it *should* be
on-channel, then we must go on-channel.
This patch adds a flag to the ieee80211_cfg_on_oper_channel
logic to disable comparing the actual hardware so we do not
have to create another tricky method with similar logic.
Reported-by: Eliad Peller <eliad@wizery.com>
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
NOTE: This is tricky stuff, please do not apply until at
least Johannes gets time to review this.
:100644 100644 4c3d1f5... 40ca484... M net/mac80211/ieee80211_i.h
:100644 100644 d4ee6d2... 3ead637... M net/mac80211/main.c
:100644 100644 397343a... d1b6b29... M net/mac80211/scan.c
:100644 100644 bf5be22... 62a3357... M net/mac80211/work.c
net/mac80211/ieee80211_i.h | 3 ++-
net/mac80211/main.c | 13 ++++++++-----
net/mac80211/scan.c | 10 +++++-----
net/mac80211/work.c | 11 +++++++----
4 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 4c3d1f5..40ca484 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1139,7 +1139,8 @@ int ieee80211_request_sched_scan_stop(struct ieee80211_sub_if_data *sdata);
void ieee80211_sched_scan_stopped_work(struct work_struct *work);
/* off-channel helpers */
-bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local);
+bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local,
+ bool check_current_hw_cfg);
void ieee80211_offchannel_enable_all_ps(struct ieee80211_local *local,
bool tell_ap);
void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d4ee6d2..3ead637 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -94,11 +94,13 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
/*
* Returns true if we are logically configured to be on
- * the operating channel AND the hardware-conf is currently
- * configured on the operating channel. Compares channel-type
+ * the operating channel and channel-type.
+ * If the check_current_hw_cfg argument is TRUE,
+ * the currently configured hardware value is checked
* as well.
*/
-bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local)
+bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local,
+ bool check_current_hw_cfg)
{
struct ieee80211_channel *chan, *scan_chan;
enum nl80211_channel_type channel_type;
@@ -126,8 +128,9 @@ bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local)
return false;
/* Check current hardware-config against oper_channel. */
- if ((local->oper_channel != local->hw.conf.channel) ||
- (local->_oper_channel_type != local->hw.conf.channel_type))
+ if (check_current_hw_cfg &&
+ ((local->oper_channel != local->hw.conf.channel) ||
+ (local->_oper_channel_type != local->hw.conf.channel_type)))
return false;
return true;
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 397343a..d1b6b29 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -216,7 +216,7 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
* current channel, pass the pkt on up the stack so that
* the rest of the stack can make use of it.
*/
- if (ieee80211_cfg_on_oper_channel(sdata->local)
+ if (ieee80211_cfg_on_oper_channel(sdata->local, true)
&& (channel == sdata->local->oper_channel))
return RX_CONTINUE;
@@ -297,7 +297,7 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
local->scanning = 0;
local->scan_channel = NULL;
- on_oper_chan = ieee80211_cfg_on_oper_channel(local);
+ on_oper_chan = ieee80211_cfg_on_oper_channel(local, true);
if (was_hw_scan || !on_oper_chan)
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
@@ -309,7 +309,7 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
bool on_oper_chan2;
ieee80211_configure_filter(local);
drv_sw_scan_complete(local);
- on_oper_chan2 = ieee80211_cfg_on_oper_channel(local);
+ on_oper_chan2 = ieee80211_cfg_on_oper_channel(local, true);
/* We should always be on-channel at this point. */
WARN_ON(!on_oper_chan2);
if (on_oper_chan2 && (on_oper_chan != on_oper_chan2))
@@ -509,7 +509,7 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
next_chan = local->scan_req->channels[local->scan_channel_idx];
- if (ieee80211_cfg_on_oper_channel(local)) {
+ if (ieee80211_cfg_on_oper_channel(local, true)) {
/* We're currently on operating channel. */
if (next_chan == local->oper_channel)
/* We don't need to move off of operating channel. */
@@ -587,7 +587,7 @@ static void ieee80211_scan_state_enter_oper_channel(struct ieee80211_local *loca
{
/* switch back to the operating channel */
local->scan_channel = NULL;
- if (!ieee80211_cfg_on_oper_channel(local))
+ if (!ieee80211_cfg_on_oper_channel(local, true))
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
/*
diff --git a/net/mac80211/work.c b/net/mac80211/work.c
index bf5be22..62a3357 100644
--- a/net/mac80211/work.c
+++ b/net/mac80211/work.c
@@ -973,7 +973,8 @@ static void ieee80211_work_work(struct work_struct *work)
bool tmp_chan_changed = false;
bool on_oper_chan2;
enum nl80211_channel_type wk_ct;
- on_oper_chan = ieee80211_cfg_on_oper_channel(local);
+ on_oper_chan = ieee80211_cfg_on_oper_channel(local,
+ true);
/* Work with existing channel type if possible. */
wk_ct = wk->chan_type;
@@ -993,7 +994,8 @@ static void ieee80211_work_work(struct work_struct *work)
* happen to be on the same channel as
* the requested channel.
*/
- on_oper_chan2 = ieee80211_cfg_on_oper_channel(local);
+ on_oper_chan2 = ieee80211_cfg_on_oper_channel(local,
+ true);
if (on_oper_chan != on_oper_chan2) {
if (on_oper_chan2) {
/* going off oper channel, PS too */
@@ -1091,7 +1093,7 @@ 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);
+ bool on_oper_chan = ieee80211_cfg_on_oper_channel(local, true);
local->tmp_channel = NULL;
/* If tmp_channel wasn't operating channel, then
* we need to go back on-channel.
@@ -1101,7 +1103,8 @@ 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, false) &&
+ !on_oper_chan)
ieee80211_hw_config(local, 0);
/* At the least, we need to disable offchannel_ps,
--
1.7.3.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211: Fix off-channel problem in work task.
2011-10-19 18:44 [PATCH] mac80211: Fix off-channel problem in work task greearb
@ 2011-10-20 14:58 ` Stanislaw Gruszka
2011-10-20 15:22 ` Ben Greear
0 siblings, 1 reply; 5+ messages in thread
From: Stanislaw Gruszka @ 2011-10-20 14:58 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless, Eliad Peller, Johannes Berg
On Wed, Oct 19, 2011 at 11:44:36AM -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> The ieee80211_cfg_on_oper_channel method compared the
> current hardware config as well as the desired hardware
> config. In most cases, this is proper, but when deciding
> whether to go back on-channel, if the hardware is not
> configured on-channel, but logically it *should* be
> on-channel, then we must go on-channel.
>
> This patch adds a flag to the ieee80211_cfg_on_oper_channel
> logic to disable comparing the actual hardware so we do not
> have to create another tricky method with similar logic.
>
> Reported-by: Eliad Peller <eliad@wizery.com>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
I much more prefer previous one-line patch from Eliad
http://news.gmane.org/find-root.php?message_id=%3c1311607763%2d12603%2d3%2dgit%2dsend%2demail%2deliad%40wizery.com%3e
this one seems to provide unneeded code complexity, but
behaviour i.e. number of channel switches in hardware
is the same.
Stanislaw
> ---
>
> NOTE: This is tricky stuff, please do not apply until at
> least Johannes gets time to review this.
>
> :100644 100644 4c3d1f5... 40ca484... M net/mac80211/ieee80211_i.h
> :100644 100644 d4ee6d2... 3ead637... M net/mac80211/main.c
> :100644 100644 397343a... d1b6b29... M net/mac80211/scan.c
> :100644 100644 bf5be22... 62a3357... M net/mac80211/work.c
> net/mac80211/ieee80211_i.h | 3 ++-
> net/mac80211/main.c | 13 ++++++++-----
> net/mac80211/scan.c | 10 +++++-----
> net/mac80211/work.c | 11 +++++++----
> 4 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 4c3d1f5..40ca484 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1139,7 +1139,8 @@ int ieee80211_request_sched_scan_stop(struct ieee80211_sub_if_data *sdata);
> void ieee80211_sched_scan_stopped_work(struct work_struct *work);
>
> /* off-channel helpers */
> -bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local);
> +bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local,
> + bool check_current_hw_cfg);
> void ieee80211_offchannel_enable_all_ps(struct ieee80211_local *local,
> bool tell_ap);
> void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local,
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index d4ee6d2..3ead637 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -94,11 +94,13 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
>
> /*
> * Returns true if we are logically configured to be on
> - * the operating channel AND the hardware-conf is currently
> - * configured on the operating channel. Compares channel-type
> + * the operating channel and channel-type.
> + * If the check_current_hw_cfg argument is TRUE,
> + * the currently configured hardware value is checked
> * as well.
> */
> -bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local)
> +bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local,
> + bool check_current_hw_cfg)
> {
> struct ieee80211_channel *chan, *scan_chan;
> enum nl80211_channel_type channel_type;
> @@ -126,8 +128,9 @@ bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local)
> return false;
>
> /* Check current hardware-config against oper_channel. */
> - if ((local->oper_channel != local->hw.conf.channel) ||
> - (local->_oper_channel_type != local->hw.conf.channel_type))
> + if (check_current_hw_cfg &&
> + ((local->oper_channel != local->hw.conf.channel) ||
> + (local->_oper_channel_type != local->hw.conf.channel_type)))
> return false;
>
> return true;
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index 397343a..d1b6b29 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -216,7 +216,7 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
> * current channel, pass the pkt on up the stack so that
> * the rest of the stack can make use of it.
> */
> - if (ieee80211_cfg_on_oper_channel(sdata->local)
> + if (ieee80211_cfg_on_oper_channel(sdata->local, true)
> && (channel == sdata->local->oper_channel))
> return RX_CONTINUE;
>
> @@ -297,7 +297,7 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
> local->scanning = 0;
> local->scan_channel = NULL;
>
> - on_oper_chan = ieee80211_cfg_on_oper_channel(local);
> + on_oper_chan = ieee80211_cfg_on_oper_channel(local, true);
>
> if (was_hw_scan || !on_oper_chan)
> ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
> @@ -309,7 +309,7 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
> bool on_oper_chan2;
> ieee80211_configure_filter(local);
> drv_sw_scan_complete(local);
> - on_oper_chan2 = ieee80211_cfg_on_oper_channel(local);
> + on_oper_chan2 = ieee80211_cfg_on_oper_channel(local, true);
> /* We should always be on-channel at this point. */
> WARN_ON(!on_oper_chan2);
> if (on_oper_chan2 && (on_oper_chan != on_oper_chan2))
> @@ -509,7 +509,7 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
>
> next_chan = local->scan_req->channels[local->scan_channel_idx];
>
> - if (ieee80211_cfg_on_oper_channel(local)) {
> + if (ieee80211_cfg_on_oper_channel(local, true)) {
> /* We're currently on operating channel. */
> if (next_chan == local->oper_channel)
> /* We don't need to move off of operating channel. */
> @@ -587,7 +587,7 @@ static void ieee80211_scan_state_enter_oper_channel(struct ieee80211_local *loca
> {
> /* switch back to the operating channel */
> local->scan_channel = NULL;
> - if (!ieee80211_cfg_on_oper_channel(local))
> + if (!ieee80211_cfg_on_oper_channel(local, true))
> ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
>
> /*
> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> index bf5be22..62a3357 100644
> --- a/net/mac80211/work.c
> +++ b/net/mac80211/work.c
> @@ -973,7 +973,8 @@ static void ieee80211_work_work(struct work_struct *work)
> bool tmp_chan_changed = false;
> bool on_oper_chan2;
> enum nl80211_channel_type wk_ct;
> - on_oper_chan = ieee80211_cfg_on_oper_channel(local);
> + on_oper_chan = ieee80211_cfg_on_oper_channel(local,
> + true);
>
> /* Work with existing channel type if possible. */
> wk_ct = wk->chan_type;
> @@ -993,7 +994,8 @@ static void ieee80211_work_work(struct work_struct *work)
> * happen to be on the same channel as
> * the requested channel.
> */
> - on_oper_chan2 = ieee80211_cfg_on_oper_channel(local);
> + on_oper_chan2 = ieee80211_cfg_on_oper_channel(local,
> + true);
> if (on_oper_chan != on_oper_chan2) {
> if (on_oper_chan2) {
> /* going off oper channel, PS too */
> @@ -1091,7 +1093,7 @@ 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);
> + bool on_oper_chan = ieee80211_cfg_on_oper_channel(local, true);
> local->tmp_channel = NULL;
> /* If tmp_channel wasn't operating channel, then
> * we need to go back on-channel.
> @@ -1101,7 +1103,8 @@ 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, false) &&
> + !on_oper_chan)
> ieee80211_hw_config(local, 0);
>
> /* At the least, we need to disable offchannel_ps,
> --
> 1.7.3.4
>
> --
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211: Fix off-channel problem in work task.
2011-10-20 14:58 ` Stanislaw Gruszka
@ 2011-10-20 15:22 ` Ben Greear
2011-10-20 16:09 ` Stanislaw Gruszka
0 siblings, 1 reply; 5+ messages in thread
From: Ben Greear @ 2011-10-20 15:22 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: linux-wireless, Eliad Peller, Johannes Berg
On 10/20/2011 07:58 AM, Stanislaw Gruszka wrote:
> On Wed, Oct 19, 2011 at 11:44:36AM -0700, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> The ieee80211_cfg_on_oper_channel method compared the
>> current hardware config as well as the desired hardware
>> config. In most cases, this is proper, but when deciding
>> whether to go back on-channel, if the hardware is not
>> configured on-channel, but logically it *should* be
>> on-channel, then we must go on-channel.
>>
>> This patch adds a flag to the ieee80211_cfg_on_oper_channel
>> logic to disable comparing the actual hardware so we do not
>> have to create another tricky method with similar logic.
>>
>> Reported-by: Eliad Peller<eliad@wizery.com>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>
> I much more prefer previous one-line patch from Eliad
> http://news.gmane.org/find-root.php?message_id=%3c1311607763%2d12603%2d3%2dgit%2dsend%2demail%2deliad%40wizery.com%3e
>
> this one seems to provide unneeded code complexity, but
> behaviour i.e. number of channel switches in hardware
> is the same.
I believe it's possible to set tmp_channel to NULL and not actually
change the on/off channel configuration, and my patch should allow us to
skip a hardware config in that case.
However, this might only be possible if we are in this code while
scanning, and currently, I don't believe we can be in this work
code while scanning.
So, his patch is probably good enough for now, and if the scanning change
ever happens, we can do something more like what I have.
Thanks,
Ben
>
> Stanislaw
>
>> ---
>>
>> NOTE: This is tricky stuff, please do not apply until at
>> least Johannes gets time to review this.
>>
>> :100644 100644 4c3d1f5... 40ca484... M net/mac80211/ieee80211_i.h
>> :100644 100644 d4ee6d2... 3ead637... M net/mac80211/main.c
>> :100644 100644 397343a... d1b6b29... M net/mac80211/scan.c
>> :100644 100644 bf5be22... 62a3357... M net/mac80211/work.c
>> net/mac80211/ieee80211_i.h | 3 ++-
>> net/mac80211/main.c | 13 ++++++++-----
>> net/mac80211/scan.c | 10 +++++-----
>> net/mac80211/work.c | 11 +++++++----
>> 4 files changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>> index 4c3d1f5..40ca484 100644
>> --- a/net/mac80211/ieee80211_i.h
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -1139,7 +1139,8 @@ int ieee80211_request_sched_scan_stop(struct ieee80211_sub_if_data *sdata);
>> void ieee80211_sched_scan_stopped_work(struct work_struct *work);
>>
>> /* off-channel helpers */
>> -bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local);
>> +bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local,
>> + bool check_current_hw_cfg);
>> void ieee80211_offchannel_enable_all_ps(struct ieee80211_local *local,
>> bool tell_ap);
>> void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local,
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index d4ee6d2..3ead637 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -94,11 +94,13 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
>>
>> /*
>> * Returns true if we are logically configured to be on
>> - * the operating channel AND the hardware-conf is currently
>> - * configured on the operating channel. Compares channel-type
>> + * the operating channel and channel-type.
>> + * If the check_current_hw_cfg argument is TRUE,
>> + * the currently configured hardware value is checked
>> * as well.
>> */
>> -bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local)
>> +bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local,
>> + bool check_current_hw_cfg)
>> {
>> struct ieee80211_channel *chan, *scan_chan;
>> enum nl80211_channel_type channel_type;
>> @@ -126,8 +128,9 @@ bool ieee80211_cfg_on_oper_channel(struct ieee80211_local *local)
>> return false;
>>
>> /* Check current hardware-config against oper_channel. */
>> - if ((local->oper_channel != local->hw.conf.channel) ||
>> - (local->_oper_channel_type != local->hw.conf.channel_type))
>> + if (check_current_hw_cfg&&
>> + ((local->oper_channel != local->hw.conf.channel) ||
>> + (local->_oper_channel_type != local->hw.conf.channel_type)))
>> return false;
>>
>> return true;
>> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
>> index 397343a..d1b6b29 100644
>> --- a/net/mac80211/scan.c
>> +++ b/net/mac80211/scan.c
>> @@ -216,7 +216,7 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
>> * current channel, pass the pkt on up the stack so that
>> * the rest of the stack can make use of it.
>> */
>> - if (ieee80211_cfg_on_oper_channel(sdata->local)
>> + if (ieee80211_cfg_on_oper_channel(sdata->local, true)
>> && (channel == sdata->local->oper_channel))
>> return RX_CONTINUE;
>>
>> @@ -297,7 +297,7 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
>> local->scanning = 0;
>> local->scan_channel = NULL;
>>
>> - on_oper_chan = ieee80211_cfg_on_oper_channel(local);
>> + on_oper_chan = ieee80211_cfg_on_oper_channel(local, true);
>>
>> if (was_hw_scan || !on_oper_chan)
>> ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
>> @@ -309,7 +309,7 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
>> bool on_oper_chan2;
>> ieee80211_configure_filter(local);
>> drv_sw_scan_complete(local);
>> - on_oper_chan2 = ieee80211_cfg_on_oper_channel(local);
>> + on_oper_chan2 = ieee80211_cfg_on_oper_channel(local, true);
>> /* We should always be on-channel at this point. */
>> WARN_ON(!on_oper_chan2);
>> if (on_oper_chan2&& (on_oper_chan != on_oper_chan2))
>> @@ -509,7 +509,7 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
>>
>> next_chan = local->scan_req->channels[local->scan_channel_idx];
>>
>> - if (ieee80211_cfg_on_oper_channel(local)) {
>> + if (ieee80211_cfg_on_oper_channel(local, true)) {
>> /* We're currently on operating channel. */
>> if (next_chan == local->oper_channel)
>> /* We don't need to move off of operating channel. */
>> @@ -587,7 +587,7 @@ static void ieee80211_scan_state_enter_oper_channel(struct ieee80211_local *loca
>> {
>> /* switch back to the operating channel */
>> local->scan_channel = NULL;
>> - if (!ieee80211_cfg_on_oper_channel(local))
>> + if (!ieee80211_cfg_on_oper_channel(local, true))
>> ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
>>
>> /*
>> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
>> index bf5be22..62a3357 100644
>> --- a/net/mac80211/work.c
>> +++ b/net/mac80211/work.c
>> @@ -973,7 +973,8 @@ static void ieee80211_work_work(struct work_struct *work)
>> bool tmp_chan_changed = false;
>> bool on_oper_chan2;
>> enum nl80211_channel_type wk_ct;
>> - on_oper_chan = ieee80211_cfg_on_oper_channel(local);
>> + on_oper_chan = ieee80211_cfg_on_oper_channel(local,
>> + true);
>>
>> /* Work with existing channel type if possible. */
>> wk_ct = wk->chan_type;
>> @@ -993,7 +994,8 @@ static void ieee80211_work_work(struct work_struct *work)
>> * happen to be on the same channel as
>> * the requested channel.
>> */
>> - on_oper_chan2 = ieee80211_cfg_on_oper_channel(local);
>> + on_oper_chan2 = ieee80211_cfg_on_oper_channel(local,
>> + true);
>> if (on_oper_chan != on_oper_chan2) {
>> if (on_oper_chan2) {
>> /* going off oper channel, PS too */
>> @@ -1091,7 +1093,7 @@ 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);
>> + bool on_oper_chan = ieee80211_cfg_on_oper_channel(local, true);
>> local->tmp_channel = NULL;
>> /* If tmp_channel wasn't operating channel, then
>> * we need to go back on-channel.
>> @@ -1101,7 +1103,8 @@ 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, false)&&
>> + !on_oper_chan)
>> ieee80211_hw_config(local, 0);
>>
>> /* At the least, we need to disable offchannel_ps,
>> --
>> 1.7.3.4
>>
>> --
>> 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
> --
> 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
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211: Fix off-channel problem in work task.
2011-10-20 15:22 ` Ben Greear
@ 2011-10-20 16:09 ` Stanislaw Gruszka
2011-10-20 16:12 ` Ben Greear
0 siblings, 1 reply; 5+ messages in thread
From: Stanislaw Gruszka @ 2011-10-20 16:09 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless, Eliad Peller, Johannes Berg
On Thu, Oct 20, 2011 at 08:22:55AM -0700, Ben Greear wrote:
> On 10/20/2011 07:58 AM, Stanislaw Gruszka wrote:
> >On Wed, Oct 19, 2011 at 11:44:36AM -0700, greearb@candelatech.com wrote:
> >>From: Ben Greear<greearb@candelatech.com>
> >>
> >>The ieee80211_cfg_on_oper_channel method compared the
> >>current hardware config as well as the desired hardware
> >>config. In most cases, this is proper, but when deciding
> >>whether to go back on-channel, if the hardware is not
> >>configured on-channel, but logically it *should* be
> >>on-channel, then we must go on-channel.
> >>
> >>This patch adds a flag to the ieee80211_cfg_on_oper_channel
> >>logic to disable comparing the actual hardware so we do not
> >>have to create another tricky method with similar logic.
> >>
> >>Reported-by: Eliad Peller<eliad@wizery.com>
> >>Signed-off-by: Ben Greear<greearb@candelatech.com>
> >
> >I much more prefer previous one-line patch from Eliad
> >http://news.gmane.org/find-root.php?message_id=%3c1311607763%2d12603%2d3%2dgit%2dsend%2demail%2deliad%40wizery.com%3e
> >
> >this one seems to provide unneeded code complexity, but
> >behaviour i.e. number of channel switches in hardware
> >is the same.
>
> I believe it's possible to set tmp_channel to NULL and not actually
> change the on/off channel configuration, and my patch should allow us to
> skip a hardware config in that case.
>
> However, this might only be possible if we are in this code while
> scanning, and currently, I don't believe we can be in this work
> code while scanning.
So you want to optimize a case that is not even possible at present.
Do you know that "premature optimization is the root of all evil" ? :-)
I'm not quite against by that change, but I would like to have -stable
fixes, since bug we are talking about not only annoys people by warning,
but also disallow to associate to wireless network. I prefer that we
apply Eliad patches and cc -stable and do possible further optimization
on top of that (ideally if some measurement will be available that show
optimization really works).
Thanks
Stanislaw
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211: Fix off-channel problem in work task.
2011-10-20 16:09 ` Stanislaw Gruszka
@ 2011-10-20 16:12 ` Ben Greear
0 siblings, 0 replies; 5+ messages in thread
From: Ben Greear @ 2011-10-20 16:12 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: linux-wireless, Eliad Peller, Johannes Berg
On 10/20/2011 09:09 AM, Stanislaw Gruszka wrote:
> On Thu, Oct 20, 2011 at 08:22:55AM -0700, Ben Greear wrote:
>> On 10/20/2011 07:58 AM, Stanislaw Gruszka wrote:
>>> On Wed, Oct 19, 2011 at 11:44:36AM -0700, greearb@candelatech.com wrote:
>>>> From: Ben Greear<greearb@candelatech.com>
>>>>
>>>> The ieee80211_cfg_on_oper_channel method compared the
>>>> current hardware config as well as the desired hardware
>>>> config. In most cases, this is proper, but when deciding
>>>> whether to go back on-channel, if the hardware is not
>>>> configured on-channel, but logically it *should* be
>>>> on-channel, then we must go on-channel.
>>>>
>>>> This patch adds a flag to the ieee80211_cfg_on_oper_channel
>>>> logic to disable comparing the actual hardware so we do not
>>>> have to create another tricky method with similar logic.
>>>>
>>>> Reported-by: Eliad Peller<eliad@wizery.com>
>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>
>>> I much more prefer previous one-line patch from Eliad
>>> http://news.gmane.org/find-root.php?message_id=%3c1311607763%2d12603%2d3%2dgit%2dsend%2demail%2deliad%40wizery.com%3e
>>>
>>> this one seems to provide unneeded code complexity, but
>>> behaviour i.e. number of channel switches in hardware
>>> is the same.
>>
>> I believe it's possible to set tmp_channel to NULL and not actually
>> change the on/off channel configuration, and my patch should allow us to
>> skip a hardware config in that case.
>>
>> However, this might only be possible if we are in this code while
>> scanning, and currently, I don't believe we can be in this work
>> code while scanning.
>
> So you want to optimize a case that is not even possible at present.
> Do you know that "premature optimization is the root of all evil" ? :-)
I was trying to clean out some of the sloppiness in the on/off channel
logic, but in this case, I don't think the extra parania helped much,
and my attempt at cleverness obviously introduced some bugs...
> I'm not quite against by that change, but I would like to have -stable
> fixes, since bug we are talking about not only annoys people by warning,
> but also disallow to associate to wireless network. I prefer that we
> apply Eliad patches and cc -stable and do possible further optimization
> on top of that (ideally if some measurement will be available that show
> optimization really works).
That is fine with me.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-10-20 16:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-19 18:44 [PATCH] mac80211: Fix off-channel problem in work task greearb
2011-10-20 14:58 ` Stanislaw Gruszka
2011-10-20 15:22 ` Ben Greear
2011-10-20 16:09 ` Stanislaw Gruszka
2011-10-20 16:12 ` Ben Greear
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).