2013-02-06 15:01:22

by Seth Forshee

Subject: [PATCH 0/4] Improve queue handling for off-channel operation

Hi Johannes,

Here are some updates to the tx queue handling patches during
off-channel operation that were part of the larger series I sent last
week. I didn't see any reason to sit on these while I finished
implementing the powersave changes. This includes the patches to return
a status from tx operations, add an off-channel queue stop reason, and
improve error handling when going off-channel.

Changes since v1:
* Add unlikely() to the off-channel condition in ieee80211_tx_frags()
* Expand the queue stop reasons that will cause off-channel frames to
be dropped to include everything except the off-channel stop reason
* Remove changes to abort scans when sending of probe request frames
failed

Thanks,
Seth


Seth Forshee (4):
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

net/mac80211/agg-tx.c | 4 +-
net/mac80211/cfg.c | 2 +-
net/mac80211/ht.c | 2 +-
net/mac80211/ieee80211_i.h | 70 ++++++++++++++++++------
net/mac80211/mlme.c | 6 +-
net/mac80211/offchannel.c | 55 +++++++++++--------
net/mac80211/scan.c | 16 +++++-
net/mac80211/sta_info.c | 2 +-
net/mac80211/tx.c | 130 +++++++++++++++++++++++++-------------------
net/mac80211/util.c | 3 +-
10 files changed, 182 insertions(+), 108 deletions(-)

2013-02-08 17:17:38

by Johannes Berg

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, 2013-02-08 at 11:11 -0600, Seth Forshee wrote:

> > I started wondering -- is there a reason to modify the entire TX path?
> > Could we maybe bypass it instead and call the driver's TX op almost
> > directly? The frames in question don't really need much TX handling, the
> > only thing that might be relevant _could_ be rate control but even that
> > I'd argue isn't really needed, just using rate_control_send_low() should
> > be ok (by setting IEEE80211_TX_CTL_USE_MINRATE it will always return
> > true). For the null data packets the sta pointer is also obvious, the AP
> > station (BSSID) ... we don't need any of the extra monitor/whatever
> > handling either.
> >
> > That might be simpler overall?
>
> Okay, I'll take a look at this.
>
> Another option that might simplify things a bit would be to use a
> ieee80211_tx_data flag. If I added another interface into tx.c for
> offchannel frames then the offchan argument would only be needed for
> ieee80211_xmit() and ieee80211_tx(). Though it would be nice to avoid
> adding an argument to ieee80211_xmit().

Oh, I forgot all about ROC and off-channel frames there, but those are
public action frames (only?) so the same applies with min-rate etc.

Overall I'm not sure. On the one hand that might make the code changes
simpler, on the other it might make the code more complex?

johannes

2013-02-06 22:26:20

by Johannes Berg

Subject: Re: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

On Wed, 2013-02-06 at 16:20 -0600, Seth Forshee wrote:

> > > I'd considered trying to expand this to check whether or not the frame
> > > was acked -- in fact just today I captured a trace where the AP didn't
> > > ack the frame but the STA went off-channel anyway. I'm not sure how to
> > > implement that yet, and haven't found time to look into it.
> >
> > It means waiting for the TX status from the driver, which might not
> > really come with all drivers at all, making it somewhat tricky in
> > general.
> >
> > Anyway my point is that this doesn't really help all that much, and
> > patch 1 and 3 together is a lot of churn ...
>
> Fair enough. If you want I can drop those and resend 2 and 4, because
> they do fix real problems.

Please. I'll quickly go over them for comments first. I was going to say
I can apply them but I don't think they'll compile :)

johannes

2013-02-06 15:01:29

by Seth Forshee

Subject: [PATCH 4/4] mac80211: Add flushes before going off-channel

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 beca4db..9ecd1660 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

2013-02-08 17:12:02

by Seth Forshee

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, Feb 08, 2013 at 10:03:32AM +0100, Johannes Berg wrote:
> On Thu, 2013-02-07 at 11:54 -0600, Seth Forshee wrote:
> > 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 be passed down to the driver if the queues have only been stopped for
> > off-channel. Add ieee80211_tx_skb_offchannel() for transmitting
> > off-channel frames with this flag set.
>
> I started wondering -- is there a reason to modify the entire TX path?
> Could we maybe bypass it instead and call the driver's TX op almost
> directly? The frames in question don't really need much TX handling, the
> only thing that might be relevant _could_ be rate control but even that
> I'd argue isn't really needed, just using rate_control_send_low() should
> be ok (by setting IEEE80211_TX_CTL_USE_MINRATE it will always return
> true). For the null data packets the sta pointer is also obvious, the AP
> station (BSSID) ... we don't need any of the extra monitor/whatever
> handling either.
>
> That might be simpler overall?

