All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] mac80211: support non-data TXQs
@ 2017-06-20 21:03 Johannes Berg
  2017-06-20 21:09 ` Johannes Berg
  2017-06-21  4:19 ` Igor Mitsyanko
  0 siblings, 2 replies; 11+ messages in thread
From: Johannes Berg @ 2017-06-20 21:03 UTC (permalink / raw)
  To: linux-wireless; +Cc: nbd, Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

Some drivers may want to also use the TXQ abstraction with
non-data packets, so add a hardware flag to allow this.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h     |  9 +++++++--
 net/mac80211/debugfs.c     |  1 +
 net/mac80211/debugfs_sta.c |  4 +++-
 net/mac80211/sta_info.c    | 15 ++++++++++++---
 net/mac80211/tx.c          | 20 ++++++++++++++------
 5 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 13c59e6f88d4..b64148c49db0 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1908,7 +1908,7 @@ struct ieee80211_sta {
 	bool support_p2p_ps;
 	u16 max_rc_amsdu_len;
 
-	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS];
+	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS + 1];
 
 	/* must be last */
 	u8 drv_priv[0] __aligned(sizeof(void *));
@@ -1942,7 +1942,8 @@ struct ieee80211_tx_control {
  *
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  * @sta: station table entry, %NULL for per-vif queue
- * @tid: the TID for this queue (unused for per-vif queue)
+ * @tid: the TID for this queue (unused for per-vif queue),
+ *	%IEEE80211_NUM_TIDS for non-data (if enabled)
  * @ac: the AC for this queue
  * @drv_priv: driver private area, sized by hw->txq_data_size
  *
@@ -2141,6 +2142,9 @@ struct ieee80211_txq {
  *	The stack will not do fragmentation.
  *	The callback for @set_frag_threshold should be set as well.
  *
+ * @IEEE80211_HW_WANT_NONDATA_TXQ: The driver/hardware wants to have a TXQ
+ *	(&struct ieee80211_txq) for non-data frames
+ *
  * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
  */
 enum ieee80211_hw_flags {
@@ -2183,6 +2187,7 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_TX_FRAG_LIST,
 	IEEE80211_HW_REPORTS_LOW_ACK,
 	IEEE80211_HW_SUPPORTS_TX_FRAG,
+	IEEE80211_HW_WANT_NONDATA_TXQ,
 
 	/* keep last, obviously */
 	NUM_IEEE80211_HW_FLAGS
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index d40ec17829a9..d754dc9ff56f 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -791,6 +791,7 @@ static const char *hw_flag_names[] = {
 	FLAG(TX_FRAG_LIST),
 	FLAG(REPORTS_LOW_ACK),
 	FLAG(SUPPORTS_TX_FRAG),
+	FLAG(WANT_NONDATA_TXQ),
 #undef FLAG
 };
 
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 0f122a554d2e..48984f6cabd9 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -171,7 +171,9 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
 		       bufsz+buf-p,
 		       "tid ac backlog-bytes backlog-packets new-flows drops marks overlimit collisions tx-bytes tx-packets\n");
 
-	for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
+	for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
+		if (!sta->sta.txq[i])
+			continue;
 		txqi = to_txq_info(sta->sta.txq[i]);
 		p += scnprintf(p, bufsz+buf-p,
 			       "%d %d %u %u %u %u %u %u %u %u %u\n",
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 0297f544e0cc..8b71960eb2a0 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -112,7 +112,12 @@ static void __cleanup_single_sta(struct sta_info *sta)
 
 	if (sta->sta.txq[0]) {
 		for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
-			struct txq_info *txqi = to_txq_info(sta->sta.txq[i]);
+			struct txq_info *txqi;
+
+			if (!sta->sta.txq[i])
+				continue;
+
+			txqi = to_txq_info(sta->sta.txq[i]);
 
 			spin_lock_bh(&fq->lock);
 			ieee80211_txq_purge(local, txqi);
@@ -384,15 +389,19 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 		ewma_signal_init(&sta->rx_stats_avg.chain_signal[i]);
 
 	if (local->ops->wake_tx_queue) {
+		int num_queues = IEEE80211_NUM_TIDS;
 		void *txq_data;
 		int size = sizeof(struct txq_info) +
 			   ALIGN(hw->txq_data_size, sizeof(void *));
 
-		txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
+		if (ieee80211_hw_check(&local->hw, WANT_NONDATA_TXQ))
+			num_queues++;
+
+		txq_data = kcalloc(num_queues, size, gfp);
 		if (!txq_data)
 			goto free;
 
-		for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
+		for (i = 0; i < num_queues; i++) {
 			struct txq_info *txq = txq_data + i * size;
 
 			ieee80211_txq_init(sdata, sta, txq, i);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3d3e37f29bcb..d1cef5ad91ee 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -4,6 +4,7 @@
  * Copyright 2006-2007	Jiri Benc <jbenc@suse.cz>
  * Copyright 2007	Johannes Berg <johannes@sipsolutions.net>
  * Copyright 2013-2014  Intel Mobile Communications GmbH
+ * Copyright 2017	Intel Deutschland GmbH
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -1245,24 +1246,28 @@ static struct txq_info *ieee80211_get_txq(struct ieee80211_local *local,
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-	struct ieee80211_txq *txq = NULL;
+	struct ieee80211_txq *txq;
 
 	if ((info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) ||
 	    (info->control.flags & IEEE80211_TX_CTRL_PS_RESPONSE))
 		return NULL;
 
-	if (!ieee80211_is_data(hdr->frame_control))
+	if (!vif) {
 		return NULL;
+	} else if (!sta) {
+		txq = vif->txq;
+	} else if (!ieee80211_is_data(hdr->frame_control)) {
+		if (!sta->uploaded)
+			return NULL;
 
-	if (sta) {
+		txq = sta->sta.txq[IEEE80211_NUM_TIDS];
+	} else {
 		u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
 
 		if (!sta->uploaded)
 			return NULL;
 
 		txq = sta->sta.txq[tid];
-	} else if (vif) {
-		txq = vif->txq;
 	}
 
 	if (!txq)
@@ -1417,7 +1422,10 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
 		txqi->txq.sta = &sta->sta;
 		sta->sta.txq[tid] = &txqi->txq;
 		txqi->txq.tid = tid;
-		txqi->txq.ac = ieee80211_ac_from_tid(tid);
+		if (tid == IEEE80211_NUM_TIDS + 1)
+			txqi->txq.ac = IEEE80211_AC_VO;
+		else
+			txqi->txq.ac = ieee80211_ac_from_tid(tid);
 	} else {
 		sdata->vif.txq = &txqi->txq;
 		txqi->txq.tid = 0;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-20 21:03 [RFC] mac80211: support non-data TXQs Johannes Berg
@ 2017-06-20 21:09 ` Johannes Berg
  2017-06-21  4:19 ` Igor Mitsyanko
  1 sibling, 0 replies; 11+ messages in thread
From: Johannes Berg @ 2017-06-20 21:09 UTC (permalink / raw)
  To: linux-wireless; +Cc: nbd

On Tue, 2017-06-20 at 23:03 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Some drivers may want to also use the TXQ abstraction with
> non-data packets, so add a hardware flag to allow this.

Need to replace various ARRAY_SIZE(sta->txq) in ath9k/ath10k by
IEEE80211_NUM_TIDS.

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-20 21:03 [RFC] mac80211: support non-data TXQs Johannes Berg
  2017-06-20 21:09 ` Johannes Berg
@ 2017-06-21  4:19 ` Igor Mitsyanko
  2017-06-21  7:09   ` Johannes Berg
  1 sibling, 1 reply; 11+ messages in thread
From: Igor Mitsyanko @ 2017-06-21  4:19 UTC (permalink / raw)
  To: johannes, linux-wireless

On 06/20/2017 02:03 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Some drivers may want to also use the TXQ abstraction with
> non-data packets, so add a hardware flag to allow this.
> 
> Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>   include/net/mac80211.h     |  9 +++++++--
>   net/mac80211/debugfs.c     |  1 +
>   net/mac80211/debugfs_sta.c |  4 +++-
>   net/mac80211/sta_info.c    | 15 ++++++++++++---
>   net/mac80211/tx.c          | 20 ++++++++++++++------
>   5 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 13c59e6f88d4..b64148c49db0 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1908,7 +1908,7 @@ struct ieee80211_sta {
>   	bool support_p2p_ps;
>   	u16 max_rc_amsdu_len;
>   
> -	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS];
> +	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS + 1];

Isn't that a little confusing? Wouldn't it be better to have a separate 
member for non-data txq and name it accordingly (something like 
txq_nodata). You have to handle it specially in most cases anyway I guess.
With this approach you won't have to replace ARRAY_SIZE(sta->txq) by 
IEEE80211_NUM_TIDS anywhere.

>   
>   	/* must be last */
>   	u8 drv_priv[0] __aligned(sizeof(void *));
> @@ -1942,7 +1942,8 @@ struct ieee80211_tx_control {
>    *
>    * @vif: &struct ieee80211_vif pointer from the add_interface callback.
>    * @sta: station table entry, %NULL for per-vif queue
> - * @tid: the TID for this queue (unused for per-vif queue)
> + * @tid: the TID for this queue (unused for per-vif queue),
> + *	%IEEE80211_NUM_TIDS for non-data (if enabled)
>    * @ac: the AC for this queue
>    * @drv_priv: driver private area, sized by hw->txq_data_size
>    *
> @@ -2141,6 +2142,9 @@ struct ieee80211_txq {
>    *	The stack will not do fragmentation.
>    *	The callback for @set_frag_threshold should be set as well.
>    *
> + * @IEEE80211_HW_WANT_NONDATA_TXQ: The driver/hardware wants to have a TXQ
> + *	(&struct ieee80211_txq) for non-data frames
> + *
>    * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
>    */
>   enum ieee80211_hw_flags {
> @@ -2183,6 +2187,7 @@ enum ieee80211_hw_flags {
>   	IEEE80211_HW_TX_FRAG_LIST,
>   	IEEE80211_HW_REPORTS_LOW_ACK,
>   	IEEE80211_HW_SUPPORTS_TX_FRAG,
> +	IEEE80211_HW_WANT_NONDATA_TXQ,
>   
>   	/* keep last, obviously */
>   	NUM_IEEE80211_HW_FLAGS
> diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> index d40ec17829a9..d754dc9ff56f 100644
> --- a/net/mac80211/debugfs.c
> +++ b/net/mac80211/debugfs.c
> @@ -791,6 +791,7 @@ static const char *hw_flag_names[] = {
>   	FLAG(TX_FRAG_LIST),
>   	FLAG(REPORTS_LOW_ACK),
>   	FLAG(SUPPORTS_TX_FRAG),
> +	FLAG(WANT_NONDATA_TXQ),
>   #undef FLAG
>   };
>   
> diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
> index 0f122a554d2e..48984f6cabd9 100644
> --- a/net/mac80211/debugfs_sta.c
> +++ b/net/mac80211/debugfs_sta.c
> @@ -171,7 +171,9 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
>   		       bufsz+buf-p,
>   		       "tid ac backlog-bytes backlog-packets new-flows drops marks overlimit collisions tx-bytes tx-packets\n");
>   
> -	for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
> +	for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> +		if (!sta->sta.txq[i])
> +			continue;
>   		txqi = to_txq_info(sta->sta.txq[i]);
>   		p += scnprintf(p, bufsz+buf-p,
>   			       "%d %d %u %u %u %u %u %u %u %u %u\n",
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 0297f544e0cc..8b71960eb2a0 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -112,7 +112,12 @@ static void __cleanup_single_sta(struct sta_info *sta)
>   
>   	if (sta->sta.txq[0]) {
>   		for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> -			struct txq_info *txqi = to_txq_info(sta->sta.txq[i]);
> +			struct txq_info *txqi;
> +
> +			if (!sta->sta.txq[i])
> +				continue;
> +
> +			txqi = to_txq_info(sta->sta.txq[i]);
>   
>   			spin_lock_bh(&fq->lock);
>   			ieee80211_txq_purge(local, txqi);
> @@ -384,15 +389,19 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>   		ewma_signal_init(&sta->rx_stats_avg.chain_signal[i]);
>   
>   	if (local->ops->wake_tx_queue) {
> +		int num_queues = IEEE80211_NUM_TIDS;
>   		void *txq_data;
>   		int size = sizeof(struct txq_info) +
>   			   ALIGN(hw->txq_data_size, sizeof(void *));
>   
> -		txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> +		if (ieee80211_hw_check(&local->hw, WANT_NONDATA_TXQ))
> +			num_queues++;
> +
> +		txq_data = kcalloc(num_queues, size, gfp);
>   		if (!txq_data)
>   			goto free;
>   
> -		for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> +		for (i = 0; i < num_queues; i++) {
>   			struct txq_info *txq = txq_data + i * size;
>   
>   			ieee80211_txq_init(sdata, sta, txq, i);
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 3d3e37f29bcb..d1cef5ad91ee 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -4,6 +4,7 @@
>    * Copyright 2006-2007	Jiri Benc <jbenc-AlSwsSmVLrQ@public.gmane.org>
>    * Copyright 2007	Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
>    * Copyright 2013-2014  Intel Mobile Communications GmbH
> + * Copyright 2017	Intel Deutschland GmbH
>    *
>    * This program is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License version 2 as
> @@ -1245,24 +1246,28 @@ static struct txq_info *ieee80211_get_txq(struct ieee80211_local *local,
>   {
>   	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
>   	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> -	struct ieee80211_txq *txq = NULL;
> +	struct ieee80211_txq *txq;
>   
>   	if ((info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) ||
>   	    (info->control.flags & IEEE80211_TX_CTRL_PS_RESPONSE))
>   		return NULL;
>   
> -	if (!ieee80211_is_data(hdr->frame_control))
> +	if (!vif) {
>   		return NULL;
> +	} else if (!sta) {
> +		txq = vif->txq;
> +	} else if (!ieee80211_is_data(hdr->frame_control)) {
> +		if (!sta->uploaded)
> +			return NULL;
>   
> -	if (sta) {
> +		txq = sta->sta.txq[IEEE80211_NUM_TIDS];
> +	} else {
>   		u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
>   
>   		if (!sta->uploaded)
>   			return NULL;
>   
>   		txq = sta->sta.txq[tid];
> -	} else if (vif) {
> -		txq = vif->txq;
>   	}
>   
>   	if (!txq)
> @@ -1417,7 +1422,10 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
>   		txqi->txq.sta = &sta->sta;
>   		sta->sta.txq[tid] = &txqi->txq;
>   		txqi->txq.tid = tid;
> -		txqi->txq.ac = ieee80211_ac_from_tid(tid);
> +		if (tid == IEEE80211_NUM_TIDS + 1)
> +			txqi->txq.ac = IEEE80211_AC_VO;

Why voice, maybe commit message should mention it?

> +		else
> +			txqi->txq.ac = ieee80211_ac_from_tid(tid);
>   	} else {
>   		sdata->vif.txq = &txqi->txq;
>   		txqi->txq.tid = 0;
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  4:19 ` Igor Mitsyanko
@ 2017-06-21  7:09   ` Johannes Berg
  2017-06-21  7:38     ` Igor Mitsyanko
  2017-06-21  8:37     ` Johannes Berg
  0 siblings, 2 replies; 11+ messages in thread
From: Johannes Berg @ 2017-06-21  7:09 UTC (permalink / raw)
  To: Igor Mitsyanko, linux-wireless

On Tue, 2017-06-20 at 21:19 -0700, Igor Mitsyanko wrote:

> > -	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS];
> > +	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS + 1];
> 
> Isn't that a little confusing? Wouldn't it be better to have a
> separate  member for non-data txq and name it accordingly (something
> like  txq_nodata). 

We do this trick in quite a number of places, so it shouldn't really
come as a surprise.

> You have to handle it specially in most cases anyway I guess.
> With this approach you won't have to replace ARRAY_SIZE(sta->txq) by 
> IEEE80211_NUM_TIDS anywhere.

Yeah, that would indeed be a good reason - I didn't realize the
ARRAY_SIZE() when I originally wrote the patch. And yes, I do need to
treat it specially - except then if it's separate I also have to
initialize it separately, so it's a bit of a trade-off.

> [snip]
[please trim the amount of text you quote]

> > -		txqi->txq.ac = ieee80211_ac_from_tid(tid);
> > +		if (tid == IEEE80211_NUM_TIDS + 1)
> > +			txqi->txq.ac = IEEE80211_AC_VO;
> 
> Why voice, maybe commit message should mention it?

That's standard for management frames, I really didn't think that'd
need any comment?

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  7:09   ` Johannes Berg
@ 2017-06-21  7:38     ` Igor Mitsyanko
  2017-06-21  8:36       ` Johannes Berg
  2017-06-21  8:37     ` Johannes Berg
  1 sibling, 1 reply; 11+ messages in thread
From: Igor Mitsyanko @ 2017-06-21  7:38 UTC (permalink / raw)
  To: Johannes Berg, linux-wireless

On 06/21/2017 12:09 AM, Johannes Berg wrote:
> 
>> [snip]
> [please trim the amount of text you quote]
> 
>>> -           txqi->txq.ac = ieee80211_ac_from_tid(tid);
>>> +           if (tid == IEEE80211_NUM_TIDS + 1)
>>> +                   txqi->txq.ac = IEEE80211_AC_VO;
>>
>> Why voice, maybe commit message should mention it?
> 
> That's standard for management frames, I really didn't think that'd
> need any comment?

You're right, for some reason I thought about "null data" frames instead 
of "no data" frames.

> 
> johannes
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  7:38     ` Igor Mitsyanko
@ 2017-06-21  8:36       ` Johannes Berg
  0 siblings, 0 replies; 11+ messages in thread
From: Johannes Berg @ 2017-06-21  8:36 UTC (permalink / raw)
  To: Igor Mitsyanko, linux-wireless

On Wed, 2017-06-21 at 00:38 -0700, Igor Mitsyanko wrote:
> 
> > That's standard for management frames, I really didn't think that'd
> > need any comment?
> 
> You're right, for some reason I thought about "null data" frames
> instead of "no data" frames.

Well, the next patch actually does put those there as well :)

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  7:09   ` Johannes Berg
  2017-06-21  7:38     ` Igor Mitsyanko
@ 2017-06-21  8:37     ` Johannes Berg
  2017-06-21  8:48       ` Arend van Spriel
  1 sibling, 1 reply; 11+ messages in thread
From: Johannes Berg @ 2017-06-21  8:37 UTC (permalink / raw)
  To: Igor Mitsyanko, linux-wireless

On Wed, 2017-06-21 at 09:09 +0200, Johannes Berg wrote:
> On Tue, 2017-06-20 at 21:19 -0700, Igor Mitsyanko wrote:
> 
> > > -	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS];
> > > +	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS + 1];
> > 
> > Isn't that a little confusing? Wouldn't it be better to have a
> > separate  member for non-data txq and name it accordingly
> > (something
> > like  txq_nodata). 
> 
> We do this trick in quite a number of places, so it shouldn't really
> come as a surprise.
> 
> > You have to handle it specially in most cases anyway I guess.
> > With this approach you won't have to replace ARRAY_SIZE(sta->txq)
> > by IEEE80211_NUM_TIDS anywhere.
> 
> Yeah, that would indeed be a good reason - I didn't realize the
> ARRAY_SIZE() when I originally wrote the patch. And yes, I do need to
> treat it specially - except then if it's separate I also have to
> initialize it separately, so it's a bit of a trade-off.

I changed my mind on changing this, the allocation, initialization and
teardown etc. just gets much more complicated with doing that, needing
special cases in quite a number of places.

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  8:37     ` Johannes Berg
@ 2017-06-21  8:48       ` Arend van Spriel
  2017-06-21  9:14         ` Johannes Berg
  0 siblings, 1 reply; 11+ messages in thread
From: Arend van Spriel @ 2017-06-21  8:48 UTC (permalink / raw)
  To: Johannes Berg, Igor Mitsyanko, linux-wireless

On 6/21/2017 10:37 AM, Johannes Berg wrote:
> On Wed, 2017-06-21 at 09:09 +0200, Johannes Berg wrote:
>> On Tue, 2017-06-20 at 21:19 -0700, Igor Mitsyanko wrote:
>>
>>>> -	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS];
>>>> +	struct ieee80211_txq *txq[IEEE80211_NUM_TIDS + 1];
>>>
>>> Isn't that a little confusing? Wouldn't it be better to have a
>>> separate  member for non-data txq and name it accordingly
>>> (something
>>> like  txq_nodata).
>>
>> We do this trick in quite a number of places, so it shouldn't really
>> come as a surprise.
>>
>>> You have to handle it specially in most cases anyway I guess.
>>> With this approach you won't have to replace ARRAY_SIZE(sta->txq)
>>> by IEEE80211_NUM_TIDS anywhere.
>>
>> Yeah, that would indeed be a good reason - I didn't realize the
>> ARRAY_SIZE() when I originally wrote the patch. And yes, I do need to
>> treat it specially - except then if it's separate I also have to
>> initialize it separately, so it's a bit of a trade-off.
> 
> I changed my mind on changing this, the allocation, initialization and
> teardown etc. just gets much more complicated with doing that, needing
> special cases in quite a number of places.

So for the sake of ath{9,10}k and possible future drivers you may want 
to document it clearly in mac80211.h(?), ie. when driver does not want a 
separate non-data txq they should use IEEE80211_NUM_TIDS.

Regards,
Arend

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  8:48       ` Arend van Spriel
@ 2017-06-21  9:14         ` Johannes Berg
  2017-06-21  9:24           ` Johannes Berg
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Berg @ 2017-06-21  9:14 UTC (permalink / raw)
  To: Arend van Spriel, Igor Mitsyanko, linux-wireless

On Wed, 2017-06-21 at 10:48 +0200, Arend van Spriel wrote:
> 
> So for the sake of ath{9,10}k and possible future drivers you may
> want  to document it clearly in mac80211.h(?), ie. when driver does
> not want a  separate non-data txq they should use IEEE80211_NUM_TIDS.

Yeah, will do.

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  9:14         ` Johannes Berg
@ 2017-06-21  9:24           ` Johannes Berg
  2017-06-21  9:45             ` Johannes Berg
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Berg @ 2017-06-21  9:24 UTC (permalink / raw)
  To: Arend van Spriel, Igor Mitsyanko, linux-wireless

On Wed, 2017-06-21 at 11:14 +0200, Johannes Berg wrote:
> On Wed, 2017-06-21 at 10:48 +0200, Arend van Spriel wrote:
> > 
> > So for the sake of ath{9,10}k and possible future drivers you may
> > want  to document it clearly in mac80211.h(?), ie. when driver does
> > not want a  separate non-data txq they should use
> > IEEE80211_NUM_TIDS.
> 
> Yeah, will do.

Nah, know what? I'll just make that a compiler error.

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] mac80211: support non-data TXQs
  2017-06-21  9:24           ` Johannes Berg
@ 2017-06-21  9:45             ` Johannes Berg
  0 siblings, 0 replies; 11+ messages in thread
From: Johannes Berg @ 2017-06-21  9:45 UTC (permalink / raw)
  To: Arend van Spriel, Igor Mitsyanko, linux-wireless

On Wed, 2017-06-21 at 11:24 +0200, Johannes Berg wrote:
> On Wed, 2017-06-21 at 11:14 +0200, Johannes Berg wrote:
> > On Wed, 2017-06-21 at 10:48 +0200, Arend van Spriel wrote:
> > > 
> > > So for the sake of ath{9,10}k and possible future drivers you may
> > > want  to document it clearly in mac80211.h(?), ie. when driver
> > > does
> > > not want a  separate non-data txq they should use
> > > IEEE80211_NUM_TIDS.
> > 
> > Yeah, will do.
> 
> Nah, know what? I'll just make that a compiler error.

Hah, no, that doesn't work ... oh well.

johannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-06-21  9:45 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-20 21:03 [RFC] mac80211: support non-data TXQs Johannes Berg
2017-06-20 21:09 ` Johannes Berg
2017-06-21  4:19 ` Igor Mitsyanko
2017-06-21  7:09   ` Johannes Berg
2017-06-21  7:38     ` Igor Mitsyanko
2017-06-21  8:36       ` Johannes Berg
2017-06-21  8:37     ` Johannes Berg
2017-06-21  8:48       ` Arend van Spriel
2017-06-21  9:14         ` Johannes Berg
2017-06-21  9:24           ` Johannes Berg
2017-06-21  9:45             ` Johannes Berg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.