This patch series fixes a number of problems observed during software
scanning, as described in [1].
The first four patches implement improved queue handling during
off-channel operation and add some needed flushing of the hardware
queues, as suggested by Johannes in [2]. This includes adding a new
queue stop reason of off-channel operation, a transmit path for frames
which do need to be transmitted when off-channel, and some improvements
in error handling.
The last three fix a problem specific to brcmsmac (and likely b43,
though I don't have hardware for testing b43). Broadcom hardware
actively clears the PM bit in frame control whenever the hardware is not
configured correctly, and since wireless drivers have no knowledge of
off-channel powersave such configuration cannot be done. The patches
expand the driver powersave configuration with an off-channel state and
update brcmsmac to make use of it.
Johannes: I have a couple of comments/questions for you related to these
patches.
First, in the patches I've added an offchan_tx_ok argument to the tx
operations, but this seems a little awkward to me since it has to be
propogated down through a fairly deep call stack. The alternative idea
that occurred to me is to use a tx control flag, but that seems to be
pretty crowded. Any thoughts?
Second, I attempted to test these patches with iwlwifi (Centrino
Advanced-N 6235) to verify that I didn't break anything for drivers with
hw scanning. My standard test for this involves running iperf while
triggering nearly continuous scans, but I'm seeing lots of problems
running a tcp iperf test even with unpatched 3.8-rc4. iperf with udp
does fine in either direction. I haven't had time to do any kind of
debugging yet, but I thought you'd want to know.
Thanks,
Seth
[1] http://marc.info/?l=linux-wireless&m=135766865110986&w=2
[2] http://marc.info/?l=linux-wireless&m=135838252227053&w=2
Seth Forshee (7):
mac80211: Return a status for tx operations
mac80211: Fix tx queue handling during scans
mac80211: Improve error handling for off-channel operation
mac80211: Add flushes before going off-channel
mac80211: Expand powersave configuration flag to be two bits
mac80211: Add off-channel powersave state
brcmsmac: Add support for off-channel powersave
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +-
drivers/net/wireless/ath/ath9k/main.c | 2 +-
drivers/net/wireless/ath/carl9170/main.c | 2 +-
drivers/net/wireless/ath/carl9170/rx.c | 2 +-
.../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 15 ++-
drivers/net/wireless/brcm80211/brcmsmac/main.c | 9 ++
drivers/net/wireless/brcm80211/brcmsmac/pub.h | 1 +
drivers/net/wireless/iwlwifi/dvm/power.c | 2 +-
drivers/net/wireless/mac80211_hwsim.c | 2 +-
drivers/net/wireless/p54/fwio.c | 2 +-
drivers/net/wireless/p54/txrx.c | 2 +-
drivers/net/wireless/rt2x00/rt2400pci.c | 3 +-
drivers/net/wireless/rt2x00/rt2500pci.c | 3 +-
drivers/net/wireless/rt2x00/rt2500usb.c | 3 +-
drivers/net/wireless/rt2x00/rt2800lib.c | 3 +-
drivers/net/wireless/rt2x00/rt2x00config.c | 4 +-
drivers/net/wireless/rt2x00/rt2x00dev.c | 2 +-
drivers/net/wireless/rt2x00/rt61pci.c | 3 +-
drivers/net/wireless/rt2x00/rt73usb.c | 3 +-
drivers/net/wireless/rtlwifi/core.c | 2 +-
drivers/net/wireless/rtlwifi/ps.c | 4 +-
drivers/net/wireless/ti/wl1251/main.c | 7 +-
drivers/net/wireless/ti/wlcore/main.c | 6 +-
include/net/mac80211.h | 125 +++++++++++++++----
net/mac80211/agg-tx.c | 4 +-
net/mac80211/cfg.c | 2 +-
net/mac80211/ht.c | 2 +-
net/mac80211/ieee80211_i.h | 72 ++++++++---
net/mac80211/mlme.c | 32 ++---
net/mac80211/offchannel.c | 114 +++++++++--------
net/mac80211/scan.c | 44 +++++--
net/mac80211/sta_info.c | 2 +-
net/mac80211/tx.c | 131 +++++++++++---------
net/mac80211/util.c | 13 +-
34 files changed, 400 insertions(+), 225 deletions(-)
On Thu, Jan 31, 2013 at 04:08:36PM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 16:04 +0100, Johannes Berg wrote:
> > On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> >
> > > Johannes: I have a couple of comments/questions for you related to these
> > > patches.
> > >
> > > First, in the patches I've added an offchan_tx_ok argument to the tx
> > > operations, but this seems a little awkward to me since it has to be
> > > propogated down through a fairly deep call stack. The alternative idea
> > > that occurred to me is to use a tx control flag, but that seems to be
> > > pretty crowded. Any thoughts?
> >
> > Maybe you can bypass by using a flag in struct ieee80211_tx_data, so
> > only the first few functions in the call chain need the argument?
> > Otherwise, I guess adding a flag should be OK. I know it's crowded, but
> > if we really run out I guess we could move all the internal flags etc.
> > wholesale ...
>
> Ok no that was wrong ... we can't do that because many flags need to
> survive queueing.
An ieee80211_tx_data flag would work for this case, though it doesn't
quite have the effect I was hoping for since it really only gets rid of
the argument internal to tx.c. I avoided most of the pain by leaving
ieee80211_tx_skb() unmodified, but it all still seems a bit ugly.
If you're okay with the way I've got things now I'll just stick with it.
I hate to gobble up valuable real estate in the tx control flags just to
satisfy my sense of aesthetics ;-)
Seth
On Thu, 2013-01-31 at 10:33 -0600, Seth Forshee wrote:
> On Thu, Jan 31, 2013 at 04:20:48PM +0100, Johannes Berg wrote:
> > On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> >
> > > +static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
> >
> > > +static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
> >
> > Huh, is that worth the confusion? It seems !enabled should be the same
> > as disabled, but it's not quite the same, which might be confusing.
>
> In this patch there's no distinction, but after adding the off-channel
> powersave state there is -- disabled == !enabled && !offchannel.
I thought it was something like that, yeah.
> Actually one of the last bugs I fixed before sending these was a place
> where I had used disabled instead of !enabled, and the frames ended up
> with PM set when it shouldn't have been.
>
> I agree though that the distinction is confusing. Maybe some better
> state names are needed. Perhaps awake, offchannel, and doze?
I think what you really want is to distinguish between "HW can go to
powersave" and "PM bit should be set"? That's pretty much what your
CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe
putting it in different terms would make it less confusing?
> > > +/**
> > > + * ieee80211_set_ps_state - set device powersave state
> > > + *
> > > + * Sets the powersave state in the supplied device configuration to the
> > > + * specified state.
> > > + *
> > > + * @conf: device configuration
> > > + * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_*
> > > + * flags from enum ieee80211_conf_flags.
> > > + */
> > > +static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf,
> > > + u32 state)
> > > +{
> > > + conf->flags = (conf->flags & ~IEEE80211_CONF_PS_MASK) |
> > > + (state & IEEE80211_CONF_PS_MASK);
> > > +}
> >
> > I don't think the driver should do this, so the inline shouldn't be
> > here?
>
> That's true. Would moving it to ieee80211_i.h be appropriate, or is
> there somewhere better?
ieee80211_i.h is good
johannes
On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> +static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
> +static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
Huh, is that worth the confusion? It seems !enabled should be the same
as disabled, but it's not quite the same, which might be confusing.
> +/**
> + * ieee80211_set_ps_state - set device powersave state
> + *
> + * Sets the powersave state in the supplied device configuration to the
> + * specified state.
> + *
> + * @conf: device configuration
> + * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_*
> + * flags from enum ieee80211_conf_flags.
> + */
> +static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf,
> + u32 state)
> +{
> + conf->flags = (conf->flags & ~IEEE80211_CONF_PS_MASK) |
> + (state & IEEE80211_CONF_PS_MASK);
> +}
I don't think the driver should do this, so the inline shouldn't be
here?
johannes
Scans currently work by stopping the netdev tx queues but leaving the
mac80211 queues active. This stops the flow of incoming packets while
still allowing mac80211 to transmit nullfunc and probe request frames
to facilitate scanning. However, the driver may try to wake the
mac80211 queues while in this state, which will also wake the netdev
queues.
To prevent this, add a new queue stop reason,
IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL, to be used when stopping the tx
queues for off-channel operation. This prevents the netdev queues from
waking when a driver wakes the mac80211 queues.
This also stops all frames from being transmitted, even those required
for scanning. To get around this, add a new offchan_tx_ok argument to
most of the tx interfaces. This flag can be set for frames which need
to be transmitted during off-channel operation, allowing off-channel
frames to always be passed down to the driver as long as the driver
hasn't stopped the queues. Add ieee80211_tx_skb_offchannel() for
transmitting off-channel frames with this flag set.
Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/agg-tx.c | 4 ++--
net/mac80211/cfg.c | 2 +-
net/mac80211/ht.c | 2 +-
net/mac80211/ieee80211_i.h | 25 ++++++++++++++++++-------
net/mac80211/mlme.c | 2 +-
net/mac80211/offchannel.c | 32 +++++++++++---------------------
net/mac80211/sta_info.c | 2 +-
net/mac80211/tx.c | 44 +++++++++++++++++++++++++++++---------------
net/mac80211/util.c | 3 ++-
9 files changed, 66 insertions(+), 50 deletions(-)
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 2f0ccbc..3c836fc 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -107,7 +107,7 @@ static void ieee80211_send_addba_request(struct ieee80211_sub_if_data *sdata,
mgmt->u.action.u.addba_req.start_seq_num =
cpu_to_le16(start_seq_num << 4);
- ieee80211_tx_skb_tid(sdata, skb, tid);
+ ieee80211_tx_skb_tid(sdata, skb, tid, false);
}
void ieee80211_send_bar(struct ieee80211_vif *vif, u8 *ra, u16 tid, u16 ssn)
@@ -137,7 +137,7 @@ void ieee80211_send_bar(struct ieee80211_vif *vif, u8 *ra, u16 tid, u16 ssn)
IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_INTFL_DONT_ENCRYPT |
IEEE80211_TX_CTL_REQ_TX_STATUS;
- ieee80211_tx_skb_tid(sdata, skb, tid);
+ ieee80211_tx_skb_tid(sdata, skb, tid, false);
}
EXPORT_SYMBOL(ieee80211_send_bar);
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 661b878..fd38c37 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3211,7 +3211,7 @@ static int ieee80211_probe_client(struct wiphy *wiphy, struct net_device *dev,
nullfunc->qos_ctrl = cpu_to_le16(7);
local_bh_disable();
- ieee80211_xmit(sdata, skb, band);
+ ieee80211_xmit(sdata, skb, band, false);
local_bh_enable();
rcu_read_unlock();
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index 61ac7c4..da61c41 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -311,7 +311,7 @@ void ieee80211_send_delba(struct ieee80211_sub_if_data *sdata,
mgmt->u.action.u.delba.params = cpu_to_le16(params);
mgmt->u.action.u.delba.reason_code = cpu_to_le16(reason_code);
- ieee80211_tx_skb_tid(sdata, skb, tid);
+ ieee80211_tx_skb_tid(sdata, skb, tid, false);
}
void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 13fd13a..8374763 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -842,6 +842,7 @@ enum queue_stop_reason {
IEEE80211_QUEUE_STOP_REASON_AGGREGATION,
IEEE80211_QUEUE_STOP_REASON_SUSPEND,
IEEE80211_QUEUE_STOP_REASON_SKB_ADD,
+ IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL,
};
#ifdef CONFIG_MAC80211_LEDS
@@ -1510,22 +1511,23 @@ enum ieee80211_tx_status {
enum ieee80211_tx_status
ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
- enum ieee80211_band band);
+ enum ieee80211_band band, bool offchan_tx_ok);
enum ieee80211_tx_status
__ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
struct sk_buff *skb, int tid,
- enum ieee80211_band band);
+ enum ieee80211_band band, bool offchan_tx_ok);
static inline enum ieee80211_tx_status
ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
struct sk_buff *skb, int tid,
- enum ieee80211_band band)
+ enum ieee80211_band band, bool offchan_tx_ok)
{
enum ieee80211_tx_status ret;
rcu_read_lock();
- ret = __ieee80211_tx_skb_tid_band(sdata, skb, tid, band);
+ ret = __ieee80211_tx_skb_tid_band(sdata, skb, tid, band,
+ offchan_tx_ok);
rcu_read_unlock();
return ret;
@@ -1533,7 +1535,7 @@ ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
static inline enum ieee80211_tx_status
ieee80211_tx_skb_tid(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
- int tid)
+ int tid, bool offchan_tx_ok)
{
struct ieee80211_chanctx_conf *chanctx_conf;
enum ieee80211_tx_status ret;
@@ -1547,7 +1549,8 @@ ieee80211_tx_skb_tid(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
}
ret = __ieee80211_tx_skb_tid_band(sdata, skb, tid,
- chanctx_conf->def.chan->band);
+ chanctx_conf->def.chan->band,
+ offchan_tx_ok);
rcu_read_unlock();
return ret;
@@ -1557,7 +1560,15 @@ static inline enum ieee80211_tx_status
ieee80211_tx_skb(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
{
/* Send all internal mgmt frames on VO. Accordingly set TID to 7. */
- return ieee80211_tx_skb_tid(sdata, skb, 7);
+ return ieee80211_tx_skb_tid(sdata, skb, 7, false);
+}
+
+static inline enum ieee80211_tx_status
+ieee80211_tx_skb_offchannel(struct ieee80211_sub_if_data *sdata,
+ struct sk_buff *skb)
+{
+ /* Send all internal mgmt frames on VO. Accordingly set TID to 7. */
+ return ieee80211_tx_skb_tid(sdata, skb, 7, true);
}
void ieee802_11_parse_elems(u8 *start, size_t len,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index e930175..6757ee2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -685,7 +685,7 @@ void ieee80211_send_nullfunc(struct ieee80211_local *local,
IEEE80211_STA_CONNECTION_POLL))
IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_CTL_USE_MINRATE;
- ieee80211_tx_skb(sdata, skb);
+ ieee80211_tx_skb_offchannel(sdata, skb);
}
static void ieee80211_send_4addr_nullfunc(struct ieee80211_local *local,
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 82baf5b..5b9b3b8 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -113,6 +113,10 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local)
* notify the AP about us leaving the channel and stop all
* STA interfaces.
*/
+
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL);
+
mutex_lock(&local->iflist_mtx);
list_for_each_entry(sdata, &local->interfaces, list) {
if (!ieee80211_sdata_running(sdata))
@@ -133,12 +137,9 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local)
sdata, BSS_CHANGED_BEACON_ENABLED);
}
- if (sdata->vif.type != NL80211_IFTYPE_MONITOR) {
- netif_tx_stop_all_queues(sdata->dev);
- if (sdata->vif.type == NL80211_IFTYPE_STATION &&
- sdata->u.mgd.associated)
- ieee80211_offchannel_ps_enable(sdata);
- }
+ if (sdata->vif.type == NL80211_IFTYPE_STATION &&
+ sdata->u.mgd.associated)
+ ieee80211_offchannel_ps_enable(sdata);
}
mutex_unlock(&local->iflist_mtx);
}
@@ -166,20 +167,6 @@ void ieee80211_offchannel_return(struct ieee80211_local *local)
sdata->u.mgd.associated)
ieee80211_offchannel_ps_disable(sdata);
- if (sdata->vif.type != NL80211_IFTYPE_MONITOR) {
- /*
- * This may wake up queues even though the driver
- * currently has them stopped. This is not very
- * likely, since the driver won't have gotten any
- * (or hardly any) new packets while we weren't
- * on the right channel, and even if it happens
- * it will at most lead to queueing up one more
- * packet per queue in mac80211 rather than on
- * the interface qdisc.
- */
- netif_tx_wake_all_queues(sdata->dev);
- }
-
if (test_and_clear_bit(SDATA_STATE_OFFCHANNEL_BEACON_STOPPED,
&sdata->state)) {
sdata->vif.bss_conf.enable_beacon = true;
@@ -188,6 +175,9 @@ void ieee80211_offchannel_return(struct ieee80211_local *local)
}
}
mutex_unlock(&local->iflist_mtx);
+
+ ieee80211_wake_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL);
}
void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc)
@@ -198,7 +188,7 @@ void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc)
if (roc->mgmt_tx_cookie) {
if (!WARN_ON(!roc->frame)) {
ieee80211_tx_skb_tid_band(roc->sdata, roc->frame, 7,
- roc->chan->band);
+ roc->chan->band, true);
roc->frame = NULL;
}
} else {
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 9d864ed..d4e5915 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1122,7 +1122,7 @@ static void ieee80211_send_null_response(struct ieee80211_sub_if_data *sdata,
return;
}
- ieee80211_xmit(sdata, skb, chanctx_conf->def.chan->band);
+ ieee80211_xmit(sdata, skb, chanctx_conf->def.chan->band, false);
rcu_read_unlock();
}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 7d25bb6..80b514a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1203,7 +1203,7 @@ ieee80211_tx_prepare(struct ieee80211_sub_if_data *sdata,
static enum ieee80211_tx_status
ieee80211_tx_frags(struct ieee80211_local *local, struct ieee80211_vif *vif,
struct ieee80211_sta *sta, struct sk_buff_head *skbs,
- bool txpending)
+ bool txpending, bool offchan_tx_ok)
{
struct ieee80211_tx_control control;
struct sk_buff *skb, *tmp;
@@ -1222,8 +1222,20 @@ ieee80211_tx_frags(struct ieee80211_local *local, struct ieee80211_vif *vif,
#endif
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
- if (local->queue_stop_reasons[q] ||
- (!txpending && !skb_queue_empty(&local->pending[q]))) {
+ if (offchan_tx_ok) {
+ /*
+ * Always directly transmit off-channel frames
+ * unless the driver has stopped the queues.
+ */
+ if (test_bit(IEEE80211_QUEUE_STOP_REASON_DRIVER,
+ &local->queue_stop_reasons[q])) {
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock,
+ flags);
+ ieee80211_purge_tx_queue(&local->hw, skbs);
+ return IEEE80211_TX_DROPPED;
+ }
+ } else if (local->queue_stop_reasons[q] ||
+ (!txpending && !skb_queue_empty(&local->pending[q]))) {
/*
* Since queue is stopped, queue up frames for later
* transmission from the tx-pending tasklet when the
@@ -1253,7 +1265,8 @@ ieee80211_tx_frags(struct ieee80211_local *local, struct ieee80211_vif *vif,
static enum ieee80211_tx_status
__ieee80211_tx(struct ieee80211_local *local, struct sk_buff_head *skbs,
- int led_len, struct sta_info *sta, bool txpending)
+ int led_len, struct sta_info *sta, bool txpending,
+ bool offchan_tx_ok)
{
struct ieee80211_tx_info *info;
struct ieee80211_sub_if_data *sdata;
@@ -1301,7 +1314,7 @@ __ieee80211_tx(struct ieee80211_local *local, struct sk_buff_head *skbs,
}
result = ieee80211_tx_frags(local, vif, pubsta, skbs,
- txpending);
+ txpending, offchan_tx_ok);
ieee80211_tpt_led_trig_tx(local, fc, led_len);
ieee80211_led_tx(local, 1);
@@ -1369,7 +1382,7 @@ static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
static enum ieee80211_tx_status
ieee80211_tx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
- bool txpending, enum ieee80211_band band)
+ bool txpending, enum ieee80211_band band, bool offchan_tx_ok)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_tx_data tx;
@@ -1412,7 +1425,7 @@ ieee80211_tx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
if (!invoke_tx_handlers(&tx))
result = __ieee80211_tx(local, &tx.skbs, led_len,
- tx.sta, txpending);
+ tx.sta, txpending, offchan_tx_ok);
return result;
}
@@ -1450,7 +1463,7 @@ static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata,
enum ieee80211_tx_status
ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
- enum ieee80211_band band)
+ enum ieee80211_band band, bool offchan_tx_ok)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
@@ -1483,7 +1496,7 @@ ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
}
ieee80211_set_qos_hdr(sdata, skb);
- return ieee80211_tx(sdata, skb, false, band);
+ return ieee80211_tx(sdata, skb, false, band, offchan_tx_ok);
}
static bool ieee80211_parse_tx_radiotap(struct sk_buff *skb)
@@ -1703,7 +1716,7 @@ netdev_tx_t ieee80211_monitor_start_xmit(struct sk_buff *skb,
IEEE80211_CHAN_PASSIVE_SCAN)))
goto fail_rcu;
- ieee80211_xmit(sdata, skb, chan->band);
+ ieee80211_xmit(sdata, skb, chan->band, false);
rcu_read_unlock();
return NETDEV_TX_OK;
@@ -2147,7 +2160,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
info->flags = info_flags;
info->ack_frame_id = info_id;
- ieee80211_xmit(sdata, skb, band);
+ ieee80211_xmit(sdata, skb, band, false);
rcu_read_unlock();
return NETDEV_TX_OK;
@@ -2198,7 +2211,7 @@ ieee80211_tx_pending_skb(struct ieee80211_local *local, struct sk_buff *skb)
return IEEE80211_TX_DROPPED;
}
tx_stat = ieee80211_tx(sdata, skb, true,
- chanctx_conf->def.chan->band);
+ chanctx_conf->def.chan->band, false);
} else {
struct sk_buff_head skbs;
@@ -2208,7 +2221,8 @@ ieee80211_tx_pending_skb(struct ieee80211_local *local, struct sk_buff *skb)
hdr = (struct ieee80211_hdr *)skb->data;
sta = sta_info_get(sdata, hdr->addr1);
- tx_stat = __ieee80211_tx(local, &skbs, skb->len, sta, true);
+ tx_stat = __ieee80211_tx(local, &skbs, skb->len, sta, true,
+ false);
}
return tx_stat;
@@ -2779,7 +2793,7 @@ EXPORT_SYMBOL(ieee80211_get_buffered_bc);
enum ieee80211_tx_status
__ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
struct sk_buff *skb, int tid,
- enum ieee80211_band band)
+ enum ieee80211_band band, bool offchan_tx_ok)
{
int ac = ieee802_1d_to_ac[tid & 7];
enum ieee80211_tx_status ret;
@@ -2797,7 +2811,7 @@ __ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
* requirements are that we do not come into tx with bhs on.
*/
local_bh_disable();
- ret = ieee80211_xmit(sdata, skb, band);
+ ret = ieee80211_xmit(sdata, skb, band, offchan_tx_ok);
local_bh_enable();
return ret;
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 7519018..5259557 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1290,7 +1290,8 @@ void ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
IEEE80211_SKB_CB(skb)->flags |=
IEEE80211_TX_CTL_NO_CCK_RATE;
if (scan)
- ieee80211_tx_skb_tid_band(sdata, skb, 7, channel->band);
+ ieee80211_tx_skb_tid_band(sdata, skb, 7,
+ channel->band, true);
else
ieee80211_tx_skb(sdata, skb);
}
--
1.7.9.5
On Thu, 2013-01-31 at 11:18 -0600, Seth Forshee wrote:
> > > Actually one of the last bugs I fixed before sending these was a place
> > > where I had used disabled instead of !enabled, and the frames ended up
> > > with PM set when it shouldn't have been.
> > >
> > > I agree though that the distinction is confusing. Maybe some better
> > > state names are needed. Perhaps awake, offchannel, and doze?
> >
> > I think what you really want is to distinguish between "HW can go to
> > powersave" and "PM bit should be set"? That's pretty much what your
> > CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe
>
> Correct, with the understanding that "HW can go to powersave" also
> implies "PM bit should be set."
>
> Another approach would be to keep the CONF_PS flag the same and add a
> CONF_PM flag or similar. I didn't go with this approach because CONF_PS
> && !CONF_PM really doesn't make any sense, which really doesn't help
> with reducing confusion. The advantage is that it separates setting PM
> from PS for those driver that don't support PS but need to configure the
> hardware to set PM for off-channel.
Good point, that'd work too. PS && !PM would never be used, I guess?
It'd also have the advantage of not having to touch all the drivers?
It doesn't really matter all that much to me though, I just think what
you have right now is (too) confusing.
johannes
On Thu, Jan 31, 2013 at 04:14:05PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
>
> > + if (offchan_tx_ok) {
>
> unlikely(), this is one of the hottest code paths in mac80211 after
> all :-)
Ack, I'll change this for v2.
> > + /*
> > + * Always directly transmit off-channel frames
> > + * unless the driver has stopped the queues.
> > + */
> > + if (test_bit(IEEE80211_QUEUE_STOP_REASON_DRIVER,
> > + &local->queue_stop_reasons[q])) {
>
> might there be other reasons to TX, i.e. ignore only
> STOP_REASON_OFFCHANNEL?
I originally wrote it that way, but I'm not sure that all of the stop
reasons need to block tx here. I'll take another look.
> That might be more efficient too:
>
> offchflag = offchan_tx_ok << log2(STOP_REASON_OFFCHANNEL);
>
> > + } else if (local->queue_stop_reasons[q] ||
>
> if (local->queue_stop_reasons[q] & ~offchflag || ...
>
> OTOH, I guess you want a different return value too. But that's in a
> relatively unlikely code path again, so might still be better to then
> again differentiate within that if.
Good suggestions, I'll take a look at applying them in some form.
Seth
On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> + if (offchan_tx_ok) {
unlikely(), this is one of the hottest code paths in mac80211 after
all :-)
> + /*
> + * Always directly transmit off-channel frames
> + * unless the driver has stopped the queues.
> + */
> + if (test_bit(IEEE80211_QUEUE_STOP_REASON_DRIVER,
> + &local->queue_stop_reasons[q])) {
might there be other reasons to TX, i.e. ignore only
STOP_REASON_OFFCHANNEL?
That might be more efficient too:
offchflag = offchan_tx_ok << log2(STOP_REASON_OFFCHANNEL);
> + } else if (local->queue_stop_reasons[q] ||
if (local->queue_stop_reasons[q] & ~offchflag || ...
OTOH, I guess you want a different return value too. But that's in a
relatively unlikely code path again, so might still be better to then
again differentiate within that if.
johannes
On Thu, Jan 31, 2013 at 04:20:48PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
>
> > +static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
>
> > +static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
>
> Huh, is that worth the confusion? It seems !enabled should be the same
> as disabled, but it's not quite the same, which might be confusing.
In this patch there's no distinction, but after adding the off-channel
powersave state there is -- disabled == !enabled && !offchannel.
Actually one of the last bugs I fixed before sending these was a place
where I had used disabled instead of !enabled, and the frames ended up
with PM set when it shouldn't have been.
I agree though that the distinction is confusing. Maybe some better
state names are needed. Perhaps awake, offchannel, and doze?
> > +/**
> > + * ieee80211_set_ps_state - set device powersave state
> > + *
> > + * Sets the powersave state in the supplied device configuration to the
> > + * specified state.
> > + *
> > + * @conf: device configuration
> > + * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_*
> > + * flags from enum ieee80211_conf_flags.
> > + */
> > +static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf,
> > + u32 state)
> > +{
> > + conf->flags = (conf->flags & ~IEEE80211_CONF_PS_MASK) |
> > + (state & IEEE80211_CONF_PS_MASK);
> > +}
>
> I don't think the driver should do this, so the inline shouldn't be
> here?
That's true. Would moving it to ieee80211_i.h be appropriate, or is
there somewhere better?
Seth
Some driviers need to perform hardware configuration or other activities
in order to properly support off-channel powersave. Add an off-channel
powersave state to inform drivers that MAC-level support for powersave
is required while keeping the hardware powered on for off-channel
operation, and update the documentation with informtion about this
state.
Also modify the conditions for sending nullfunc frames from mac80211 for
off-channel operation. Hardware which supports sending the frames will
now have accurate information about the powersave state and should no
longer require that mac80211 generate the frames. Continue sending them
for all drivers which do not support powersave, however.
Signed-off-by: Seth Forshee <[email protected]>
---
include/net/mac80211.h | 29 ++++++++++++++++++++++++
net/mac80211/offchannel.c | 55 +++++++++++++++++++++------------------------
2 files changed, 55 insertions(+), 29 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d6c24d9..eb5d5e9 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -871,6 +871,11 @@ struct ieee80211_rx_status {
* up for beacons, is able to transmit frames and receive the possible
* acknowledgement frames. Not to be confused with hardware specific
* wakeup/sleep states; the driver is responsible for that.
+ * @IEEE80211_CONF_PS_OFFCHANNEL: Power save is enabled at the AP, but the
+ * hardware remains powered on for transmitting and receiving frames. This
+ * is used to request that the AP buffer frames while the STA leaves the
+ * operating channel. The hardware must be configured to set PM in
+ * frame control, if applicable.
* @IEEE80211_CONF_IDLE: The device is running, but idle; if the flag is set
* the driver should be prepared to handle configuration requests but
* may turn the device off as much as possible. Typically, this flag will
@@ -884,6 +889,7 @@ enum ieee80211_conf_flags {
IEEE80211_CONF_PS_MASK = (3<<1),
IEEE80211_CONF_PS_DISABLED = (0<<1),
IEEE80211_CONF_PS_ENABLED = (1<<1),
+ IEEE80211_CONF_PS_OFFCHANNEL = (2<<1),
IEEE80211_CONF_IDLE = (1<<3),
IEEE80211_CONF_OFFCHANNEL = (1<<4),
};
@@ -1013,6 +1019,20 @@ static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
}
/**
+ * ieee80211_is_ps_offchannel - check if off-channel powersave is enabled
+ *
+ * Returns true if off-channel powersave is enabled in the supplied
+ * configuration.
+ *
+ * @conf: device configuration
+ */
+static inline bool ieee80211_is_ps_offchannel(struct ieee80211_conf *conf)
+{
+ return (conf->flags & IEEE80211_CONF_PS_MASK) ==
+ IEEE80211_CONF_PS_OFFCHANNEL;
+}
+
+/**
* ieee80211_set_ps_state - set device powersave state
*
* Sets the powersave state in the supplied device configuration to the
@@ -1712,6 +1732,15 @@ void ieee80211_free_txskb(struct ieee80211_hw *hw, struct sk_buff *skb);
* waking up for multicast traffic; if it cannot the driver must handle that
* as best as it can, mac80211 is too slow to do that.
*
+ * If ieee80211_is_ps_offchannel() returns true, off-channel powersave is
+ * enabled. During off-channel powersave the AP is instructed to buffer frames
+ * as if the hardware is in powersave, but the hardware must remain fully
+ * operational to support off-channel operation. If necessary the hardware
+ * must be configured to set PM in frame control for frames destined for the
+ * AP. Drivers will be told about entering and leaving off-channel powersave
+ * even if %IEEE80211_HW_SUPPORTS_PS is cleared; drivers which do not need to
+ * do any configuration can safely ignore it.
+ *
* Dynamic powersave is an extension to normal powersave in which the
* hardware stays awake for a user-specified period of time after sending a
* frame so that reply frames need not be buffered and therefore delayed to
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index d524794..5ee2ea8 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -40,25 +40,20 @@ static bool ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
cancel_work_sync(&local->dynamic_ps_enable_work);
- if (ieee80211_is_ps_enabled(&local->hw.conf)) {
+ if (ieee80211_is_ps_enabled(&local->hw.conf))
local->offchannel_ps_enabled = true;
- ieee80211_set_ps_state(&local->hw.conf,
- IEEE80211_CONF_PS_DISABLED);
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
- }
- if (!local->offchannel_ps_enabled ||
- !(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK))
- /*
- * If power save was enabled, no need to send a nullfunc
- * frame because AP knows that we are sleeping. But if the
- * hardware is creating the nullfunc frame for power save
- * status (ie. IEEE80211_HW_PS_NULLFUNC_STACK is not
- * enabled) and power save was enabled, the firmware just
- * sent a null frame with power save disabled. So we need
- * to send a new nullfunc frame to inform the AP that we
- * are again sleeping.
- */
+ /*
+ * Change powersave state even if the driver doesn't declare
+ * powersave support, as drivers still may require some
+ * configuration for off-channel powersave
+ */
+ ieee80211_set_ps_state(&local->hw.conf, IEEE80211_CONF_PS_OFFCHANNEL);
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+
+ if (!(local->hw.flags & IEEE80211_HW_SUPPORTS_PS) ||
+ (!local->offchannel_ps_enabled &&
+ local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK))
ret = ieee80211_send_nullfunc(local, sdata, 1);
return ret;
@@ -69,9 +64,7 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
{
struct ieee80211_local *local = sdata->local;
- if (!local->ps_sdata)
- ieee80211_send_nullfunc(local, sdata, 0);
- else if (local->offchannel_ps_enabled) {
+ if (local->offchannel_ps_enabled) {
/*
* In !IEEE80211_HW_PS_NULLFUNC_STACK case the hardware
* will send a nullfunc frame with the powersave bit set
@@ -85,22 +78,26 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
* we are sleeping, let's just enable power save mode in
* hardware.
*/
- /* TODO: Only set hardware if CONF_PS changed?
- * TODO: Should we set offchannel_ps_enabled to false?
- */
+ /* TODO: Should we set offchannel_ps_enabled to false? */
ieee80211_set_ps_state(&local->hw.conf,
IEEE80211_CONF_PS_ENABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
- } else if (local->hw.conf.dynamic_ps_timeout > 0) {
+ } else {
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_DISABLED);
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ if (!(local->hw.flags & IEEE80211_HW_SUPPORTS_PS) ||
+ local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)
+ ieee80211_send_nullfunc(local, sdata, 0);
+
/*
* If power save was not enabled and the dynamic_ps_timer
* had been running before leaving the operating channel,
- * restart the timer now and send a nullfunc frame to inform
- * the AP that we are awake.
+ * restart the timer now.
*/
- ieee80211_send_nullfunc(local, sdata, 0);
- mod_timer(&local->dynamic_ps_timer, jiffies +
- msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
+ if (local->hw.conf.dynamic_ps_timeout > 0)
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
}
ieee80211_sta_reset_beacon_monitor(sdata);
--
1.7.9.5
On Thu, Jan 31, 2013 at 04:04:24PM +0100, Johannes Berg wrote:
> > Second, I attempted to test these patches with iwlwifi (Centrino
> > Advanced-N 6235) to verify that I didn't break anything for drivers with
> > hw scanning. My standard test for this involves running iperf while
> > triggering nearly continuous scans, but I'm seeing lots of problems
> > running a tcp iperf test even with unpatched 3.8-rc4. iperf with udp
> > does fine in either direction. I haven't had time to do any kind of
> > debugging yet, but I thought you'd want to know.
>
> Hm, ok, thanks.
I just tried moving this card to a different slot in the same machine,
and it's working fine. Hardware problem I guess. Sorry for the false
alarm.
Seth
On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> Johannes: I have a couple of comments/questions for you related to these
> patches.
>
> First, in the patches I've added an offchan_tx_ok argument to the tx
> operations, but this seems a little awkward to me since it has to be
> propogated down through a fairly deep call stack. The alternative idea
> that occurred to me is to use a tx control flag, but that seems to be
> pretty crowded. Any thoughts?
Maybe you can bypass by using a flag in struct ieee80211_tx_data, so
only the first few functions in the call chain need the argument?
Otherwise, I guess adding a flag should be OK. I know it's crowded, but
if we really run out I guess we could move all the internal flags etc.
wholesale ...
> Second, I attempted to test these patches with iwlwifi (Centrino
> Advanced-N 6235) to verify that I didn't break anything for drivers with
> hw scanning. My standard test for this involves running iperf while
> triggering nearly continuous scans, but I'm seeing lots of problems
> running a tcp iperf test even with unpatched 3.8-rc4. iperf with udp
> does fine in either direction. I haven't had time to do any kind of
> debugging yet, but I thought you'd want to know.
Hm, ok, thanks.
johannes
On Wed, Jan 30, 2013 at 10:27:25PM +0100, Arend van Spriel wrote:
> On 01/30/2013 12:47 AM, Seth Forshee wrote:
> > This patch series fixes a number of problems observed during software
> > scanning, as described in [1].
>
> Hi Seth,
>
> Not sure why but it seems I only received patch 5, 7, and the cover
> letter. Could you resend them to me so I can give it a spin over here?
I tried to automate who was on Cc by using get_maintainer.pl, so
probably those are the only ones it said you needed to receive ;-)
All the patches should be on the linux-wireless list, but I'll send you
a private mail with all the patches as well.
Seth
On Wed, Jan 30, 2013 at 10:56:03AM +1100, Julian Calaby wrote:
> Hi Seth,
>
> On Wed, Jan 30, 2013 at 10:47 AM, Seth Forshee
> <[email protected]> wrote:
> > Broadcom hardware sets the value of the PM flag in frame control based
> > on the value of MCTL_HPS, which seems to include actively clearing PM
> > if MCTL_HPS is not set. brcmsmac needs to suppor the off-channel
> > powersave state in order to enable powersave at the AP at all.
> >
> > Add limited support for powersave to brcmsmac to prevent frame loss
> > during background scans. Full powersave supoprt remains unimplemented,
> > but switching between the off-channel and disabled states is possible.
> >
> > Signed-off-by: Seth Forshee <[email protected]>
> > ---
> > .../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 15 +++++++++++----
> > drivers/net/wireless/brcm80211/brcmsmac/main.c | 9 +++++++++
> > drivers/net/wireless/brcm80211/brcmsmac/pub.h | 1 +
> > 3 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> > index c26992a..3fb71c9 100644
> > --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
> > +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> > @@ -7545,6 +7549,11 @@ void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc, u8 interval)
> > brcms_c_bcn_li_upd(wlc);
> > }
> >
> > +void brcms_c_set_ps(struct brcms_c_info *wlc)
> > +{
> > + brcms_c_set_ps_ctrl(wlc);
> > +}
> > +
>
> Why not just use brcms_c_set_ps_ctrl() directly?
It's a remnant from an earlier version of the patches. Looks like I just
didn't take things to the logical conclusion when I refactored the code.
Seth
Broadcom hardware sets the value of the PM flag in frame control based
on the value of MCTL_HPS, which seems to include actively clearing PM
if MCTL_HPS is not set. brcmsmac needs to suppor the off-channel
powersave state in order to enable powersave at the AP at all.
Add limited support for powersave to brcmsmac to prevent frame loss
during background scans. Full powersave supoprt remains unimplemented,
but switching between the off-channel and disabled states is possible.
Signed-off-by: Seth Forshee <[email protected]>
---
.../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 15 +++++++++++----
drivers/net/wireless/brcm80211/brcmsmac/main.c | 9 +++++++++
drivers/net/wireless/brcm80211/brcmsmac/pub.h | 1 +
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
index be3820e..408eb94 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
@@ -393,10 +393,17 @@ static int brcms_ops_config(struct ieee80211_hw *hw, u32 changed)
brcms_dbg_info(core, "%s: change monitor mode: %s\n",
__func__, conf->flags & IEEE80211_CONF_MONITOR ?
"true" : "false");
- if (changed & IEEE80211_CONF_CHANGE_PS)
- brcms_err(core, "%s: change power-save mode: %s (implement)\n",
- __func__, ieee80211_is_ps_enabled(conf) ?
- "true" : "false");
+ if (changed & IEEE80211_CONF_CHANGE_PS) {
+ /*
+ * brcmsmac doesn't support powersave, but it does support
+ * setting the PM bit in frame control for off-channel PS
+ */
+ if (ieee80211_is_ps_enabled(conf))
+ brcms_err(core, "%s: cannot enable power-save mode (implement)\n",
+ __func__);
+ else
+ brcms_c_set_ps(wl->wlc);
+ }
if (changed & IEEE80211_CONF_CHANGE_POWER) {
err = brcms_c_set_tx_power(wl->wlc, conf->power_level);
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index c26992a..3fb71c9 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -3054,6 +3054,10 @@ static bool brcms_c_ps_allowed(struct brcms_c_info *wlc)
{
struct brcms_bss_cfg *cfg = wlc->bsscfg;
+ /* allow PS when off-channel PS is enabled */
+ if (ieee80211_is_ps_offchannel(&wlc->pub->ieee_hw->conf))
+ return true;
+
/* disallow PS when one of the following global conditions meets */
if (!wlc->pub->associated)
return false;
@@ -7545,6 +7549,11 @@ void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc, u8 interval)
brcms_c_bcn_li_upd(wlc);
}
+void brcms_c_set_ps(struct brcms_c_info *wlc)
+{
+ brcms_c_set_ps_ctrl(wlc);
+}
+
int brcms_c_set_tx_power(struct brcms_c_info *wlc, int txpwr)
{
uint qdbm;
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/pub.h b/drivers/net/wireless/brcm80211/brcmsmac/pub.h
index 4fb2834..5482fae 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/pub.h
+++ b/drivers/net/wireless/brcm80211/brcmsmac/pub.h
@@ -328,6 +328,7 @@ extern void brcms_c_set_shortslot_override(struct brcms_c_info *wlc,
s8 sslot_override);
extern void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc,
u8 interval);
+extern void brcms_c_set_ps(struct brcms_c_info *wlc);
extern int brcms_c_set_tx_power(struct brcms_c_info *wlc, int txpwr);
extern int brcms_c_get_tx_power(struct brcms_c_info *wlc);
extern bool brcms_c_check_radio_disabled(struct brcms_c_info *wlc);
--
1.7.9.5
On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> Errors in sending nullfunc or probe request frames during off-channel
> operation may have undesirable consequences, e.g. failure to set
> powersave at the AP. Add error handling for failures to transmit these
> frames. In the case of a nullfunc failure, fail to go off-channel and
> return an error to userspace. In the case of a failed probe request,
> abort the scan.
That latter part seems excessive? Maybe increase the time to use a
passive scan? But if there are multiple scan requests ...
Is all of this really worth the effort? The driver queues should be
empty after the flush, after all, and the driver doesn't return any TX
status. So what can really happen?
johannes
On 01/30/2013 12:47 AM, Seth Forshee wrote:
> This patch series fixes a number of problems observed during software
> scanning, as described in [1].
Hi Seth,
Not sure why but it seems I only received patch 5, 7, and the cover
letter. Could you resend them to me so I can give it a spin over here?
Gr. AvS
The powersave configuration flag is a single bit and can thus only
indicate that powersave is either on or off. This is inadequate for
off-channel powersave, where the hardware must remain powered on but
some configuration of the hardware may be required (e.g. to ensure that
PM is set in frame control in frames sent to the associated AP, as with
brcmsmac).
In preparation for supporting an off-channel powersave state, expand the
powersave configuration flag to two bits. Also introduce some interfaces
which functionally replace the current open-coded setting and checking
of the power state and update mac80211 and the drivers to use these
interfaces. Update the documentation to refer to the new interfaces.
Signed-off-by: Seth Forshee <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +-
drivers/net/wireless/ath/ath9k/main.c | 2 +-
drivers/net/wireless/ath/carl9170/main.c | 2 +-
drivers/net/wireless/ath/carl9170/rx.c | 2 +-
.../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 2 +-
drivers/net/wireless/iwlwifi/dvm/power.c | 2 +-
drivers/net/wireless/mac80211_hwsim.c | 2 +-
drivers/net/wireless/p54/fwio.c | 2 +-
drivers/net/wireless/p54/txrx.c | 2 +-
drivers/net/wireless/rt2x00/rt2400pci.c | 3 +-
drivers/net/wireless/rt2x00/rt2500pci.c | 3 +-
drivers/net/wireless/rt2x00/rt2500usb.c | 3 +-
drivers/net/wireless/rt2x00/rt2800lib.c | 3 +-
drivers/net/wireless/rt2x00/rt2x00config.c | 4 +-
drivers/net/wireless/rt2x00/rt2x00dev.c | 2 +-
drivers/net/wireless/rt2x00/rt61pci.c | 3 +-
drivers/net/wireless/rt2x00/rt73usb.c | 3 +-
drivers/net/wireless/rtlwifi/core.c | 2 +-
drivers/net/wireless/rtlwifi/ps.c | 4 +-
drivers/net/wireless/ti/wl1251/main.c | 7 +-
drivers/net/wireless/ti/wlcore/main.c | 6 +-
include/net/mac80211.h | 96 +++++++++++++++-----
net/mac80211/mlme.c | 26 +++---
net/mac80211/offchannel.c | 10 +-
net/mac80211/tx.c | 2 +-
net/mac80211/util.c | 2 +-
26 files changed, 123 insertions(+), 74 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index a8016d7..85f84fa 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -1212,7 +1212,7 @@ static int ath9k_htc_config(struct ieee80211_hw *hw, u32 changed)
}
if (changed & IEEE80211_CONF_CHANGE_PS) {
- if (conf->flags & IEEE80211_CONF_PS) {
+ if (ieee80211_is_ps_enabled(conf)) {
ath9k_htc_setpower(priv, ATH9K_PM_NETWORK_SLEEP);
priv->ps_enabled = true;
} else {
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 4b72b66..a4f2151 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1180,7 +1180,7 @@ static int ath9k_config(struct ieee80211_hw *hw, u32 changed)
if (changed & IEEE80211_CONF_CHANGE_PS) {
unsigned long flags;
spin_lock_irqsave(&sc->sc_pm_lock, flags);
- if (conf->flags & IEEE80211_CONF_PS)
+ if (ieee80211_is_ps_enabled(conf))
ath9k_enable_ps(sc);
else
ath9k_disable_ps(sc);
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index ef82751..80336e3d 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -836,7 +836,7 @@ static int carl9170_ps_update(struct ar9170 *ar)
int err = 0;
if (!ar->ps.off_override)
- ps = (ar->hw->conf.flags & IEEE80211_CONF_PS);
+ ps = ieee80211_is_ps_enabled(&ar->hw->conf);
if (ps != ar->ps.state) {
err = carl9170_powersave(ar, ps);
diff --git a/drivers/net/wireless/ath/carl9170/rx.c b/drivers/net/wireless/ath/carl9170/rx.c
index 4684dd9..424d856 100644
--- a/drivers/net/wireless/ath/carl9170/rx.c
+++ b/drivers/net/wireless/ath/carl9170/rx.c
@@ -524,7 +524,7 @@ static void carl9170_ps_beacon(struct ar9170 *ar, void *data, unsigned int len)
u8 tim_len;
bool cam;
- if (likely(!(ar->hw->conf.flags & IEEE80211_CONF_PS)))
+ if (likely(ieee80211_is_ps_disabled(&ar->hw->conf)))
return;
/* check if this really is a beacon */
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
index 7fc49ca..be3820e 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
@@ -395,7 +395,7 @@ static int brcms_ops_config(struct ieee80211_hw *hw, u32 changed)
"true" : "false");
if (changed & IEEE80211_CONF_CHANGE_PS)
brcms_err(core, "%s: change power-save mode: %s (implement)\n",
- __func__, conf->flags & IEEE80211_CONF_PS ?
+ __func__, ieee80211_is_ps_enabled(conf) ?
"true" : "false");
if (changed & IEEE80211_CONF_CHANGE_POWER) {
diff --git a/drivers/net/wireless/iwlwifi/dvm/power.c b/drivers/net/wireless/iwlwifi/dvm/power.c
index 518cf37..deea146 100644
--- a/drivers/net/wireless/iwlwifi/dvm/power.c
+++ b/drivers/net/wireless/iwlwifi/dvm/power.c
@@ -286,7 +286,7 @@ static int iwl_set_power(struct iwl_priv *priv, struct iwl_powertable_cmd *cmd)
static void iwl_power_build_cmd(struct iwl_priv *priv,
struct iwl_powertable_cmd *cmd)
{
- bool enabled = priv->hw->conf.flags & IEEE80211_CONF_PS;
+ bool enabled = ieee80211_is_ps_enabled(&priv->hw->conf);
int dtimper;
dtimper = priv->hw->conf.ps_dtim_period ?: 1;
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index b73e497..3d27eb9 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1080,7 +1080,7 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
conf->channel ? conf->channel->center_freq : 0,
hwsim_chantypes[conf->channel_type],
!!(conf->flags & IEEE80211_CONF_IDLE),
- !!(conf->flags & IEEE80211_CONF_PS),
+ ieee80211_is_ps_enabled(conf),
smps_modes[conf->smps_mode]);
data->idle = !!(conf->flags & IEEE80211_CONF_IDLE);
diff --git a/drivers/net/wireless/p54/fwio.c b/drivers/net/wireless/p54/fwio.c
index 9ba8510..3bb650c 100644
--- a/drivers/net/wireless/p54/fwio.c
+++ b/drivers/net/wireless/p54/fwio.c
@@ -601,7 +601,7 @@ int p54_set_ps(struct p54_common *priv)
unsigned int i;
u16 mode;
- if (priv->hw->conf.flags & IEEE80211_CONF_PS &&
+ if (ieee80211_is_ps_enabled(&priv->hw->conf) &&
!priv->powersave_override)
mode = P54_PSM | P54_PSM_BEACON_TIMEOUT | P54_PSM_DTIM |
P54_PSM_CHECKSUM | P54_PSM_MCBC;
diff --git a/drivers/net/wireless/p54/txrx.c b/drivers/net/wireless/p54/txrx.c
index 12f0a34..baadb8f 100644
--- a/drivers/net/wireless/p54/txrx.c
+++ b/drivers/net/wireless/p54/txrx.c
@@ -380,7 +380,7 @@ static int p54_rx_data(struct p54_common *priv, struct sk_buff *skb)
skb_pull(skb, header_len);
skb_trim(skb, le16_to_cpu(hdr->len));
- if (unlikely(priv->hw->conf.flags & IEEE80211_CONF_PS))
+ if (unlikely(ieee80211_is_ps_enabled(&priv->hw->conf)))
p54_pspoll_workaround(priv, skb);
ieee80211_rx_irqsafe(priv->hw, skb);
diff --git a/drivers/net/wireless/rt2x00/rt2400pci.c b/drivers/net/wireless/rt2x00/rt2400pci.c
index a2d2bc2..375ec2d 100644
--- a/drivers/net/wireless/rt2x00/rt2400pci.c
+++ b/drivers/net/wireless/rt2x00/rt2400pci.c
@@ -521,8 +521,7 @@ static void rt2400pci_config_ps(struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf)
{
enum dev_state state =
- (libconf->conf->flags & IEEE80211_CONF_PS) ?
- STATE_SLEEP : STATE_AWAKE;
+ ieee80211_is_ps_enabled(libconf->conf) ? STATE_SLEEP : STATE_AWAKE;
u32 reg;
if (state == STATE_SLEEP) {
diff --git a/drivers/net/wireless/rt2x00/rt2500pci.c b/drivers/net/wireless/rt2x00/rt2500pci.c
index 9bea10f..45fa63f 100644
--- a/drivers/net/wireless/rt2x00/rt2500pci.c
+++ b/drivers/net/wireless/rt2x00/rt2500pci.c
@@ -570,8 +570,7 @@ static void rt2500pci_config_ps(struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf)
{
enum dev_state state =
- (libconf->conf->flags & IEEE80211_CONF_PS) ?
- STATE_SLEEP : STATE_AWAKE;
+ ieee80211_is_ps_enabled(libconf->conf) ? STATE_SLEEP : STATE_AWAKE;
u32 reg;
if (state == STATE_SLEEP) {
diff --git a/drivers/net/wireless/rt2x00/rt2500usb.c b/drivers/net/wireless/rt2x00/rt2500usb.c
index 6b2e1e43..92cf483 100644
--- a/drivers/net/wireless/rt2x00/rt2500usb.c
+++ b/drivers/net/wireless/rt2x00/rt2500usb.c
@@ -649,8 +649,7 @@ static void rt2500usb_config_ps(struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf)
{
enum dev_state state =
- (libconf->conf->flags & IEEE80211_CONF_PS) ?
- STATE_SLEEP : STATE_AWAKE;
+ ieee80211_is_ps_enabled(libconf->conf) ? STATE_SLEEP : STATE_AWAKE;
u16 reg;
if (state == STATE_SLEEP) {
diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index a5c694f..564ec67 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -2862,8 +2862,7 @@ static void rt2800_config_ps(struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf)
{
enum dev_state state =
- (libconf->conf->flags & IEEE80211_CONF_PS) ?
- STATE_SLEEP : STATE_AWAKE;
+ ieee80211_is_ps_enabled(libconf->conf) ? STATE_SLEEP : STATE_AWAKE;
u32 reg;
if (state == STATE_SLEEP) {
diff --git a/drivers/net/wireless/rt2x00/rt2x00config.c b/drivers/net/wireless/rt2x00/rt2x00config.c
index 49a63e9..bebe0b2 100644
--- a/drivers/net/wireless/rt2x00/rt2x00config.c
+++ b/drivers/net/wireless/rt2x00/rt2x00config.c
@@ -261,7 +261,7 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
if (test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags) &&
test_bit(REQUIRE_PS_AUTOWAKE, &rt2x00dev->cap_flags) &&
(ieee80211_flags & IEEE80211_CONF_CHANGE_PS) &&
- (conf->flags & IEEE80211_CONF_PS)) {
+ ieee80211_is_ps_enabled(conf)) {
beacon_diff = (long)jiffies - (long)rt2x00dev->last_beacon;
beacon_int = msecs_to_jiffies(rt2x00dev->beacon_int);
@@ -274,7 +274,7 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
autowake_timeout - 15);
}
- if (conf->flags & IEEE80211_CONF_PS)
+ if (ieee80211_is_ps_enabled(conf))
set_bit(CONFIG_POWERSAVING, &rt2x00dev->flags);
else
clear_bit(CONFIG_POWERSAVING, &rt2x00dev->flags);
diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index b40a538..b3e8367 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -596,7 +596,7 @@ static void rt2x00lib_rxdone_check_ps(struct rt2x00_dev *rt2x00dev,
* configured, or if the device is already in powersaving mode
* we can exit now. */
if (likely(!ieee80211_is_beacon(hdr->frame_control) ||
- !(rt2x00dev->hw->conf.flags & IEEE80211_CONF_PS)))
+ ieee80211_is_ps_disabled(&rt2x00dev->hw->conf)))
return;
/* min. beacon length + FCS_LEN */
diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
index f95792c..5cd1e4e 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -942,8 +942,7 @@ static void rt61pci_config_ps(struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf)
{
enum dev_state state =
- (libconf->conf->flags & IEEE80211_CONF_PS) ?
- STATE_SLEEP : STATE_AWAKE;
+ ieee80211_is_ps_enabled(libconf->conf) ? STATE_SLEEP : STATE_AWAKE;
u32 reg;
if (state == STATE_SLEEP) {
diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
index 24eec66..f44fc3d 100644
--- a/drivers/net/wireless/rt2x00/rt73usb.c
+++ b/drivers/net/wireless/rt2x00/rt73usb.c
@@ -827,8 +827,7 @@ static void rt73usb_config_ps(struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf)
{
enum dev_state state =
- (libconf->conf->flags & IEEE80211_CONF_PS) ?
- STATE_SLEEP : STATE_AWAKE;
+ ieee80211_is_ps_enabled(libconf->conf) ? STATE_SLEEP : STATE_AWAKE;
u32 reg;
if (state == STATE_SLEEP) {
diff --git a/drivers/net/wireless/rtlwifi/core.c b/drivers/net/wireless/rtlwifi/core.c
index d3ce9fb..ca42a89 100644
--- a/drivers/net/wireless/rtlwifi/core.c
+++ b/drivers/net/wireless/rtlwifi/core.c
@@ -289,7 +289,7 @@ static int rtl_op_config(struct ieee80211_hw *hw, u32 changed)
if (changed & IEEE80211_CONF_CHANGE_PS) {
cancel_delayed_work(&rtlpriv->works.ps_work);
cancel_delayed_work(&rtlpriv->works.ps_rfon_wq);
- if (conf->flags & IEEE80211_CONF_PS) {
+ if (ieee80211_is_ps_enabled(conf)) {
rtlpriv->psc.sw_ps_enabled = true;
/* sleep here is must, or we may recv the beacon and
* cause mac80211 into wrong ps state, this will cause
diff --git a/drivers/net/wireless/rtlwifi/ps.c b/drivers/net/wireless/rtlwifi/ps.c
index 13ad33e..b181d22 100644
--- a/drivers/net/wireless/rtlwifi/ps.c
+++ b/drivers/net/wireless/rtlwifi/ps.c
@@ -468,7 +468,7 @@ void rtl_swlps_beacon(struct ieee80211_hw *hw, void *data, unsigned int len)
if (rtlpriv->psc.fwctrl_lps)
return;
- if (likely(!(hw->conf.flags & IEEE80211_CONF_PS)))
+ if (likely(ieee80211_is_ps_disabled(&hw->conf)))
return;
/* check if this really is a beacon */
@@ -624,7 +624,7 @@ void rtl_swlps_wq_callback(void *data)
struct rtl_priv *rtlpriv = rtl_priv(hw);
bool ps = false;
- ps = (hw->conf.flags & IEEE80211_CONF_PS);
+ ps = ieee80211_is_ps_enabled(&hw->conf);
/* we can sleep after ps null send ok */
if (rtlpriv->psc.state_inap) {
diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index f47e8b0..4bee40b 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -576,7 +576,7 @@ static int wl1251_op_config(struct ieee80211_hw *hw, u32 changed)
wl1251_debug(DEBUG_MAC80211, "mac80211 config ch %d psm %s power %d",
channel,
- conf->flags & IEEE80211_CONF_PS ? "on" : "off",
+ ieee80211_is_ps_enabled(conf) ? "on" : "off",
conf->power_level);
mutex_lock(&wl->mutex);
@@ -594,7 +594,7 @@ static int wl1251_op_config(struct ieee80211_hw *hw, u32 changed)
goto out_sleep;
}
- if (conf->flags & IEEE80211_CONF_PS && !wl->psm_requested) {
+ if (ieee80211_is_ps_enabled(conf) && !wl->psm_requested) {
wl1251_debug(DEBUG_PSM, "psm enabled");
wl->psm_requested = true;
@@ -610,8 +610,7 @@ static int wl1251_op_config(struct ieee80211_hw *hw, u32 changed)
ret = wl1251_ps_set_mode(wl, STATION_POWER_SAVE_MODE);
if (ret < 0)
goto out_sleep;
- } else if (!(conf->flags & IEEE80211_CONF_PS) &&
- wl->psm_requested) {
+ } else if (ieee80211_is_ps_disabled(conf) && wl->psm_requested) {
wl1251_debug(DEBUG_PSM, "psm disabled");
wl->psm_requested = false;
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index ce6e62a..4235868 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -2670,7 +2670,7 @@ static int wl12xx_config_vif(struct wl1271 *wl, struct wl12xx_vif *wlvif,
if ((changed & IEEE80211_CONF_CHANGE_PS) && !is_ap) {
- if ((conf->flags & IEEE80211_CONF_PS) &&
+ if (ieee80211_is_ps_enabled(conf) &&
test_bit(WLVIF_FLAG_STA_ASSOCIATED, &wlvif->flags) &&
!test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags)) {
@@ -2693,7 +2693,7 @@ static int wl12xx_config_vif(struct wl1271 *wl, struct wl12xx_vif *wlvif,
wl1271_warning("enter %s ps failed %d",
ps_mode_str, ret);
- } else if (!(conf->flags & IEEE80211_CONF_PS) &&
+ } else if (ieee80211_is_ps_disabled(conf) &&
test_bit(WLVIF_FLAG_IN_PS, &wlvif->flags)) {
wl1271_debug(DEBUG_PSM, "auto ps disabled");
@@ -2728,7 +2728,7 @@ static int wl1271_op_config(struct ieee80211_hw *hw, u32 changed)
wl1271_debug(DEBUG_MAC80211, "mac80211 config ch %d psm %s power %d %s"
" changed 0x%x",
channel,
- conf->flags & IEEE80211_CONF_PS ? "on" : "off",
+ ieee80211_is_ps_enabled(conf) ? "on" : "off",
conf->power_level,
conf->flags & IEEE80211_CONF_IDLE ? "idle" : "in use",
changed);
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 3037f49..d6c24d9 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -861,13 +861,16 @@ struct ieee80211_rx_status {
* @IEEE80211_CONF_MONITOR: there's a monitor interface present -- use this
* to determine for example whether to calculate timestamps for packets
* or not, do not use instead of filter flags!
- * @IEEE80211_CONF_PS: Enable 802.11 power save mode (managed mode only).
- * This is the power save mode defined by IEEE 802.11-2007 section 11.2,
- * meaning that the hardware still wakes up for beacons, is able to
- * transmit frames and receive the possible acknowledgment frames.
- * Not to be confused with hardware specific wakeup/sleep states,
- * driver is responsible for that. See the section "Powersave support"
- * for more.
+ * @IEEE80211_CONF_PS_MASK: This mask defines the bits which identify the
+ * 802.11 power save mode defined by IEEE 802.11-2007 secion 11.2.
+ * Possible values for this fields are defined by subsequent
+ * IEEE80211_CONF_PS_* flags. See the secion "Powersave support" for more.
+ * @IEEE80211_CONF_PS_DISABLED: Power save is disabled. The hardware is able
+ * to transmit and receive frames.
+ * @IEEE80211_CONF_PS_ENABLED: Power save is enabled. The hardware still wakes
+ * up for beacons, is able to transmit frames and receive the possible
+ * acknowledgement frames. Not to be confused with hardware specific
+ * wakeup/sleep states; the driver is responsible for that.
* @IEEE80211_CONF_IDLE: The device is running, but idle; if the flag is set
* the driver should be prepared to handle configuration requests but
* may turn the device off as much as possible. Typically, this flag will
@@ -878,12 +881,13 @@ struct ieee80211_rx_status {
*/
enum ieee80211_conf_flags {
IEEE80211_CONF_MONITOR = (1<<0),
- IEEE80211_CONF_PS = (1<<1),
- IEEE80211_CONF_IDLE = (1<<2),
- IEEE80211_CONF_OFFCHANNEL = (1<<3),
+ IEEE80211_CONF_PS_MASK = (3<<1),
+ IEEE80211_CONF_PS_DISABLED = (0<<1),
+ IEEE80211_CONF_PS_ENABLED = (1<<1),
+ IEEE80211_CONF_IDLE = (1<<3),
+ IEEE80211_CONF_OFFCHANNEL = (1<<4),
};
-
/**
* enum ieee80211_conf_changed - denotes which configuration changed
*
@@ -983,6 +987,49 @@ struct ieee80211_conf {
};
/**
+ * ieee80211_is_ps_disabled - check if powersave is disabled
+ *
+ * Returns true if powersave is disabled in the supplied configuration.
+ *
+ * @conf: device configuration
+ */
+static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
+{
+ return (conf->flags & IEEE80211_CONF_PS_MASK) ==
+ IEEE80211_CONF_PS_DISABLED;
+}
+
+/**
+ * ieee80211_is_ps_enabled - check if powersave is enabled
+ *
+ * Returns true if powersave is enabled in the supplied configuration.
+ *
+ * @conf: device configuration
+ */
+static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
+{
+ return (conf->flags & IEEE80211_CONF_PS_MASK) ==
+ IEEE80211_CONF_PS_ENABLED;
+}
+
+/**
+ * ieee80211_set_ps_state - set device powersave state
+ *
+ * Sets the powersave state in the supplied device configuration to the
+ * specified state.
+ *
+ * @conf: device configuration
+ * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_*
+ * flags from enum ieee80211_conf_flags.
+ */
+static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf,
+ u32 state)
+{
+ conf->flags = (conf->flags & ~IEEE80211_CONF_PS_MASK) |
+ (state & IEEE80211_CONF_PS_MASK);
+}
+
+/**
* struct ieee80211_channel_switch - holds the channel switch data
*
* The information provided in this structure is required for channel switch
@@ -1640,13 +1687,14 @@ void ieee80211_free_txskb(struct ieee80211_hw *hw, struct sk_buff *skb);
* First, it can support hardware that handles all powersaving by itself,
* such hardware should simply set the %IEEE80211_HW_SUPPORTS_PS hardware
* flag. In that case, it will be told about the desired powersave mode
- * with the %IEEE80211_CONF_PS flag depending on the association status.
+ * with the %IEEE80211_CONF_PS_* states depending on the association status.
+ * The current state can be checked with the ieee80211_is_ps_*() functions.
* The hardware must take care of sending nullfunc frames when necessary,
* i.e. when entering and leaving powersave mode. The hardware is required
* to look at the AID in beacons and signal to the AP that it woke up when
* it finds traffic directed to it.
*
- * %IEEE80211_CONF_PS flag enabled means that the powersave mode defined in
+ * When ieee80211_is_ps_enabled() returns true the powersave mode defined in
* IEEE 802.11-2007 section 11.2 is enabled. This is not to be confused
* with hardware wakeup and sleep states. Driver is responsible for waking
* up the hardware before issuing commands to the hardware and putting it
@@ -1678,9 +1726,9 @@ void ieee80211_free_txskb(struct ieee80211_hw *hw, struct sk_buff *skb);
* %IEEE80211_HW_SUPPORTS_DYNAMIC_PS flag to indicate that it can support
* dynamic PS mode itself. The driver needs to look at the
* @dynamic_ps_timeout hardware configuration value and use it that value
- * whenever %IEEE80211_CONF_PS is set. In this case mac80211 will disable
- * dynamic PS feature in stack and will just keep %IEEE80211_CONF_PS
- * enabled whenever user has enabled powersave.
+ * whenever ieee80211_is_ps_enabled() returns true. In this case mac80211 will
+ * disable dynamic PS feature in stack and will just keep powersave enabled
+ * whenever user has enabled powersave.
*
* Some hardware need to toggle a single shared antenna between WLAN and
* Bluetooth to facilitate co-existence. These types of hardware set
@@ -1715,9 +1763,10 @@ void ieee80211_free_txskb(struct ieee80211_hw *hw, struct sk_buff *skb);
*
* Beacon filter support is advertised with the %IEEE80211_VIF_BEACON_FILTER
* interface capability. The driver needs to enable beacon filter support
- * whenever power save is enabled, that is %IEEE80211_CONF_PS is set. When
- * power save is enabled, the stack will not check for beacon loss and the
- * driver needs to notify about loss of beacons with ieee80211_beacon_loss().
+ * whenever power save is enabled, that is ieee80211_is_ps_enabled() returns
+ * true. When power save is enabled, the stack will not check for beacon loss
+ * and the driver needs to notify about loss of beacons with
+ * ieee80211_beacon_loss().
*
* The time (or number of beacons missed) until the firmware notifies the
* driver of a beacon loss event (which in turn causes the driver to call
@@ -3847,8 +3896,8 @@ struct sk_buff *ieee80211_ap_probereq_get(struct ieee80211_hw *hw,
* @vif: &struct ieee80211_vif pointer from the add_interface callback.
*
* When beacon filtering is enabled with %IEEE80211_VIF_BEACON_FILTER and
- * %IEEE80211_CONF_PS is set, the driver needs to inform whenever the
- * hardware is not receiving beacons with this function.
+ * ieee80211_is_ps_enabled() returns true, the driver needs to inform whenever
+ * the hardware is not receiving beacons with this function.
*/
void ieee80211_beacon_loss(struct ieee80211_vif *vif);
@@ -3858,8 +3907,9 @@ void ieee80211_beacon_loss(struct ieee80211_vif *vif);
* @vif: &struct ieee80211_vif pointer from the add_interface callback.
*
* When beacon filtering is enabled with %IEEE80211_VIF_BEACON_FILTER, and
- * %IEEE80211_CONF_PS and %IEEE80211_HW_CONNECTION_MONITOR are set, the driver
- * needs to inform if the connection to the AP has been lost.
+ * ieee80211_is_ps_enabled() returns true and %IEEE80211_HW_CONNECTION_MONITOR
+ * are set, the driver needs to inform if the connection to the AP has been
+ * lost.
*
* This function will cause immediate change to disassociated state,
* without connection recovery attempts.
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 7211344..df64efe 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1004,7 +1004,7 @@ static void ieee80211_enable_ps(struct ieee80211_local *local,
(local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS))
return;
- conf->flags |= IEEE80211_CONF_PS;
+ ieee80211_set_ps_state(conf, IEEE80211_CONF_PS_ENABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
}
@@ -1015,8 +1015,8 @@ static void ieee80211_change_ps(struct ieee80211_local *local)
if (local->ps_sdata) {
ieee80211_enable_ps(local, local->ps_sdata);
- } else if (conf->flags & IEEE80211_CONF_PS) {
- conf->flags &= ~IEEE80211_CONF_PS;
+ } else if (ieee80211_is_ps_enabled(conf)) {
+ ieee80211_set_ps_state(conf, IEEE80211_CONF_PS_DISABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
del_timer_sync(&local->dynamic_ps_timer);
cancel_work_sync(&local->dynamic_ps_enable_work);
@@ -1150,8 +1150,9 @@ void ieee80211_dynamic_ps_disable_work(struct work_struct *work)
container_of(work, struct ieee80211_local,
dynamic_ps_disable_work);
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
- local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ if (ieee80211_is_ps_enabled(&local->hw.conf)) {
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_DISABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -1175,7 +1176,7 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
ifmgd = &sdata->u.mgd;
- if (local->hw.conf.flags & IEEE80211_CONF_PS)
+ if (ieee80211_is_ps_enabled(&local->hw.conf))
return;
if (!local->disable_dynamic_ps &&
@@ -1226,7 +1227,8 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
- local->hw.conf.flags |= IEEE80211_CONF_PS;
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_ENABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -1534,8 +1536,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
* to do it before sending disassoc, as otherwise the null-packet
* won't be valid.
*/
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
- local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ if (!ieee80211_is_ps_disabled(&local->hw.conf)) {
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_DISABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
local->ps_sdata = NULL;
@@ -2671,8 +2674,9 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
ifmgd->aid);
if (directed_tim) {
if (local->hw.conf.dynamic_ps_timeout > 0) {
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
- local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ if (ieee80211_is_ps_enabled(&local->hw.conf)) {
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_DISABLED);
ieee80211_hw_config(local,
IEEE80211_CONF_CHANGE_PS);
}
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 28274f9..d524794 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -40,9 +40,10 @@ static bool ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
cancel_work_sync(&local->dynamic_ps_enable_work);
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ if (ieee80211_is_ps_enabled(&local->hw.conf)) {
local->offchannel_ps_enabled = true;
- local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_DISABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -87,11 +88,12 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
/* TODO: Only set hardware if CONF_PS changed?
* TODO: Should we set offchannel_ps_enabled to false?
*/
- local->hw.conf.flags |= IEEE80211_CONF_PS;
+ ieee80211_set_ps_state(&local->hw.conf,
+ IEEE80211_CONF_PS_ENABLED);
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
} else if (local->hw.conf.dynamic_ps_timeout > 0) {
/*
- * If IEEE80211_CONF_PS was not set and the dynamic_ps_timer
+ * If power save was not enabled and the dynamic_ps_timer
* had been running before leaving the operating channel,
* restart the timer now and send a nullfunc frame to inform
* the AP that we are awake.
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 80b514a..ff64a4b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -231,7 +231,7 @@ ieee80211_tx_h_dynamic_ps(struct ieee80211_tx_data *tx)
skb_get_queue_mapping(tx->skb) == IEEE80211_AC_VO)
return TX_CONTINUE;
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ if (ieee80211_is_ps_enabled(&local->hw.conf)) {
ieee80211_stop_queues_by_reason(&local->hw,
IEEE80211_QUEUE_STOP_REASON_PS);
ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 0923892..cf9ea6f 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1594,7 +1594,7 @@ int ieee80211_reconfig(struct ieee80211_local *local)
* explicitly send a null packet in order to make sure
* it'll sync against the ap (and get out of psm).
*/
- if (!(local->hw.conf.flags & IEEE80211_CONF_PS)) {
+ if (ieee80211_is_ps_disabled(&local->hw.conf)) {
list_for_each_entry(sdata, &local->interfaces, list) {
if (sdata->vif.type != NL80211_IFTYPE_STATION)
continue;
--
1.7.9.5
We've got a couple of races when enabling powersave with an AP for
off-channel operation. The first is fairly simple. If we go off-channel
before the nullfunc frame to enable PS is transmitted then it may not be
received by the AP. Add a flush after enabling off-channel PS to prevent
this from happening.
The second race is a bit more subtle. If the driver supports QoS and has
frames queued when the nullfunc frame is queued, those frames may get
transmitted after the nullfunc frame. If PM is not set then the AP is
being told that we've exited PS before we go off-channel and may try to
deliver frames. To prevent this, add a flush after stopping the queues
but before passing the nullfunc frame to the driver.
Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/offchannel.c | 5 +++++
net/mac80211/scan.c | 3 +++
2 files changed, 8 insertions(+)
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 650af94..28274f9 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -118,8 +118,13 @@ bool ieee80211_offchannel_stop_vifs(struct ieee80211_local *local)
* STA interfaces.
*/
+ /*
+ * Stop queues and transmit all frames queued by the driver
+ * before sending nullfunc to enable powersave at the AP.
+ */
ieee80211_stop_queues_by_reason(&local->hw,
IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL);
+ drv_flush(local, false);
mutex_lock(&local->iflist_mtx);
list_for_each_entry(sdata, &local->interfaces, list) {
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 894c95e..c8103e4 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -343,6 +343,9 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
if (!ieee80211_offchannel_stop_vifs(local))
goto error;
+ /* ensure nullfunc is transmitted before leaving operating channel */
+ drv_flush(local, false);
+
ieee80211_configure_filter(local);
/* We need to set power level at maximum rate for scanning. */
--
1.7.9.5
For off-channel operation we would like to know whether or not frames
submitted for tx are dropped, but most tx interfaces don't return
any status. The low-level interfaces do return bool but make no
distinction between dropping a packet and sucessfully handing it off to
the driver.
Add an enum named ieee80211_tx_status which defines status codes to
indicate whether a tx request results in successfully handing the frame
off to the driver, queueing the frame, or dropping it. Convert all tx
interfaces to return one of the values.
Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/ieee80211_i.h | 53 ++++++++++++++++-------
net/mac80211/tx.c | 103 +++++++++++++++++++++++---------------------
2 files changed, 92 insertions(+), 64 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 63f0430..13fd13a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1495,46 +1495,69 @@ void mac80211_ev_michael_mic_failure(struct ieee80211_sub_if_data *sdata, int ke
gfp_t gfp);
void ieee80211_set_wmm_default(struct ieee80211_sub_if_data *sdata,
bool bss_notify);
-void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
- enum ieee80211_band band);
-void __ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
- struct sk_buff *skb, int tid,
- enum ieee80211_band band);
+/**
+ * enum ieee80211_tx_status - Tx request status
+ * @IEEE80211_TX_OK: Tx request handed off to driver
+ * @IEEE80211_TX_DROPPED: Tx request dropped
+ * @IEEE80211_TX_QUEUED: Tx requeust queued
+ */
+enum ieee80211_tx_status {
+ IEEE80211_TX_OK,
+ IEEE80211_TX_DROPPED,
+ IEEE80211_TX_QUEUED,
+};
-static inline void
+enum ieee80211_tx_status
+ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
+ enum ieee80211_band band);
+
+enum ieee80211_tx_status
+__ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
+ struct sk_buff *skb, int tid,
+ enum ieee80211_band band);
+
+static inline enum ieee80211_tx_status
ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
struct sk_buff *skb, int tid,
enum ieee80211_band band)
{
+ enum ieee80211_tx_status ret;
+
rcu_read_lock();
- __ieee80211_tx_skb_tid_band(sdata, skb, tid, band);
+ ret = __ieee80211_tx_skb_tid_band(sdata, skb, tid, band);
rcu_read_unlock();
+
+ return ret;
}
-static inline void ieee80211_tx_skb_tid(struct ieee80211_sub_if_data *sdata,
- struct sk_buff *skb, int tid)
+static inline enum ieee80211_tx_status
+ieee80211_tx_skb_tid(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
+ int tid)
{
struct ieee80211_chanctx_conf *chanctx_conf;
+ enum ieee80211_tx_status ret;
rcu_read_lock();
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
if (WARN_ON(!chanctx_conf)) {
rcu_read_unlock();
kfree_skb(skb);
- return;
+ return IEEE80211_TX_DROPPED;
}
- __ieee80211_tx_skb_tid_band(sdata, skb, tid,
- chanctx_conf->def.chan->band);
+ ret = __ieee80211_tx_skb_tid_band(sdata, skb, tid,
+ chanctx_conf->def.chan->band);
rcu_read_unlock();
+
+ return ret;
}
-static inline void ieee80211_tx_skb(struct ieee80211_sub_if_data *sdata,
- struct sk_buff *skb)
+static inline enum ieee80211_tx_status
+ieee80211_tx_skb(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
{
/* Send all internal mgmt frames on VO. Accordingly set TID to 7. */
- ieee80211_tx_skb_tid(sdata, skb, 7);
+ return ieee80211_tx_skb_tid(sdata, skb, 7);
}
void ieee802_11_parse_elems(u8 *start, size_t len,
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f32d681..7d25bb6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1200,11 +1200,10 @@ ieee80211_tx_prepare(struct ieee80211_sub_if_data *sdata,
return TX_CONTINUE;
}
-static bool ieee80211_tx_frags(struct ieee80211_local *local,
- struct ieee80211_vif *vif,
- struct ieee80211_sta *sta,
- struct sk_buff_head *skbs,
- bool txpending)
+static enum ieee80211_tx_status
+ieee80211_tx_frags(struct ieee80211_local *local, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta, struct sk_buff_head *skbs,
+ bool txpending)
{
struct ieee80211_tx_control control;
struct sk_buff *skb, *tmp;
@@ -1238,7 +1237,7 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
spin_unlock_irqrestore(&local->queue_stop_reason_lock,
flags);
- return false;
+ return IEEE80211_TX_QUEUED;
}
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
@@ -1249,26 +1248,23 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
drv_tx(local, &control, skb);
}
- return true;
+ return IEEE80211_TX_OK;
}
-/*
- * Returns false if the frame couldn't be transmitted but was queued instead.
- */
-static bool __ieee80211_tx(struct ieee80211_local *local,
- struct sk_buff_head *skbs, int led_len,
- struct sta_info *sta, bool txpending)
+static enum ieee80211_tx_status
+__ieee80211_tx(struct ieee80211_local *local, struct sk_buff_head *skbs,
+ int led_len, struct sta_info *sta, bool txpending)
{
struct ieee80211_tx_info *info;
struct ieee80211_sub_if_data *sdata;
struct ieee80211_vif *vif;
struct ieee80211_sta *pubsta;
struct sk_buff *skb;
- bool result = true;
+ enum ieee80211_tx_status result;
__le16 fc;
if (WARN_ON(skb_queue_empty(skbs)))
- return true;
+ return IEEE80211_TX_OK;
skb = skb_peek(skbs);
fc = ((struct ieee80211_hdr *)skb->data)->frame_control;
@@ -1291,7 +1287,7 @@ static bool __ieee80211_tx(struct ieee80211_local *local,
vif->hw_queue[skb_get_queue_mapping(skb)];
} else if (local->hw.flags & IEEE80211_HW_QUEUE_CONTROL) {
dev_kfree_skb(skb);
- return true;
+ return IEEE80211_TX_DROPPED;
} else
vif = NULL;
break;
@@ -1371,23 +1367,20 @@ static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
return 0;
}
-/*
- * Returns false if the frame couldn't be transmitted but was queued instead.
- */
-static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata,
- struct sk_buff *skb, bool txpending,
- enum ieee80211_band band)
+static enum ieee80211_tx_status
+ieee80211_tx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
+ bool txpending, enum ieee80211_band band)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_tx_data tx;
ieee80211_tx_result res_prepare;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- bool result = true;
+ enum ieee80211_tx_status result = IEEE80211_TX_OK;
int led_len;
if (unlikely(skb->len < 10)) {
dev_kfree_skb(skb);
- return true;
+ return IEEE80211_TX_DROPPED;
}
/* initialises tx */
@@ -1396,9 +1389,17 @@ static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata,
if (unlikely(res_prepare == TX_DROP)) {
ieee80211_free_txskb(&local->hw, skb);
- return true;
+ return IEEE80211_TX_DROPPED;
} else if (unlikely(res_prepare == TX_QUEUED)) {
- return true;
+ /*
+ * TX_QUEUED here means that the frame was queued in
+ * tid_tx while aggregation is being set up. This need
+ * not prevent tx of other pending frames, so indicate
+ * success if txpending is true.
+ */
+ if (txpending)
+ return IEEE80211_TX_OK;
+ return IEEE80211_TX_QUEUED;
}
info->band = band;
@@ -1447,8 +1448,9 @@ static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata,
return 0;
}
-void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
- enum ieee80211_band band)
+enum ieee80211_tx_status
+ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
+ enum ieee80211_band band)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
@@ -1466,7 +1468,7 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
if (ieee80211_skb_resize(sdata, skb, headroom, may_encrypt)) {
ieee80211_free_txskb(&local->hw, skb);
- return;
+ return IEEE80211_TX_DROPPED;
}
hdr = (struct ieee80211_hdr *) skb->data;
@@ -1477,11 +1479,11 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
!is_multicast_ether_addr(hdr->addr1) &&
mesh_nexthop_resolve(skb, sdata)) {
/* skb queued: don't free */
- return;
+ return IEEE80211_TX_QUEUED;
}
ieee80211_set_qos_hdr(sdata, skb);
- ieee80211_tx(sdata, skb, false, band);
+ return ieee80211_tx(sdata, skb, false, band);
}
static bool ieee80211_parse_tx_radiotap(struct sk_buff *skb)
@@ -2174,18 +2176,17 @@ void ieee80211_clear_tx_pending(struct ieee80211_local *local)
}
/*
- * Returns false if the frame couldn't be transmitted but was queued instead,
- * which in this case means re-queued -- take as an indication to stop sending
- * more pending frames.
+ * A return value of IEEE80211_TX_QUEUED in this case means re-queued --
+ * take as an indication to stop sending more pending frames.
*/
-static bool ieee80211_tx_pending_skb(struct ieee80211_local *local,
- struct sk_buff *skb)
+static enum ieee80211_tx_status
+ieee80211_tx_pending_skb(struct ieee80211_local *local, struct sk_buff *skb)
{
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ieee80211_sub_if_data *sdata;
struct sta_info *sta;
struct ieee80211_hdr *hdr;
- bool result;
+ enum ieee80211_tx_status tx_stat;
struct ieee80211_chanctx_conf *chanctx_conf;
sdata = vif_to_sdata(info->control.vif);
@@ -2194,10 +2195,10 @@ static bool ieee80211_tx_pending_skb(struct ieee80211_local *local,
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
if (unlikely(!chanctx_conf)) {
dev_kfree_skb(skb);
- return true;
+ return IEEE80211_TX_DROPPED;
}
- result = ieee80211_tx(sdata, skb, true,
- chanctx_conf->def.chan->band);
+ tx_stat = ieee80211_tx(sdata, skb, true,
+ chanctx_conf->def.chan->band);
} else {
struct sk_buff_head skbs;
@@ -2207,10 +2208,10 @@ static bool ieee80211_tx_pending_skb(struct ieee80211_local *local,
hdr = (struct ieee80211_hdr *)skb->data;
sta = sta_info_get(sdata, hdr->addr1);
- result = __ieee80211_tx(local, &skbs, skb->len, sta, true);
+ tx_stat = __ieee80211_tx(local, &skbs, skb->len, sta, true);
}
- return result;
+ return tx_stat;
}
/*
@@ -2221,7 +2222,7 @@ void ieee80211_tx_pending(unsigned long data)
struct ieee80211_local *local = (struct ieee80211_local *)data;
unsigned long flags;
int i;
- bool txok;
+ enum ieee80211_tx_status tx_stat;
rcu_read_lock();
@@ -2247,10 +2248,10 @@ void ieee80211_tx_pending(unsigned long data)
spin_unlock_irqrestore(&local->queue_stop_reason_lock,
flags);
- txok = ieee80211_tx_pending_skb(local, skb);
+ tx_stat = ieee80211_tx_pending_skb(local, skb);
spin_lock_irqsave(&local->queue_stop_reason_lock,
flags);
- if (!txok)
+ if (tx_stat == IEEE80211_TX_QUEUED)
break;
}
@@ -2775,11 +2776,13 @@ ieee80211_get_buffered_bc(struct ieee80211_hw *hw,
}
EXPORT_SYMBOL(ieee80211_get_buffered_bc);
-void __ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
- struct sk_buff *skb, int tid,
- enum ieee80211_band band)
+enum ieee80211_tx_status
+__ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
+ struct sk_buff *skb, int tid,
+ enum ieee80211_band band)
{
int ac = ieee802_1d_to_ac[tid & 7];
+ enum ieee80211_tx_status ret;
skb_set_mac_header(skb, 0);
skb_set_network_header(skb, 0);
@@ -2794,6 +2797,8 @@ void __ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
* requirements are that we do not come into tx with bhs on.
*/
local_bh_disable();
- ieee80211_xmit(sdata, skb, band);
+ ret = ieee80211_xmit(sdata, skb, band);
local_bh_enable();
+
+ return ret;
}
--
1.7.9.5
On Thu, Jan 31, 2013 at 04:15:50PM +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> > Errors in sending nullfunc or probe request frames during off-channel
> > operation may have undesirable consequences, e.g. failure to set
> > powersave at the AP. Add error handling for failures to transmit these
> > frames. In the case of a nullfunc failure, fail to go off-channel and
> > return an error to userspace. In the case of a failed probe request,
> > abort the scan.
>
> That latter part seems excessive? Maybe increase the time to use a
> passive scan? But if there are multiple scan requests ...
>
> Is all of this really worth the effort? The driver queues should be
> empty after the flush, after all, and the driver doesn't return any TX
> status. So what can really happen?
Hmm, yeah, maybe it is a bit excessive. The idea of falling back to a
passive scan is interesting though. I'll rip out the scan abort stuff
for v2 and think about doing the passive scan fallback as a future
enhancement.
Seth
On Thu, Jan 31, 2013 at 05:53:49PM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 10:33 -0600, Seth Forshee wrote:
> > On Thu, Jan 31, 2013 at 04:20:48PM +0100, Johannes Berg wrote:
> > > On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
> > >
> > > > +static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf)
> > >
> > > > +static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf)
> > >
> > > Huh, is that worth the confusion? It seems !enabled should be the same
> > > as disabled, but it's not quite the same, which might be confusing.
> >
> > In this patch there's no distinction, but after adding the off-channel
> > powersave state there is -- disabled == !enabled && !offchannel.
>
> I thought it was something like that, yeah.
>
> > Actually one of the last bugs I fixed before sending these was a place
> > where I had used disabled instead of !enabled, and the frames ended up
> > with PM set when it shouldn't have been.
> >
> > I agree though that the distinction is confusing. Maybe some better
> > state names are needed. Perhaps awake, offchannel, and doze?
>
> I think what you really want is to distinguish between "HW can go to
> powersave" and "PM bit should be set"? That's pretty much what your
> CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe
Correct, with the understanding that "HW can go to powersave" also
implies "PM bit should be set."
Another approach would be to keep the CONF_PS flag the same and add a
CONF_PM flag or similar. I didn't go with this approach because CONF_PS
&& !CONF_PM really doesn't make any sense, which really doesn't help
with reducing confusion. The advantage is that it separates setting PM
from PS for those driver that don't support PS but need to configure the
hardware to set PM for off-channel.
> putting it in different terms would make it less confusing?
Yes. I think the reason enabled/disabled is confusing because it would
usually imply a binary state.
Seth
On Tue, Jan 29, 2013 at 05:47:28PM -0600, Seth Forshee wrote:
> This patch series fixes a number of problems observed during software
> scanning, as described in [1].
>
> The first four patches implement improved queue handling during
> off-channel operation and add some needed flushing of the hardware
> queues, as suggested by Johannes in [2]. This includes adding a new
> queue stop reason of off-channel operation, a transmit path for frames
> which do need to be transmitted when off-channel, and some improvements
> in error handling.
>
> The last three fix a problem specific to brcmsmac (and likely b43,
> though I don't have hardware for testing b43). Broadcom hardware
> actively clears the PM bit in frame control whenever the hardware is not
> configured correctly, and since wireless drivers have no knowledge of
> off-channel powersave such configuration cannot be done. The patches
> expand the driver powersave configuration with an off-channel state and
> update brcmsmac to make use of it.
>
> Johannes: I have a couple of comments/questions for you related to these
> patches.
>
> First, in the patches I've added an offchan_tx_ok argument to the tx
> operations, but this seems a little awkward to me since it has to be
> propogated down through a fairly deep call stack. The alternative idea
> that occurred to me is to use a tx control flag, but that seems to be
> pretty crowded. Any thoughts?
>
> Second, I attempted to test these patches with iwlwifi (Centrino
> Advanced-N 6235) to verify that I didn't break anything for drivers with
> hw scanning. My standard test for this involves running iperf while
> triggering nearly continuous scans, but I'm seeing lots of problems
> running a tcp iperf test even with unpatched 3.8-rc4. iperf with udp
> does fine in either direction. I haven't had time to do any kind of
> debugging yet, but I thought you'd want to know.
>
> Thanks,
> Seth
>
> [1] http://marc.info/?l=linux-wireless&m=135766865110986&w=2
> [2] http://marc.info/?l=linux-wireless&m=135838252227053&w=2
>
>
> Seth Forshee (7):
> mac80211: Return a status for tx operations
> mac80211: Fix tx queue handling during scans
> mac80211: Improve error handling for off-channel operation
> mac80211: Add flushes before going off-channel
> mac80211: Expand powersave configuration flag to be two bits
> mac80211: Add off-channel powersave state
> brcmsmac: Add support for off-channel powersave
Johannes,
Please take the brcsmac patch through your tree if/when you take
the others.
Thanks!
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Thu, 2013-01-31 at 16:04 +0100, Johannes Berg wrote:
> On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote:
>
> > Johannes: I have a couple of comments/questions for you related to these
> > patches.
> >
> > First, in the patches I've added an offchan_tx_ok argument to the tx
> > operations, but this seems a little awkward to me since it has to be
> > propogated down through a fairly deep call stack. The alternative idea
> > that occurred to me is to use a tx control flag, but that seems to be
> > pretty crowded. Any thoughts?
>
> Maybe you can bypass by using a flag in struct ieee80211_tx_data, so
> only the first few functions in the call chain need the argument?
> Otherwise, I guess adding a flag should be OK. I know it's crowded, but
> if we really run out I guess we could move all the internal flags etc.
> wholesale ...
Ok no that was wrong ... we can't do that because many flags need to
survive queueing.
johannes
Hi Seth,
On Wed, Jan 30, 2013 at 10:47 AM, Seth Forshee
<[email protected]> wrote:
> Broadcom hardware sets the value of the PM flag in frame control based
> on the value of MCTL_HPS, which seems to include actively clearing PM
> if MCTL_HPS is not set. brcmsmac needs to suppor the off-channel
> powersave state in order to enable powersave at the AP at all.
>
> Add limited support for powersave to brcmsmac to prevent frame loss
> during background scans. Full powersave supoprt remains unimplemented,
> but switching between the off-channel and disabled states is possible.
>
> Signed-off-by: Seth Forshee <[email protected]>
> ---
> .../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 15 +++++++++++----
> drivers/net/wireless/brcm80211/brcmsmac/main.c | 9 +++++++++
> drivers/net/wireless/brcm80211/brcmsmac/pub.h | 1 +
> 3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> index c26992a..3fb71c9 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> @@ -7545,6 +7549,11 @@ void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc, u8 interval)
> brcms_c_bcn_li_upd(wlc);
> }
>
> +void brcms_c_set_ps(struct brcms_c_info *wlc)
> +{
> + brcms_c_set_ps_ctrl(wlc);
> +}
> +
Why not just use brcms_c_set_ps_ctrl() directly?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
Errors in sending nullfunc or probe request frames during off-channel
operation may have undesirable consequences, e.g. failure to set
powersave at the AP. Add error handling for failures to transmit these
frames. In the case of a nullfunc failure, fail to go off-channel and
return an error to userspace. In the case of a failed probe request,
abort the scan.
Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/ieee80211_i.h | 6 +++---
net/mac80211/mlme.c | 6 +++---
net/mac80211/offchannel.c | 24 ++++++++++++++++++------
net/mac80211/scan.c | 41 ++++++++++++++++++++++++++++-------------
net/mac80211/util.c | 12 ++++++++----
5 files changed, 60 insertions(+), 29 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8374763..c062d20 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1369,7 +1369,7 @@ 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 */
-void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local);
+bool ieee80211_offchannel_stop_vifs(struct ieee80211_local *local);
void ieee80211_offchannel_return(struct ieee80211_local *local);
void ieee80211_roc_setup(struct ieee80211_local *local);
void ieee80211_start_next_roc(struct ieee80211_local *local);
@@ -1582,7 +1582,7 @@ u32 ieee80211_mandatory_rates(struct ieee80211_local *local,
void ieee80211_dynamic_ps_enable_work(struct work_struct *work);
void ieee80211_dynamic_ps_disable_work(struct work_struct *work);
void ieee80211_dynamic_ps_timer(unsigned long data);
-void ieee80211_send_nullfunc(struct ieee80211_local *local,
+bool ieee80211_send_nullfunc(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
int powersave);
void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
@@ -1627,7 +1627,7 @@ struct sk_buff *ieee80211_build_probe_req(struct ieee80211_sub_if_data *sdata,
const u8 *ssid, size_t ssid_len,
const u8 *ie, size_t ie_len,
bool directed);
-void ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
+bool ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
const u8 *ssid, size_t ssid_len,
const u8 *ie, size_t ie_len,
u32 ratemask, bool directed, bool no_cck,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 6757ee2..7211344 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -664,7 +664,7 @@ void ieee80211_send_pspoll(struct ieee80211_local *local,
ieee80211_tx_skb(sdata, skb);
}
-void ieee80211_send_nullfunc(struct ieee80211_local *local,
+bool ieee80211_send_nullfunc(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
int powersave)
{
@@ -674,7 +674,7 @@ void ieee80211_send_nullfunc(struct ieee80211_local *local,
skb = ieee80211_nullfunc_get(&local->hw, &sdata->vif);
if (!skb)
- return;
+ return false;
nullfunc = (struct ieee80211_hdr_3addr *) skb->data;
if (powersave)
@@ -685,7 +685,7 @@ void ieee80211_send_nullfunc(struct ieee80211_local *local,
IEEE80211_STA_CONNECTION_POLL))
IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_CTL_USE_MINRATE;
- ieee80211_tx_skb_offchannel(sdata, skb);
+ return ieee80211_tx_skb_offchannel(sdata, skb) == IEEE80211_TX_OK;
}
static void ieee80211_send_4addr_nullfunc(struct ieee80211_local *local,
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 5b9b3b8..650af94 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -24,10 +24,11 @@
* because we *may* be doing work on-operating channel, and want our
* hardware unconditionally awake, but still let the AP send us normal frames.
*/
-static void ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
+static bool ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
+ bool ret = true;
local->offchannel_ps_enabled = false;
@@ -57,7 +58,9 @@ static void ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
* to send a new nullfunc frame to inform the AP that we
* are again sleeping.
*/
- ieee80211_send_nullfunc(local, sdata, 1);
+ ret = ieee80211_send_nullfunc(local, sdata, 1);
+
+ return ret;
}
/* inform AP that we are awake again, unless power save is enabled */
@@ -102,12 +105,13 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
ieee80211_sta_reset_conn_monitor(sdata);
}
-void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local)
+bool ieee80211_offchannel_stop_vifs(struct ieee80211_local *local)
{
struct ieee80211_sub_if_data *sdata;
+ bool ret = true;
if (WARN_ON(local->use_chanctx))
- return;
+ return false;
/*
* notify the AP about us leaving the channel and stop all
@@ -138,10 +142,18 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local)
}
if (sdata->vif.type == NL80211_IFTYPE_STATION &&
- sdata->u.mgd.associated)
- ieee80211_offchannel_ps_enable(sdata);
+ sdata->u.mgd.associated) {
+ ret = ieee80211_offchannel_ps_enable(sdata);
+ if (!ret)
+ break;
+ }
}
mutex_unlock(&local->iflist_mtx);
+
+ /* Attempt to recover if failed */
+ if (!ret)
+ ieee80211_offchannel_return(local);
+ return ret;
}
void ieee80211_offchannel_return(struct ieee80211_local *local)
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 607684c..894c95e 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -340,7 +340,8 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
local->next_scan_state = SCAN_DECISION;
local->scan_channel_idx = 0;
- ieee80211_offchannel_stop_vifs(local);
+ if (!ieee80211_offchannel_stop_vifs(local))
+ goto error;
ieee80211_configure_filter(local);
@@ -351,6 +352,10 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
&local->scan_work, 0);
return 0;
+
+error:
+ drv_sw_scan_complete(local);
+ return -EBUSY;
}
static bool ieee80211_can_scan(struct ieee80211_local *local,
@@ -390,26 +395,32 @@ static void ieee80211_scan_state_send_probe(struct ieee80211_local *local,
int i;
struct ieee80211_sub_if_data *sdata;
enum ieee80211_band band = local->hw.conf.channel->band;
+ bool success = true;
sdata = rcu_dereference_protected(local->scan_sdata,
lockdep_is_held(&local->mtx));
- for (i = 0; i < local->scan_req->n_ssids; i++)
- ieee80211_send_probe_req(
- sdata, NULL,
- local->scan_req->ssids[i].ssid,
- local->scan_req->ssids[i].ssid_len,
- local->scan_req->ie, local->scan_req->ie_len,
- local->scan_req->rates[band], false,
- local->scan_req->no_cck,
- local->hw.conf.channel, true);
+ for (i = 0; success && i < local->scan_req->n_ssids; i++)
+ success = ieee80211_send_probe_req(
+ sdata, NULL,
+ local->scan_req->ssids[i].ssid,
+ local->scan_req->ssids[i].ssid_len,
+ local->scan_req->ie, local->scan_req->ie_len,
+ local->scan_req->rates[band], false,
+ local->scan_req->no_cck,
+ local->hw.conf.channel, true);
/*
* After sending probe requests, wait for probe responses
* on the channel.
*/
- *next_delay = IEEE80211_CHANNEL_TIME;
- local->next_scan_state = SCAN_DECISION;
+ if (success) {
+ *next_delay = IEEE80211_CHANNEL_TIME;
+ local->next_scan_state = SCAN_DECISION;
+ } else {
+ *next_delay = 0;
+ local->next_scan_state = SCAN_ABORT;
+ }
}
static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
@@ -688,7 +699,11 @@ static void ieee80211_scan_state_suspend(struct ieee80211_local *local,
static void ieee80211_scan_state_resume(struct ieee80211_local *local,
unsigned long *next_delay)
{
- ieee80211_offchannel_stop_vifs(local);
+ if (!ieee80211_offchannel_stop_vifs(local)) {
+ *next_delay = 0;
+ local->next_scan_state = SCAN_ABORT;
+ return;
+ }
if (local->ops->flush) {
drv_flush(local, false);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 5259557..0923892 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1274,13 +1274,14 @@ struct sk_buff *ieee80211_build_probe_req(struct ieee80211_sub_if_data *sdata,
return skb;
}
-void ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
+bool ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
const u8 *ssid, size_t ssid_len,
const u8 *ie, size_t ie_len,
u32 ratemask, bool directed, bool no_cck,
struct ieee80211_channel *channel, bool scan)
{
struct sk_buff *skb;
+ enum ieee80211_tx_status tx_stat = IEEE80211_TX_DROPPED;
skb = ieee80211_build_probe_req(sdata, dst, ratemask, channel,
ssid, ssid_len,
@@ -1290,11 +1291,14 @@ void ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
IEEE80211_SKB_CB(skb)->flags |=
IEEE80211_TX_CTL_NO_CCK_RATE;
if (scan)
- ieee80211_tx_skb_tid_band(sdata, skb, 7,
- channel->band, true);
+ tx_stat = ieee80211_tx_skb_tid_band(sdata, skb, 7,
+ channel->band,
+ true);
else
- ieee80211_tx_skb(sdata, skb);
+ tx_stat = ieee80211_tx_skb(sdata, skb);
}
+
+ return tx_stat != IEEE80211_TX_DROPPED;
}
u32 ieee80211_sta_get_rates(struct ieee80211_local *local,
--
1.7.9.5
On Wed, 2013-02-06 at 11:09 -0600, Seth Forshee wrote:
> > Does it just mean "I support actually turning off the radio"? But then
> > what's the difference to supporting powersave? Can we maybe just
> > disregard wl1251, which has the stupidest powersave implementation on
> > the planet, and solve the "normal" problems first? :)
>
> PS_DOZE means it actually supports putting the hardware into a low-power
> state for powersave. I did take the term from the spec (802.11-2012). It
> is usually used with regard to mesh, but it also appears wrt
> infrastructure BSS (see especially 10.2.1.2 which defines both awake and
> doze in the context of infrastructure networks).
>
> I'm open to other terms, doze just seems to be consistent with the spec.
Thanks for the reference. I had actually thought the term seemed
familiar but then only found mesh references ... need to brush up my
spec knowledge I guess :)
> I haven't considered wl1251 specifically, only enough to update it so
> that it continues to build.
Ah, wl1251 is the weird thing that wants to be told when to wake up/go
to sleep, and then sends nulldata packets itself ... so when we send
nulldata packets *again* for off-channel, because it also uses software
scanning. Like I said before -- "stupidest powersave implementation on
the planet".
> Maybe what's confusing here is that I'm making a differentiation between
> "powersave" as a mac-level feature and "powersave" as a low-power
> hardware state. Which is why I'm trying to change the mac80211
> terminology around so that "doze" now means the low-power state and
> "powersave" refers only to the state in which the AP is bufferring
> frames for us.
Right, ok. Maybe another term would be worthwhile? I mean, *all*
hardware has to support "MAC-level powersave" since you always want
background scanning. The question is how it's implemented ... I'm off
for dinner in a minute but I'll think about it a bit.
> So using these definition powersave is already a mandatory feature for
> any hardware which uses software scanning.
Even offloaded scanning, it's just not visible to mac80211 then.
> All I'm really doing is
> making this explicit, and drivers would now opt in to being placed into
> the doze state rather than opting into powersave in general. And of
> course drivers are now told about transitions to the non-doze powersave
> state (what I'm calling offchannel), so that drivers which need to do
> hardware configuration for this state can do so.
Ok, so maybe just calling it offchannel makes more sense, although that
really interacts only this way with powersave in managed mode, in P2P GO
mode .... oh well.
> > > In practice the changes shouldn't end up much different than what I have
> > > in these patches, but I think it's conceptually cleaner (and less
> > > confusing!). Does this sound reasonable to you?
> >
> > Not really sure I understand it enough to comment :)
>
> I've got working patches now, so maybe those will make everything clear.
> I need to do a little more testing and give them a quick review, then
> I'll send them out.
Ok.
johannes
On Thu, Jan 31, 2013 at 06:50:34PM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 11:18 -0600, Seth Forshee wrote:
>
> > > > Actually one of the last bugs I fixed before sending these was a place
> > > > where I had used disabled instead of !enabled, and the frames ended up
> > > > with PM set when it shouldn't have been.
> > > >
> > > > I agree though that the distinction is confusing. Maybe some better
> > > > state names are needed. Perhaps awake, offchannel, and doze?
> > >
> > > I think what you really want is to distinguish between "HW can go to
> > > powersave" and "PM bit should be set"? That's pretty much what your
> > > CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe
> >
> > Correct, with the understanding that "HW can go to powersave" also
> > implies "PM bit should be set."
> >
> > Another approach would be to keep the CONF_PS flag the same and add a
> > CONF_PM flag or similar. I didn't go with this approach because CONF_PS
> > && !CONF_PM really doesn't make any sense, which really doesn't help
> > with reducing confusion. The advantage is that it separates setting PM
> > from PS for those driver that don't support PS but need to configure the
> > hardware to set PM for off-channel.
>
> Good point, that'd work too. PS && !PM would never be used, I guess?
> It'd also have the advantage of not having to touch all the drivers?
>
> It doesn't really matter all that much to me though, I just think what
> you have right now is (too) confusing.
Hi Johannes,
I've been thinking about and playing around with these ideas. I've
implemented the CONF_PM idea, and it does end up with fewer changes, but
I just don't think separating powersave from setting PM makes much
sense. In the end it just seems like a kludge to fix a problem with
Broadcom chips, and if I want a kludge I can do it entirely within the
driver.
So what I'm planning to do know is implement the awake/doze/offchannel
powersave modes like I had mentioned, but taking things a bit further.
I'd change IEEE80211_HW_SUPPORTS_PS to SUPPORTS_PS_DOZE, indicating
support for a low-power powersave state rather than support for
powersave in general. All hardware will be reconfigured for
awake<->offchannel transitions (though most drivers can simply ignore
these transitions), but hardware will only be put in the doze state if
it indicates PS_DOZE support.
This will make it compulsory for all drivers to indicate whether or not
they require IEEE802111_HW_PS_NULLFUNC_STACK. I'll simply set this flag
for any drivers not currently supporting powersave.
In practice the changes shouldn't end up much different than what I have
in these patches, but I think it's conceptually cleaner (and less
confusing!). Does this sound reasonable to you?
Thanks,
Seth
Hi Seth,
> I've been thinking about and playing around with these ideas. I've
> implemented the CONF_PM idea, and it does end up with fewer changes, but
> I just don't think separating powersave from setting PM makes much
> sense. In the end it just seems like a kludge to fix a problem with
> Broadcom chips, and if I want a kludge I can do it entirely within the
> driver.
>
> So what I'm planning to do know is implement the awake/doze/offchannel
> powersave modes like I had mentioned, but taking things a bit further.
> I'd change IEEE80211_HW_SUPPORTS_PS to SUPPORTS_PS_DOZE, indicating
> support for a low-power powersave state rather than support for
> powersave in general.
Hm, I'm not sure I fully understand this. What does PS_DOZE mean then? I
think in the spec doze is a specific term for mesh?
Does it just mean "I support actually turning off the radio"? But then
what's the difference to supporting powersave? Can we maybe just
disregard wl1251, which has the stupidest powersave implementation on
the planet, and solve the "normal" problems first? :)
> All hardware will be reconfigured for
> awake<->offchannel transitions (though most drivers can simply ignore
> these transitions), but hardware will only be put in the doze state if
> it indicates PS_DOZE support.
>
> This will make it compulsory for all drivers to indicate whether or not
> they require IEEE802111_HW_PS_NULLFUNC_STACK. I'll simply set this flag
> for any drivers not currently supporting powersave.
That seems odd, why would they advertise they want null-data packets for
powersave, when they don't have powersave? Just for the interaction with
scan/offchannel?
> In practice the changes shouldn't end up much different than what I have
> in these patches, but I think it's conceptually cleaner (and less
> confusing!). Does this sound reasonable to you?
Not really sure I understand it enough to comment :)
johannes
On Wed, Feb 06, 2013 at 05:48:10PM +0100, Johannes Berg wrote:
> Hi Seth,
>
> > I've been thinking about and playing around with these ideas. I've
> > implemented the CONF_PM idea, and it does end up with fewer changes, but
> > I just don't think separating powersave from setting PM makes much
> > sense. In the end it just seems like a kludge to fix a problem with
> > Broadcom chips, and if I want a kludge I can do it entirely within the
> > driver.
> >
> > So what I'm planning to do know is implement the awake/doze/offchannel
> > powersave modes like I had mentioned, but taking things a bit further.
> > I'd change IEEE80211_HW_SUPPORTS_PS to SUPPORTS_PS_DOZE, indicating
> > support for a low-power powersave state rather than support for
> > powersave in general.
>
> Hm, I'm not sure I fully understand this. What does PS_DOZE mean then? I
> think in the spec doze is a specific term for mesh?
>
> Does it just mean "I support actually turning off the radio"? But then
> what's the difference to supporting powersave? Can we maybe just
> disregard wl1251, which has the stupidest powersave implementation on
> the planet, and solve the "normal" problems first? :)
PS_DOZE means it actually supports putting the hardware into a low-power
state for powersave. I did take the term from the spec (802.11-2012). It
is usually used with regard to mesh, but it also appears wrt
infrastructure BSS (see especially 10.2.1.2 which defines both awake and
doze in the context of infrastructure networks).
I'm open to other terms, doze just seems to be consistent with the spec.
I haven't considered wl1251 specifically, only enough to update it so
that it continues to build.
> > All hardware will be reconfigured for
> > awake<->offchannel transitions (though most drivers can simply ignore
> > these transitions), but hardware will only be put in the doze state if
> > it indicates PS_DOZE support.
> >
> > This will make it compulsory for all drivers to indicate whether or not
> > they require IEEE802111_HW_PS_NULLFUNC_STACK. I'll simply set this flag
> > for any drivers not currently supporting powersave.
>
> That seems odd, why would they advertise they want null-data packets for
> powersave, when they don't have powersave? Just for the interaction with
> scan/offchannel?
Yes.
Maybe what's confusing here is that I'm making a differentiation between
"powersave" as a mac-level feature and "powersave" as a low-power
hardware state. Which is why I'm trying to change the mac80211
terminology around so that "doze" now means the low-power state and
"powersave" refers only to the state in which the AP is bufferring
frames for us.
So using these definition powersave is already a mandatory feature for
any hardware which uses software scanning. All I'm really doing is
making this explicit, and drivers would now opt in to being placed into
the doze state rather than opting into powersave in general. And of
course drivers are now told about transitions to the non-doze powersave
state (what I'm calling offchannel), so that drivers which need to do
hardware configuration for this state can do so.
> > In practice the changes shouldn't end up much different than what I have
> > in these patches, but I think it's conceptually cleaner (and less
> > confusing!). Does this sound reasonable to you?
>
> Not really sure I understand it enough to comment :)
I've got working patches now, so maybe those will make everything clear.
I need to do a little more testing and give them a quick review, then
I'll send them out.
Seth
On Wed, Feb 06, 2013 at 06:44:16PM +0100, Johannes Berg wrote:
> > I haven't considered wl1251 specifically, only enough to update it so
> > that it continues to build.
>
> Ah, wl1251 is the weird thing that wants to be told when to wake up/go
> to sleep, and then sends nulldata packets itself ... so when we send
> nulldata packets *again* for off-channel, because it also uses software
> scanning. Like I said before -- "stupidest powersave implementation on
> the planet".
Ah, I had wondered what hardware that code was there for :-)
So I think the approach I'm advocating could possibly make things better
for wl1251. If it's told about going to off-channel powersave rather
than being told it's leaving powersave, maybe it can do the right thing
with the nullfunc frames without mac80211 generating the extra nullfunc.
I don't know that for sure though because I don't understand how the
hardware works.
> > So using these definition powersave is already a mandatory feature for
> > any hardware which uses software scanning.
>
> Even offloaded scanning, it's just not visible to mac80211 then.
Okay, sure. I'm just thinking of things from the mac80211 perspective.
> > All I'm really doing is
> > making this explicit, and drivers would now opt in to being placed into
> > the doze state rather than opting into powersave in general. And of
> > course drivers are now told about transitions to the non-doze powersave
> > state (what I'm calling offchannel), so that drivers which need to do
> > hardware configuration for this state can do so.
>
> Ok, so maybe just calling it offchannel makes more sense, although that
> really interacts only this way with powersave in managed mode, in P2P GO
> mode .... oh well.
I admit I've only been considering managed mode and know next to nothing
about P2P GO mode. If I'm creating problems for any other modes I'd
definitely like to know about it so I can take it into consideration.
Seth
On Wed, 2013-02-06 at 12:02 -0600, Seth Forshee wrote:
> > Ah, wl1251 is the weird thing that wants to be told when to wake up/go
> > to sleep, and then sends nulldata packets itself ... so when we send
> > nulldata packets *again* for off-channel, because it also uses software
> > scanning. Like I said before -- "stupidest powersave implementation on
> > the planet".
>
> Ah, I had wondered what hardware that code was there for :-)
I was actually wrong -- wl1251 has HW scan so it doesn't matter.
However, iwlegacy does this...
> So I think the approach I'm advocating could possibly make things better
> for wl1251. If it's told about going to off-channel powersave rather
> than being told it's leaving powersave, maybe it can do the right thing
> with the nullfunc frames without mac80211 generating the extra nullfunc.
> I don't know that for sure though because I don't understand how the
> hardware works.
Probably not. For iwlegacy anyway it won't help.
> > > So using these definition powersave is already a mandatory feature for
> > > any hardware which uses software scanning.
> >
> > Even offloaded scanning, it's just not visible to mac80211 then.
>
> Okay, sure. I'm just thinking of things from the mac80211 perspective.
Right, sure.
> > > All I'm really doing is
> > > making this explicit, and drivers would now opt in to being placed into
> > > the doze state rather than opting into powersave in general. And of
> > > course drivers are now told about transitions to the non-doze powersave
> > > state (what I'm calling offchannel), so that drivers which need to do
> > > hardware configuration for this state can do so.
> >
> > Ok, so maybe just calling it offchannel makes more sense, although that
> > really interacts only this way with powersave in managed mode, in P2P GO
> > mode .... oh well.
>
> I admit I've only been considering managed mode and know next to nothing
> about P2P GO mode. If I'm creating problems for any other modes I'd
> definitely like to know about it so I can take it into consideration.
No no, I'm just saying there "powersave" could have even more
meanings ...
johannes