Okay, I'll take a look at this.

Another option that might simplify things a bit would be to use a
ieee80211_tx_data flag. If I added another interface into tx.c for
offchannel frames then the offchan argument would only be needed for
ieee80211_xmit() and ieee80211_tx(). Though it would be nice to avoid
adding an argument to ieee80211_xmit().

Seth

2013-02-07 17:54:47

by Seth Forshee

Subject: [PATCH v3 2/2] mac80211: Add flushes before going off-channel

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 5b9b3b8..c5abdd4 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -114,8 +114,13 @@ void 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 607684c..d2304fd 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -342,6 +342,9 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)

ieee80211_offchannel_stop_vifs(local);

+ /* 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

2013-02-11 17:21:16

by Seth Forshee

Subject: [PATCH v4 1/2] mac80211: Fix tx queue handling during scans

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 meant to
be sent off-channel. Add a new tx control flag,
IEEE80211_TX_CTL_OFFCHAN_TX_OK, which allows frames to be transmitted
when the queues are stopped only for the off-channel stop reason. Update
all locations transmitting off-channel frames to use this flag.

Signed-off-by: Seth Forshee <[email protected]>
---
include/net/mac80211.h | 4 ++++
net/mac80211/cfg.c | 3 ++-
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 3 ++-
net/mac80211/offchannel.c | 30 ++++++++++--------------------
net/mac80211/scan.c | 9 ++++++---
net/mac80211/tx.c | 17 +++++++++++++++--
7 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 7da1121..8f51e96 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -391,6 +391,9 @@ struct ieee80211_bss_conf {
* @IEEE80211_TX_CTL_RATE_CTRL_PROBE: internal to mac80211, can be
* set by rate control algorithms to indicate probe rate, will
* be cleared for fragmented frames (except on the last fragment)
+ * @IEEE80211_TX_CTL_OFFCHAN_TX_OK: Internal to mac80211. Used to indicate
+ * that a frame can be transmitted while the queues are stopped for
+ * off-channel operation.
* @IEEE80211_TX_INTFL_NEED_TXPROCESSING: completely internal to mac80211,
* used to indicate that a pending frame requires TX processing before
* it can be sent out.
@@ -456,6 +459,7 @@ enum mac80211_tx_control_flags {
IEEE80211_TX_STAT_AMPDU = BIT(10),
IEEE80211_TX_STAT_AMPDU_NO_BACK = BIT(11),
IEEE80211_TX_CTL_RATE_CTRL_PROBE = BIT(12),
+ IEEE80211_TX_CTL_OFFCHAN_TX_OK = BIT(13),
IEEE80211_TX_INTFL_NEED_TXPROCESSING = BIT(14),
IEEE80211_TX_INTFL_RETRIED = BIT(15),
IEEE80211_TX_INTFL_DONT_ENCRYPT = BIT(16),
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f4f7e76..3c7f9d8 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2747,7 +2747,8 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
goto out_unlock;
}

- IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_CTL_TX_OFFCHAN;
+ IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_CTL_TX_OFFCHAN |
+ IEEE80211_TX_CTL_OFFCHAN_TX_OK;
if (local->hw.flags & IEEE80211_HW_QUEUE_CONTROL)
IEEE80211_SKB_CB(skb)->hw_queue =
local->hw.offchannel_tx_hw_queue;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5fe9db7..815fb80 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -812,6 +812,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
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 353b690..b459f7a 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -685,7 +685,8 @@ void ieee80211_send_nullfunc(struct ieee80211_local *local,
if (powersave)
nullfunc->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);

- IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_INTFL_DONT_ENCRYPT;
+ IEEE80211_SKB_CB(skb)->flags |= (IEEE80211_TX_INTFL_DONT_ENCRYPT |
+ IEEE80211_TX_CTL_OFFCHAN_TX_OK);
if (ifmgd->flags & (IEEE80211_STA_BEACON_POLL |
IEEE80211_STA_CONNECTION_POLL))
IEEE80211_SKB_CB(skb)->flags |= IEEE80211_TX_CTL_USE_MINRATE;
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 82baf5b..4c3ee3e 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)
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 7f80f0a..29e7d6e 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -378,6 +378,11 @@ 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;
+ u32 tx_flags;
+
+ tx_flags = IEEE80211_TX_CTL_OFFCHAN_TX_OK;
+ if (local->scan_req->no_cck)
+ tx_flags |= IEEE80211_TX_CTL_NO_CCK_RATE;

sdata = rcu_dereference_protected(local->scan_sdata,
lockdep_is_held(&local->mtx));
@@ -389,9 +394,7 @@ static void ieee80211_scan_state_send_probe(struct ieee80211_local *local,
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 ?
- IEEE80211_TX_CTL_NO_CCK_RATE : 0,
- local->hw.conf.channel, true);
+ tx_flags, local->hw.conf.channel, true);

/*
* After sending probe requests, wait for probe responses
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 2ef0e19..ab603d7 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1228,8 +1228,21 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
#endif

spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
- if (local->queue_stop_reasons[q] ||
- (!txpending && !skb_queue_empty(&local->pending[q]))) {
+ if (unlikely(info->flags & IEEE80211_TX_CTL_OFFCHAN_TX_OK)) {
+ /*
+ * Drop off-channel frames if queues are stopped for
+ * any reason other than off-channel operation. Never
+ * queue them.
+ */
+ if (local->queue_stop_reasons[q] &
+ ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL)) {
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock,
+ flags);
+ ieee80211_purge_tx_queue(&local->hw, skbs);
+ return true;
+ }
+ } 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
--
1.7.9.5

2013-02-06 22:05:22

by Seth Forshee

Subject: Re: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

On Wed, Feb 06, 2013 at 10:44:37PM +0100, Johannes Berg wrote:
> On Wed, 2013-02-06 at 09:01 -0600, Seth Forshee wrote:
> > Errors in sending the nullfunc frame to set powersave at the AP for
> > off-channel operation can lead to high packet loss. Add error handling
> > to fail going off-channel when this happens, and return an error to
> > userspace.
>
> With the flushes in place, have you ever seen this fail? This and patch
> ones seems like a lot of churn for only half of what you'd want -- what
> you really want is to know if the AP ACKed the frame...

That's a good point. I've seen iw fail to initiate scans, but I can't
say whether or not any of them was due to the queues being stopped for
some reason. When I was testing NetworkManager was still managing the
interface, so at least some failures were undobtedly because another
scan was ongoing.

I'd considered trying to expand this to check whether or not the frame
was acked -- in fact just today I captured a trace where the AP didn't
ack the frame but the STA went off-channel anyway. I'm not sure how to
implement that yet, and haven't found time to look into it.

Seth

2013-02-06 22:10:28

by Johannes Berg

Subject: Re: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

On Wed, 2013-02-06 at 16:05 -0600, Seth Forshee wrote:
> On Wed, Feb 06, 2013 at 10:44:37PM +0100, Johannes Berg wrote:
> > On Wed, 2013-02-06 at 09:01 -0600, Seth Forshee wrote:
> > > Errors in sending the nullfunc frame to set powersave at the AP for
> > > off-channel operation can lead to high packet loss. Add error handling
> > > to fail going off-channel when this happens, and return an error to
> > > userspace.
> >
> > With the flushes in place, have you ever seen this fail? This and patch
> > ones seems like a lot of churn for only half of what you'd want -- what
> > you really want is to know if the AP ACKed the frame...
>
> That's a good point. I've seen iw fail to initiate scans, but I can't
> say whether or not any of them was due to the queues being stopped for
> some reason. When I was testing NetworkManager was still managing the
> interface, so at least some failures were undobtedly because another
> scan was ongoing.

Yeah you'd expect that. I think you could tell the difference -- EBUSY
vs. whatever other error code you chose?

> I'd considered trying to expand this to check whether or not the frame
> was acked -- in fact just today I captured a trace where the AP didn't
> ack the frame but the STA went off-channel anyway. I'm not sure how to
> implement that yet, and haven't found time to look into it.

It means waiting for the TX status from the driver, which might not
really come with all drivers at all, making it somewhat tricky in
general.

Anyway my point is that this doesn't really help all that much, and
patch 1 and 3 together is a lot of churn ...

johannes

2013-02-06 22:30:07

by Johannes Berg

Subject: Re: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

On Wed, 2013-02-06 at 23:26 +0100, Johannes Berg wrote:
> On Wed, 2013-02-06 at 16:20 -0600, Seth Forshee wrote:
>
> > > > I'd considered trying to expand this to check whether or not the frame
> > > > was acked -- in fact just today I captured a trace where the AP didn't
> > > > ack the frame but the STA went off-channel anyway. I'm not sure how to
> > > > implement that yet, and haven't found time to look into it.
> > >
> > > It means waiting for the TX status from the driver, which might not
> > > really come with all drivers at all, making it somewhat tricky in
> > > general.
> > >
> > > Anyway my point is that this doesn't really help all that much, and
> > > patch 1 and 3 together is a lot of churn ...
> >
> > Fair enough. If you want I can drop those and resend 2 and 4, because
> > they do fix real problems.
>
> Please. I'll quickly go over them for comments first. I was going to say
> I can apply them but I don't think they'll compile :)

Actually they look fine. I thought the purge queue thing was weird but
it's necessary, just wasn't obvious to me right now.

johannes

2013-02-06 15:01:23

by Seth Forshee

Subject: [PATCH 1/4] mac80211: Return a status for tx operations

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 5fba867..edea19e 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1494,46 +1494,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 a2cb6a3..fde1bc9 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

2013-02-06 15:01:25

by Seth Forshee

Subject: [PATCH 2/4] mac80211: Fix tx queue handling during scans

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 be passed down to the driver if the queues have only been stopped for
off-channel. 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 | 45 +++++++++++++++++++++++++++++---------------
net/mac80211/util.c | 3 ++-
9 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 13b7683..f0840b3 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 edea19e..505ff3c 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -840,6 +840,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
@@ -1509,22 +1510,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;
@@ -1532,7 +1534,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;
@@ -1546,7 +1548,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;
@@ -1556,7 +1559,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 5913fb9..d5a3cf7 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 227233c..f57247c 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1117,7 +1117,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 fde1bc9..2619f4a 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,21 @@ 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 (unlikely(offchan_tx_ok)) {
+ /*
+ * Drop off-channel frames if queues are stopped for
+ * any reason other than off-channel operation. Never
+ * queue them.
+ */
+ if (local->queue_stop_reasons[q] &
+ ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL)) {
+ 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 +1266,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 +1315,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 +1383,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 +1426,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 +1464,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 +1497,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 +1717,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 +2161,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 +2212,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 +2222,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 +2794,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 +2812,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

2013-02-11 17:18:33

by Seth Forshee

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, Feb 08, 2013 at 09:53:12PM +0100, Johannes Berg wrote:
> On Fri, 2013-02-08 at 14:46 -0600, Seth Forshee wrote:
>
> > > > Well, I think the way that will be simplest with the fewest code changes
> > > > would be to use a tx control flag. Of course then we've gobbled up one
> > > > of the last available flags.
> > >
> > > Let's do that anyway then. I still think we need to do the
> > > PS/scan/offchannel thing I described in another mail anyway, so that'd
> > > be a better interim step than changing all the prototypes...
> >
> > When I originally looked at using a tx control flag I didn't think that
> > using IEEE80211_TX_CTL_TX_OFFCHAN would work, but now I'm not sure why.
> > Is there any reason not to do this?
>
> I guess you'd have to pre-assign the queue, since the code in
> ieee80211_tx() might skip that part, but that seems easy enough. Other
> than that, a driver that checks IEEE80211_TX_CTL_TX_OFFCHAN might treat
> that frame specially, but OTOH the only drivers using it right now are
> ours and TI's and they both have HW scan/roc, so your code never
> executes. What a future driver might do is anyone's guess...

All right, I decided to go ahead with the new flag since what I need it
to do is somewhat incongruous with what IEEE80211_TX_CTL_TX_OFFCHAN
indicates. New patches will follow.

Seth

2013-02-07 17:54:46

by Seth Forshee

Subject: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

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 be passed down to the driver if the queues have only been stopped for
off-channel. 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 | 45 +++++++++++++++++++++++++++++---------------
net/mac80211/util.c | 3 ++-
9 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 13b7683..f0840b3 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 5fba867..600ffd2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -840,6 +840,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
@@ -1495,24 +1496,25 @@ void mac80211_ev_michael_mic_failure(struct ieee80211_sub_if_data *sdata, int ke
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);
+ enum ieee80211_band band, bool offchan_tx_ok);

void __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 void
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)
{
rcu_read_lock();
- __ieee80211_tx_skb_tid_band(sdata, skb, tid, band);
+ __ieee80211_tx_skb_tid_band(sdata, skb, tid, band, offchan_tx_ok);
rcu_read_unlock();
}

static inline void ieee80211_tx_skb_tid(struct ieee80211_sub_if_data *sdata,
- struct sk_buff *skb, int tid)
+ struct sk_buff *skb, int tid,
+ bool offchan_tx_ok)
{
struct ieee80211_chanctx_conf *chanctx_conf;

@@ -1525,7 +1527,8 @@ static inline void ieee80211_tx_skb_tid(struct ieee80211_sub_if_data *sdata,
}

__ieee80211_tx_skb_tid_band(sdata, skb, tid,
- chanctx_conf->def.chan->band);
+ chanctx_conf->def.chan->band,
+ offchan_tx_ok);
rcu_read_unlock();
}

@@ -1533,7 +1536,15 @@ static inline void 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);
+ ieee80211_tx_skb_tid(sdata, skb, 7, false);
+}
+
+static inline void
+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. */
+ 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 5913fb9..d5a3cf7 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 227233c..f57247c 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1117,7 +1117,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 a2cb6a3..f477858 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1204,7 +1204,7 @@ static bool 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;
@@ -1223,8 +1223,21 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
#endif

spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
- if (local->queue_stop_reasons[q] ||
- (!txpending && !skb_queue_empty(&local->pending[q]))) {
+ if (unlikely(offchan_tx_ok)) {
+ /*
+ * Drop off-channel frames if queues are stopped for
+ * any reason other than off-channel operation. Never
+ * queue them.
+ */
+ if (local->queue_stop_reasons[q] &
+ ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL)) {
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock,
+ flags);
+ ieee80211_purge_tx_queue(&local->hw, skbs);
+ return true;
+ }
+ } 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
@@ -1257,7 +1270,8 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
*/
static bool __ieee80211_tx(struct ieee80211_local *local,
struct sk_buff_head *skbs, int led_len,
- struct sta_info *sta, bool txpending)
+ struct sta_info *sta, bool txpending,
+ bool offchan_tx_ok)
{
struct ieee80211_tx_info *info;
struct ieee80211_sub_if_data *sdata;
@@ -1305,7 +1319,7 @@ static bool __ieee80211_tx(struct ieee80211_local *local,
}

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);
@@ -1376,7 +1390,7 @@ static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
*/
static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata,
struct sk_buff *skb, bool txpending,
- enum ieee80211_band band)
+ enum ieee80211_band band, bool offchan_tx_ok)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_tx_data tx;
@@ -1411,7 +1425,7 @@ static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata,

if (!invoke_tx_handlers(&tx))
result = __ieee80211_tx(local, &tx.skbs, led_len,
- tx.sta, txpending);
+ tx.sta, txpending, offchan_tx_ok);

return result;
}
@@ -1448,7 +1462,7 @@ static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata,
}

void 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);
@@ -1481,7 +1495,7 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
}

ieee80211_set_qos_hdr(sdata, skb);
- ieee80211_tx(sdata, skb, false, band);
+ ieee80211_tx(sdata, skb, false, band, offchan_tx_ok);
}

static bool ieee80211_parse_tx_radiotap(struct sk_buff *skb)
@@ -1701,7 +1715,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;
@@ -2145,7 +2159,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;
@@ -2197,7 +2211,7 @@ static bool ieee80211_tx_pending_skb(struct ieee80211_local *local,
return true;
}
result = ieee80211_tx(sdata, skb, true,
- chanctx_conf->def.chan->band);
+ chanctx_conf->def.chan->band, false);
} else {
struct sk_buff_head skbs;

@@ -2207,7 +2221,8 @@ 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);
+ result = __ieee80211_tx(local, &skbs, skb->len, sta, true,
+ false);
}

return result;
@@ -2777,7 +2792,7 @@ 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_band band, bool offchan_tx_ok)
{
int ac = ieee802_1d_to_ac[tid & 7];

@@ -2794,6 +2809,6 @@ 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);
+ ieee80211_xmit(sdata, skb, band, offchan_tx_ok);
local_bh_enable();
}
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

2013-02-11 21:50:13

by Seth Forshee

Subject: Re: [PATCH v4 1/2] mac80211: Fix tx queue handling during scans

On Mon, Feb 11, 2013 at 10:40:44PM +0100, Johannes Berg wrote:
> On Mon, 2013-02-11 at 11:21 -0600, Seth Forshee wrote:
>
> > + * @IEEE80211_TX_CTL_OFFCHAN_TX_OK: Internal to mac80211. Used to indicate
> > + * that a frame can be transmitted while the queues are stopped for
> > + * off-channel operation.
>
> I'm renaming this to INTFL_ instead of CTL_, any objections? I don't
> think drivers should (need to) use it.

No objections. The drivers shouldn't need to use it, I just wasn't aware
of the distinction.

> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -1228,8 +1228,21 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
> > #endif
> >
> > spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> > - if (local->queue_stop_reasons[q] ||
> > - (!txpending && !skb_queue_empty(&local->pending[q]))) {
> > + if (unlikely(info->flags & IEEE80211_TX_CTL_OFFCHAN_TX_OK)) {
> > + /*
> > + * Drop off-channel frames if queues are stopped for
> > + * any reason other than off-channel operation. Never
> > + * queue them.
> > + */
> > + if (local->queue_stop_reasons[q] &
> > + ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL)) {
> > + spin_unlock_irqrestore(&local->queue_stop_reason_lock,
> > + flags);
> > + ieee80211_purge_tx_queue(&local->hw, skbs);
> > + return true;
> > + }
> > + } else if (local->queue_stop_reasons[q] ||
> > + (!txpending && !skb_queue_empty(&local->pending[q]))) {
>
> I think this bit would be better written like this:
>
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1230,6 +1230,21 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> if (local->queue_stop_reasons[q] ||
> (!txpending && !skb_queue_empty(&local->pending[q]))) {
> + if (unlikely(info->flags &
> + IEEE80211_TX_INTFL_OFFCHAN_TX_OK &&
> + local->queue_stop_reasons[q] &
> + ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL))) {
> + /*
> + * Drop off-channel frames if queues are stopped
> + * for any reason other than off-channel
> + * operation. Never queue them.
> + */
> + spin_unlock_irqrestore(
> + &local->queue_stop_reason_lock, flags);
> + ieee80211_purge_tx_queue(&local->hw, skbs);
> + return true;
> + }
> +
> /*
> * Since queue is stopped, queue up frames for later
> * transmission from the tx-pending tasklet when the
>
> That would avoid hitting the fast-path as much, since the queues being
> stopped is already something of a slow-path (and if they're stopped it
> doesn't matter much ... packets won't go out soon anyway.)

Your version is fine with me.

> I've made those changes, so just let me know if that seems OK, patch 2 I
> also applied.

Everything looks good. Thanks!

Seth

2013-02-06 21:44:15

by Johannes Berg

Subject: Re: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

On Wed, 2013-02-06 at 09:01 -0600, Seth Forshee wrote:
> Errors in sending the nullfunc frame to set powersave at the AP for
> off-channel operation can lead to high packet loss. Add error handling
> to fail going off-channel when this happens, and return an error to
> userspace.

With the flushes in place, have you ever seen this fail? This and patch
ones seems like a lot of churn for only half of what you'd want -- what
you really want is to know if the AP ACKed the frame...

johannes

2013-02-08 18:10:05

by Seth Forshee

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, Feb 08, 2013 at 06:17:32PM +0100, Johannes Berg wrote:
> On Fri, 2013-02-08 at 11:11 -0600, Seth Forshee wrote:
>
> > > I started wondering -- is there a reason to modify the entire TX path?
> > > Could we maybe bypass it instead and call the driver's TX op almost
> > > directly? The frames in question don't really need much TX handling, the
> > > only thing that might be relevant _could_ be rate control but even that
> > > I'd argue isn't really needed, just using rate_control_send_low() should
> > > be ok (by setting IEEE80211_TX_CTL_USE_MINRATE it will always return
> > > true). For the null data packets the sta pointer is also obvious, the AP
> > > station (BSSID) ... we don't need any of the extra monitor/whatever
> > > handling either.
> > >
> > > That might be simpler overall?
> >
> > Okay, I'll take a look at this.
> >
> > Another option that might simplify things a bit would be to use a
> > ieee80211_tx_data flag. If I added another interface into tx.c for
> > offchannel frames then the offchan argument would only be needed for
> > ieee80211_xmit() and ieee80211_tx(). Though it would be nice to avoid
> > adding an argument to ieee80211_xmit().

This actually doesn't work out. ieee80211_tx_data doesn't propogate down
to ieee80211_tx_frags() and isn't even there in the txpending case. So
without more substantial changes it just turns back into an argument
after a couple of levels.

> Oh, I forgot all about ROC and off-channel frames there, but those are
> public action frames (only?) so the same applies with min-rate etc.

I thought of that and was going to check and see what kind of ROC frames
are sent. Also probe request frames are obviously sent off-channel, but
I'd guess the same applies to them as well?

> Overall I'm not sure. On the one hand that might make the code changes
> simpler, on the other it might make the code more complex?

Well, I think the way that will be simplest with the fewest code changes
would be to use a tx control flag. Of course then we've gobbled up one
of the last available flags.

Seth

2013-02-08 20:46:37

by Seth Forshee

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, Feb 08, 2013 at 08:05:59PM +0100, Johannes Berg wrote:
> On Fri, 2013-02-08 at 12:10 -0600, Seth Forshee wrote:
>
> > Well, I think the way that will be simplest with the fewest code changes
> > would be to use a tx control flag. Of course then we've gobbled up one
> > of the last available flags.
>
> Let's do that anyway then. I still think we need to do the
> PS/scan/offchannel thing I described in another mail anyway, so that'd
> be a better interim step than changing all the prototypes...

When I originally looked at using a tx control flag I didn't think that
using IEEE80211_TX_CTL_TX_OFFCHAN would work, but now I'm not sure why.
Is there any reason not to do this?

Seth

2013-02-06 15:01:27

by Seth Forshee

Subject: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

Errors in sending the nullfunc frame to set powersave at the AP for
off-channel operation can lead to high packet loss. Add error handling
to fail going off-channel when this happens, and return an error to
userspace.

Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/ieee80211_i.h | 4 ++--
net/mac80211/mlme.c | 6 +++---
net/mac80211/offchannel.c | 24 ++++++++++++++++++------
net/mac80211/scan.c | 13 +++++++++++--
4 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 505ff3c..8adfdfb 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1368,7 +1368,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);
@@ -1581,7 +1581,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,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index d5a3cf7..0f4e21f 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..beca4db 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,
@@ -688,7 +693,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);
--
1.7.9.5

2013-02-11 17:21:18

by Seth Forshee

Subject: [PATCH v4 2/2] mac80211: Add flushes before going off-channel

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 4c3ee3e..cc79b4a 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -114,8 +114,13 @@ void 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 29e7d6e..ae422fa 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -330,6 +330,9 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)

ieee80211_offchannel_stop_vifs(local);

+ /* 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

2013-02-11 21:40:50

by Johannes Berg

Subject: Re: [PATCH v4 1/2] mac80211: Fix tx queue handling during scans

On Mon, 2013-02-11 at 11:21 -0600, Seth Forshee wrote:

> + * @IEEE80211_TX_CTL_OFFCHAN_TX_OK: Internal to mac80211. Used to indicate
> + * that a frame can be transmitted while the queues are stopped for
> + * off-channel operation.

I'm renaming this to INTFL_ instead of CTL_, any objections? I don't
think drivers should (need to) use it.


> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1228,8 +1228,21 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
> #endif
>
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> - if (local->queue_stop_reasons[q] ||
> - (!txpending && !skb_queue_empty(&local->pending[q]))) {
> + if (unlikely(info->flags & IEEE80211_TX_CTL_OFFCHAN_TX_OK)) {
> + /*
> + * Drop off-channel frames if queues are stopped for
> + * any reason other than off-channel operation. Never
> + * queue them.
> + */
> + if (local->queue_stop_reasons[q] &
> + ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL)) {
> + spin_unlock_irqrestore(&local->queue_stop_reason_lock,
> + flags);
> + ieee80211_purge_tx_queue(&local->hw, skbs);
> + return true;
> + }
> + } else if (local->queue_stop_reasons[q] ||
> + (!txpending && !skb_queue_empty(&local->pending[q]))) {

I think this bit would be better written like this:

--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1230,6 +1230,21 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local,
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
if (local->queue_stop_reasons[q] ||
(!txpending && !skb_queue_empty(&local->pending[q]))) {
+ if (unlikely(info->flags &
+ IEEE80211_TX_INTFL_OFFCHAN_TX_OK &&
+ local->queue_stop_reasons[q] &
+ ~BIT(IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL))) {
+ /*
+ * Drop off-channel frames if queues are stopped
+ * for any reason other than off-channel
+ * operation. Never queue them.
+ */
+ spin_unlock_irqrestore(
+ &local->queue_stop_reason_lock, flags);
+ ieee80211_purge_tx_queue(&local->hw, skbs);
+ return true;
+ }
+
/*
* Since queue is stopped, queue up frames for later
* transmission from the tx-pending tasklet when the

That would avoid hitting the fast-path as much, since the queues being
stopped is already something of a slow-path (and if they're stopped it
doesn't matter much ... packets won't go out soon anyway.)

I've made those changes, so just let me know if that seems OK, patch 2 I
also applied.

johannes

2013-02-08 09:03:37

by Johannes Berg

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Thu, 2013-02-07 at 11:54 -0600, Seth Forshee wrote:
> 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 be passed down to the driver if the queues have only been stopped for
> off-channel. Add ieee80211_tx_skb_offchannel() for transmitting
> off-channel frames with this flag set.

I started wondering -- is there a reason to modify the entire TX path?
Could we maybe bypass it instead and call the driver's TX op almost
directly? The frames in question don't really need much TX handling, the
only thing that might be relevant _could_ be rate control but even that
I'd argue isn't really needed, just using rate_control_send_low() should
be ok (by setting IEEE80211_TX_CTL_USE_MINRATE it will always return
true). For the null data packets the sta pointer is also obvious, the AP
station (BSSID) ... we don't need any of the extra monitor/whatever
handling either.

That might be simpler overall?

johannes

2013-02-08 19:06:05

by Johannes Berg

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, 2013-02-08 at 12:10 -0600, Seth Forshee wrote:

> Well, I think the way that will be simplest with the fewest code changes
> would be to use a tx control flag. Of course then we've gobbled up one
> of the last available flags.

Let's do that anyway then. I still think we need to do the
PS/scan/offchannel thing I described in another mail anyway, so that'd
be a better interim step than changing all the prototypes...

johannes

2013-02-06 22:20:55

by Seth Forshee

Subject: Re: [PATCH 3/4] mac80211: Improve error handling for off-channel operation

On Wed, Feb 06, 2013 at 11:10:51PM +0100, Johannes Berg wrote:
> On Wed, 2013-02-06 at 16:05 -0600, Seth Forshee wrote:
> > On Wed, Feb 06, 2013 at 10:44:37PM +0100, Johannes Berg wrote:
> > > On Wed, 2013-02-06 at 09:01 -0600, Seth Forshee wrote:
> > > > Errors in sending the nullfunc frame to set powersave at the AP for
> > > > off-channel operation can lead to high packet loss. Add error handling
> > > > to fail going off-channel when this happens, and return an error to
> > > > userspace.
> > >
> > > With the flushes in place, have you ever seen this fail? This and patch
> > > ones seems like a lot of churn for only half of what you'd want -- what
> > > you really want is to know if the AP ACKed the frame...
> >
> > That's a good point. I've seen iw fail to initiate scans, but I can't
> > say whether or not any of them was due to the queues being stopped for
> > some reason. When I was testing NetworkManager was still managing the
> > interface, so at least some failures were undobtedly because another
> > scan was ongoing.
>
> Yeah you'd expect that. I think you could tell the difference -- EBUSY
> vs. whatever other error code you chose?

Sure. I _can_ test for that, just saying that I haven't.

> > I'd considered trying to expand this to check whether or not the frame
> > was acked -- in fact just today I captured a trace where the AP didn't
> > ack the frame but the STA went off-channel anyway. I'm not sure how to
> > implement that yet, and haven't found time to look into it.
>
> It means waiting for the TX status from the driver, which might not
> really come with all drivers at all, making it somewhat tricky in
> general.
>
> Anyway my point is that this doesn't really help all that much, and
> patch 1 and 3 together is a lot of churn ...

Fair enough. If you want I can drop those and resend 2 and 4, because
they do fix real problems.

Seth

2013-02-08 20:53:16

by Johannes Berg

Subject: Re: [PATCH v3 1/2] mac80211: Fix tx queue handling during scans

On Fri, 2013-02-08 at 14:46 -0600, Seth Forshee wrote:

> > > Well, I think the way that will be simplest with the fewest code changes
> > > would be to use a tx control flag. Of course then we've gobbled up one
> > > of the last available flags.
> >
> > Let's do that anyway then. I still think we need to do the
> > PS/scan/offchannel thing I described in another mail anyway, so that'd
> > be a better interim step than changing all the prototypes...
>
> When I originally looked at using a tx control flag I didn't think that
> using IEEE80211_TX_CTL_TX_OFFCHAN would work, but now I'm not sure why.
> Is there any reason not to do this?

I guess you'd have to pre-assign the queue, since the code in
ieee80211_tx() might skip that part, but that seems easy enough. Other
than that, a driver that checks IEEE80211_TX_CTL_TX_OFFCHAN might treat
that frame specially, but OTOH the only drivers using it right now are
ours and TI's and they both have HW scan/roc, so your code never
executes. What a future driver might do is anyone's guess...

johannes