Re: [RFC v2] mac80211: add A-MSDU tx support

[Emmanuel Grumbach]

On Sun, Feb 7, 2016 at 11:08 AM, Felix Fietkau  wrote:
> On 2016-02-07 08:25, Emmanuel Grumbach wrote:
>> On Fri, Feb 5, 2016 at 12:41 PM, Felix Fietkau  wrote:
>>> Requires software tx queueing support. frag_list support (for zero-copy)
>>> is optional.
>>>
>>> Signed-off-by: Felix Fietkau 
>>
>> Looks nice!
>> This would allow us to create aggregates of TCP Acks, the problem is
>> that when you are mostly receiving data, the hardware queues are
>> pretty much empty (nothing besides the TCP Acks which should go out
>> quickly) so that packets don't pile up in the software queues and
>> hence you don't have enough material to build an A-MSDU.
>> I guess that for AP oriented devices, this is ideal solution since you
>> can't rely on TSO (packets are not locally generated) and this allows
>> to build an A-MSDU without adding more latency since you build an
>> A-MSDU with packets that are already in the queue waiting instead of
>> delaying transmission of the first packet.
>> IIRC, the latter was the approach chose by the new Marvell driver
>> posted a few weeks ago. This approach is better in my eyes.
>> For iwlwifi which is much more station oriented (of GO which is
>> basically an AP with locally generated traffic), I took the TSO
>> approach. I guess we could try to change iwlwifi to use your tx
>> queues, and check how that works. This would allow us to have A-MSDU
>> on bridged traffic as well, although this use case is much less common
>> for Intel devices.


> Can the iwlwifi firmware maintain per-sta per-tid queues? Because that
> way you would get the most benefits from using that tx queueing
> infrastructure.

Well... iwlwifi and athXk are very different. iwlwifi really has the
concept of queues and not flat descriptors.
Any Tx descriptor lives in the context of a Tx queue which is
per-sta-per tid when A-MPDUs is enabled.
We are now moving to a scheme in which we will have per-sta-per-tid
queue regardless of the A-MPDU state which will make much sense to tie
to the tx queueing infrastructure.
Thing is that in that case I am afraid we will not have enough packets
in the software tx queue to get A-MSDUs from your code. with TSO, it
is easier :) Still worth trying to work with this instead of TSO and
see how it goes. That won't happen anytime soon though.

>
>>> +
>>> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
>>> + struct sta_info *sta,
>>> + struct ieee80211_fast_tx *fast_tx,
>>> + struct sk_buff *skb)
>>> +{
>>> +   struct ieee80211_local *local = sdata->local;
>>> +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>>> +   struct ieee80211_txq *txq = sta->sta.txq[tid];
>>> +   struct txq_info *txqi;
>>> +   struct sk_buff **frag_tail, *head;
>>> +   int subframe_len = skb->len - ETH_ALEN;
>>> +   int max_amsdu_len;
>>> +   __be16 len;
>>> +   void *data;
>>> +   bool ret = false;
>>> +   int n = 1;
>>> +
>>> +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
>>> +   return false;
>>> +
>>> +   if (!txq)
>>> +   return false;
>>> +
>>> +   txqi = to_txq_info(txq);
>>> +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
>>> +   return false;
>>> +
>>> +   /*
>>> +* A-MPDU limits maximum MPDU size to 4095 bytes. Since aggregation
>>> +* sessions are started/stopped without txq flush, use the limit 
>>> here
>>> +* to avoid having to de-aggregate later.
>>> +*/
>>> +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
>>
>> So you can't get 10K A-MSDUs? I don't see where you check that you
>> have an A-MPDU session here. You seem to be applying the 4095 limit
>> also for streams that are not an A-MPDU?
>> I guess you could check if the sta is a VHT peer, in that case, no
>> limit applies.
> The explanation for the missing A-MPDU change is in that comment -
> checking for an active A-MPDU session would make it unnecessarily complex.
> Good point about checking for VHT capabilities to remove this limit, I
> will add that.
>
> - Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2] mac80211: add A-MSDU tx support

[Felix Fietkau]

On 2016-02-07 08:25, Emmanuel Grumbach wrote:
> On Fri, Feb 5, 2016 at 12:41 PM, Felix Fietkau  wrote:
>> Requires software tx queueing support. frag_list support (for zero-copy)
>> is optional.
>>
>> Signed-off-by: Felix Fietkau 
> 
> Looks nice!
> This would allow us to create aggregates of TCP Acks, the problem is
> that when you are mostly receiving data, the hardware queues are
> pretty much empty (nothing besides the TCP Acks which should go out
> quickly) so that packets don't pile up in the software queues and
> hence you don't have enough material to build an A-MSDU.
> I guess that for AP oriented devices, this is ideal solution since you
> can't rely on TSO (packets are not locally generated) and this allows
> to build an A-MSDU without adding more latency since you build an
> A-MSDU with packets that are already in the queue waiting instead of
> delaying transmission of the first packet.
> IIRC, the latter was the approach chose by the new Marvell driver
> posted a few weeks ago. This approach is better in my eyes.
> For iwlwifi which is much more station oriented (of GO which is
> basically an AP with locally generated traffic), I took the TSO
> approach. I guess we could try to change iwlwifi to use your tx
> queues, and check how that works. This would allow us to have A-MSDU
> on bridged traffic as well, although this use case is much less common
> for Intel devices.
Can the iwlwifi firmware maintain per-sta per-tid queues? Because that
way you would get the most benefits from using that tx queueing
infrastructure.

>> +
>> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
>> + struct sta_info *sta,
>> + struct ieee80211_fast_tx *fast_tx,
>> + struct sk_buff *skb)
>> +{
>> +   struct ieee80211_local *local = sdata->local;
>> +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>> +   struct ieee80211_txq *txq = sta->sta.txq[tid];
>> +   struct txq_info *txqi;
>> +   struct sk_buff **frag_tail, *head;
>> +   int subframe_len = skb->len - ETH_ALEN;
>> +   int max_amsdu_len;
>> +   __be16 len;
>> +   void *data;
>> +   bool ret = false;
>> +   int n = 1;
>> +
>> +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
>> +   return false;
>> +
>> +   if (!txq)
>> +   return false;
>> +
>> +   txqi = to_txq_info(txq);
>> +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
>> +   return false;
>> +
>> +   /*
>> +* A-MPDU limits maximum MPDU size to 4095 bytes. Since aggregation
>> +* sessions are started/stopped without txq flush, use the limit here
>> +* to avoid having to de-aggregate later.
>> +*/
>> +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
> 
> So you can't get 10K A-MSDUs? I don't see where you check that you
> have an A-MPDU session here. You seem to be applying the 4095 limit
> also for streams that are not an A-MPDU?
> I guess you could check if the sta is a VHT peer, in that case, no
> limit applies.
The explanation for the missing A-MPDU change is in that comment -
checking for an active A-MPDU session would make it unnecessarily complex.
Good point about checking for VHT capabilities to remove this limit, I
will add that.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2] mac80211: add A-MSDU tx support

[Felix Fietkau]

On 2016-02-07 12:32, Emmanuel Grumbach wrote:
>>> +
>>> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data 
>>> *sdata,
>>> + struct sta_info *sta,
>>> + struct ieee80211_fast_tx *fast_tx,
>>> + struct sk_buff *skb)
>>> +{
>>> +   struct ieee80211_local *local = sdata->local;
>>> +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>>> +   struct ieee80211_txq *txq = sta->sta.txq[tid];
>>> +   struct txq_info *txqi;
>>> +   struct sk_buff **frag_tail, *head;
>>> +   int subframe_len = skb->len - ETH_ALEN;
>>> +   int max_amsdu_len;
>>> +   __be16 len;
>>> +   void *data;
>>> +   bool ret = false;
>>> +   int n = 1;
>>> +
>>> +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
>>> +   return false;
>>> +
>>> +   if (!txq)
>>> +   return false;
>>> +
>>> +   txqi = to_txq_info(txq);
>>> +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
>>> +   return false;
>>> +
>>> +   /*
>>> +* A-MPDU limits maximum MPDU size to 4095 bytes. Since 
>>> aggregation
>>> +* sessions are started/stopped without txq flush, use the 
>>> limit here
>>> +* to avoid having to de-aggregate later.
>>> +*/
>>> +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
>>
>> So you can't get 10K A-MSDUs? I don't see where you check that you
>> have an A-MPDU session here. You seem to be applying the 4095 limit
>> also for streams that are not an A-MPDU?
>> I guess you could check if the sta is a VHT peer, in that case, no
>> limit applies.
> The explanation for the missing A-MPDU change is in that comment -
> checking for an active A-MPDU session would make it unnecessarily complex.
> Good point about checking for VHT capabilities to remove this limit, I
> will add that.
>>>
>>> Yes - I read the comment, but it seemed very sub-optimal to limit all
>>> the A-MSDUs to 4K. With TSO I can get up to 10K and it really helps
>>> TPT.
>> This was built with the assumption that most scenarios use A-MPDU anyway
>> and thus don't need really large A-MSDUs.
> 
> Yes - so that's interesting. We can chose to have long A-MSDUs inside
> a short (in terms of number of MPDUs) A-MPDU, of with shorter A-MSDU
> and squeeze more of these into a single A-MDPU.
> The first intuition says that we'd better have more MPDUs because of
> the CRC check for each MPDU which doesn't exist in A-MSDU. OTOH, I
> remember I could clearly see that I get a higher TPT with longer
> A-MSDUs. Maybe I wasn't looking right at the size of the A-MPDU? I
> guess I'd need to go back to the table with all the values we had, but
> since we pretty much got what we wanted, I am not sure I will able to
> find time for this :)
I think it also depends on the environment. I'd guess that under very
ideal conditions with very few retransmissions, really long A-MSDU might
have some performance benefits, but I don't think that'll hold if the
conditions are less than ideal and you have rate fluctuation and
retransmissions.

>>> One more point. In VHT, there may be a limit on the numbers of
>>> subframes in the A-MSDU. I don't see you handle that. Maybe I missed
>>> it?
>> I haven't looked at that much yet. Right now the driver can only specify
>> a limit for the number of subframes.
> 
> I am talking about a limitation the peer can advertise. Check this out:
> https://git.kernel.org/cgit/linux/kernel/git/jberg/mac80211-next.git/tree/net/mac80211/cfg.c#n1134
> 
> I couldn't see the limit the driver can specify in your code. I may
> very well have missed it.
I missed that one. Will add it in the next patch.

>>> This is an order 4 allocation, for each A-MSDU. Note
>>> that iwlwifi (and probably other drivers) can handle gather DMA in Tx,
>>> but they have a limited number of frags they can handle. iwlwifi e.g.
>>> can handle up to 20 frags, but 3 are taken for "paperwork". You'll
>>> have 2 frags per subframe at least (assuming that each subframe's
>>> payload is nicely contiguous and not on a page boundary). I think that
>>> it may be worthwhile to ask the driver how many frags it is supposed
>>> to handle. I can't promise iwlwifi will use it, but I guess it will be
>>> useful for someone.
>> You mean an extra frag limit in addition to the driver subframe limit,
>> in case individual subframes are fragmented as well?
>>
> 
> well.. Yes, you can't assume that you'll have one descriptor for one
> MSDU payload  (unless the driver doesn't advertise SG to the
> netstack).
Okay, please make a suggestion describing the exact kinds of limits you
would need for iwlwifi.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the 

[RFC v3] mac80211: add A-MSDU tx support

[Felix Fietkau]

Requires software tx queueing support. frag_list support (for zero-copy)
is optional.

Signed-off-by: Felix Fietkau 
---
 include/net/mac80211.h |  17 +
 net/mac80211/agg-tx.c  |   5 ++
 net/mac80211/debugfs.c |   2 +
 net/mac80211/ieee80211_i.h |   1 +
 net/mac80211/tx.c  | 168 +
 5 files changed, 193 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5714774..31dca81 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -709,6 +709,7 @@ enum mac80211_tx_info_flags {
  * @IEEE80211_TX_CTRL_PS_RESPONSE: This frame is a response to a poll
  * frame (PS-Poll or uAPSD).
  * @IEEE80211_TX_CTRL_RATE_INJECT: This frame is injected with rate information
+ * @IEEE80211_TX_CTRL_AMSDU: This frame is an A-MSDU frame
  *
  * These flags are used in tx_info->control.flags.
  */
@@ -716,6 +717,7 @@ enum mac80211_tx_control_flags {
IEEE80211_TX_CTRL_PORT_CTRL_PROTO   = BIT(0),
IEEE80211_TX_CTRL_PS_RESPONSE   = BIT(1),
IEEE80211_TX_CTRL_RATE_INJECT   = BIT(2),
+   IEEE80211_TX_CTRL_AMSDU = BIT(3),
 };
 
 /*
@@ -1964,6 +1966,12 @@ struct ieee80211_txq {
  * order and does not need to manage its own reorder buffer or BA session
  * timeout.
  *
+ * @IEEE80211_HW_TX_AMSDU: Hardware (or driver) supports software aggregated
+ * A-MSDU frames. Requires software tx queueing support.
+ *
+ * @IEEE80211_HW_TX_FRAG_LIST: Hardware (or driver) supports sending frag_list
+ * skbs, needed for zero-copy software A-MSDU.
+ *
  * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
  */
 enum ieee80211_hw_flags {
@@ -2001,6 +2009,8 @@ enum ieee80211_hw_flags {
IEEE80211_HW_BEACON_TX_STATUS,
IEEE80211_HW_NEEDS_UNIQUE_STA_ADDR,
IEEE80211_HW_SUPPORTS_REORDERING_BUFFER,
+   IEEE80211_HW_TX_AMSDU,
+   IEEE80211_HW_TX_FRAG_LIST,
 
/* keep last, obviously */
NUM_IEEE80211_HW_FLAGS
@@ -2073,6 +2083,11 @@ enum ieee80211_hw_flags {
  * size is smaller (an example is LinkSys WRT120N with FW v1.0.07
  * build 002 Jun 18 2012).
  *
+ * @max_tx_amsdu_subframes: maximum number of subframes used in software
+ * A-MSDU aggregation
+ *
+ * @max_tx_fragments: maximum fragments per (A-)MSDU.
+ *
  * @offchannel_tx_hw_queue: HW queue ID to use for offchannel TX
  * (if %IEEE80211_HW_QUEUE_CONTROL is set)
  *
@@ -2127,6 +2142,8 @@ struct ieee80211_hw {
u8 max_rate_tries;
u8 max_rx_aggregation_subframes;
u8 max_tx_aggregation_subframes;
+   u8 max_tx_amsdu_subframes;
+   u8 max_tx_fragments;
u8 offchannel_tx_hw_queue;
u8 radiotap_mcs_details;
u16 radiotap_vht_details;
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 4932e9f..42fa810 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -935,6 +935,7 @@ void ieee80211_process_addba_resp(struct ieee80211_local 
*local,
  size_t len)
 {
struct tid_ampdu_tx *tid_tx;
+   struct ieee80211_txq *txq;
u16 capab, tid;
u8 buf_size;
bool amsdu;
@@ -945,6 +946,10 @@ void ieee80211_process_addba_resp(struct ieee80211_local 
*local,
buf_size = (capab & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK) >> 6;
buf_size = min(buf_size, local->hw.max_tx_aggregation_subframes);
 
+   txq = sta->sta.txq[tid];
+   if (!amsdu && txq)
+   set_bit(IEEE80211_TXQ_NO_AMSDU, _txq_info(txq)->flags);
+
mutex_lock(>ampdu_mlme.mtx);
 
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index e433d0c..847779d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -127,6 +127,8 @@ static const char *hw_flag_names[NUM_IEEE80211_HW_FLAGS + 
1] = {
FLAG(BEACON_TX_STATUS),
FLAG(NEEDS_UNIQUE_STA_ADDR),
FLAG(SUPPORTS_REORDERING_BUFFER),
+   FLAG(TX_AMSDU),
+   FLAG(TX_FRAG_LIST),
 
/* keep last for the build bug below */
(void *)0x1
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a49c103..e68d8db 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -799,6 +799,7 @@ struct mac80211_qos_map {
 enum txq_info_flags {
IEEE80211_TXQ_STOP,
IEEE80211_TXQ_AMPDU,
+   IEEE80211_TXQ_NO_AMSDU,
 };
 
 struct txq_info {
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index a5aa275..f37f729 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1324,6 +1324,10 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw 
*hw,
 out:
spin_unlock_bh(>queue.lock);
 
+   if (skb && skb_has_frag_list(skb) &&
+   !ieee80211_hw_check(>hw, TX_FRAG_LIST))
+   skb_linearize(skb);
+
return skb;
 }
 EXPORT_SYMBOL(ieee80211_tx_dequeue);
@@ -2763,6 +2767,166 @@ void 

Re: [RFC v2] mac80211: add A-MSDU tx support

[Felix Fietkau]

On 2016-02-07 12:56, Emmanuel Grumbach wrote:
>>> well.. Yes, you can't assume that you'll have one descriptor for one
>>> MSDU payload  (unless the driver doesn't advertise SG to the
>>> netstack).
>> Okay, please make a suggestion describing the exact kinds of limits you
>> would need for iwlwifi.
> 
> Are athX devices able to handle MPDUs with any number of frags? Say if
> you have 30 different physically contiguous fragments, the DMA would
> be able to load all these into one single packet and send it to the
> air?
I think athX devices have no limitations there. I'm not testing this
with atheros devices though - ath9k does not have mac80211
per-sta-per-tid queueing support yet. I'm working with MediaTek MT76x2
chipsets with my mt76 driver, which I will upstream soon.

> iwlwifi currently has the limitation of 20 Transmit Buffers (BTs)
> which I mentioned earlier. I guess it'd be nice if the driver would be
> able to advertise how many fragments it can handle. Then, you'd need
> to stop the A-MSDU building if you'd cross this boundary?
> 
> You can look at skb_shinfo(skb)->nr_frags to know how many frags you
> have for each skb. On top of that, you need 1 frag for each subframe
> (subframe header).
I implemented all of your suggestions in RFC v3, let me know if
anything's missing.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2] mac80211: add A-MSDU tx support

[Emmanuel Grumbach]

On Sun, Feb 7, 2016 at 12:06 PM, Emmanuel Grumbach  wrote:
> On Sun, Feb 7, 2016 at 11:08 AM, Felix Fietkau  wrote:
>> On 2016-02-07 08:25, Emmanuel Grumbach wrote:
>>> On Fri, Feb 5, 2016 at 12:41 PM, Felix Fietkau  wrote:
 Requires software tx queueing support. frag_list support (for zero-copy)
 is optional.

 Signed-off-by: Felix Fietkau 
>>>
>>> Looks nice!
>>> This would allow us to create aggregates of TCP Acks, the problem is
>>> that when you are mostly receiving data, the hardware queues are
>>> pretty much empty (nothing besides the TCP Acks which should go out
>>> quickly) so that packets don't pile up in the software queues and
>>> hence you don't have enough material to build an A-MSDU.
>>> I guess that for AP oriented devices, this is ideal solution since you
>>> can't rely on TSO (packets are not locally generated) and this allows
>>> to build an A-MSDU without adding more latency since you build an
>>> A-MSDU with packets that are already in the queue waiting instead of
>>> delaying transmission of the first packet.
>>> IIRC, the latter was the approach chose by the new Marvell driver
>>> posted a few weeks ago. This approach is better in my eyes.
>>> For iwlwifi which is much more station oriented (of GO which is
>>> basically an AP with locally generated traffic), I took the TSO
>>> approach. I guess we could try to change iwlwifi to use your tx
>>> queues, and check how that works. This would allow us to have A-MSDU
>>> on bridged traffic as well, although this use case is much less common
>>> for Intel devices.
>
>
>> Can the iwlwifi firmware maintain per-sta per-tid queues? Because that
>> way you would get the most benefits from using that tx queueing
>> infrastructure.
>
> Well... iwlwifi and athXk are very different. iwlwifi really has the
> concept of queues and not flat descriptors.
> Any Tx descriptor lives in the context of a Tx queue which is
> per-sta-per tid when A-MPDUs is enabled.
> We are now moving to a scheme in which we will have per-sta-per-tid
> queue regardless of the A-MPDU state which will make much sense to tie
> to the tx queueing infrastructure.
> Thing is that in that case I am afraid we will not have enough packets
> in the software tx queue to get A-MSDUs from your code. with TSO, it
> is easier :) Still worth trying to work with this instead of TSO and
> see how it goes. That won't happen anytime soon though.
>
>>
 +
 +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 + struct sta_info *sta,
 + struct ieee80211_fast_tx *fast_tx,
 + struct sk_buff *skb)
 +{
 +   struct ieee80211_local *local = sdata->local;
 +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
 +   struct ieee80211_txq *txq = sta->sta.txq[tid];
 +   struct txq_info *txqi;
 +   struct sk_buff **frag_tail, *head;
 +   int subframe_len = skb->len - ETH_ALEN;
 +   int max_amsdu_len;
 +   __be16 len;
 +   void *data;
 +   bool ret = false;
 +   int n = 1;
 +
 +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
 +   return false;
 +
 +   if (!txq)
 +   return false;
 +
 +   txqi = to_txq_info(txq);
 +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
 +   return false;
 +
 +   /*
 +* A-MPDU limits maximum MPDU size to 4095 bytes. Since aggregation
 +* sessions are started/stopped without txq flush, use the limit 
 here
 +* to avoid having to de-aggregate later.
 +*/
 +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
>>>
>>> So you can't get 10K A-MSDUs? I don't see where you check that you
>>> have an A-MPDU session here. You seem to be applying the 4095 limit
>>> also for streams that are not an A-MPDU?
>>> I guess you could check if the sta is a VHT peer, in that case, no
>>> limit applies.
>> The explanation for the missing A-MPDU change is in that comment -
>> checking for an active A-MPDU session would make it unnecessarily complex.
>> Good point about checking for VHT capabilities to remove this limit, I
>> will add that.

Yes - I read the comment, but it seemed very sub-optimal to limit all
the A-MSDUs to 4K. With TSO I can get up to 10K and it really helps
TPT.
One more point. In VHT, there may be a limit on the numbers of
subframes in the A-MSDU. I don't see you handle that. Maybe I missed
it?

And... in case the driver doesn't handle frag_list, you linearize the
skb which is pretty much the only thing you can do at this stage. But,
when you'll lift the 4095 bytes limit, you'll get 11K A-MSDU,
linarizing such a long packet is really putting the memory manager
under pressure. This is an 

Re: [RFC v2] mac80211: add A-MSDU tx support

[Felix Fietkau]

On 2016-02-07 11:22, Emmanuel Grumbach wrote:
> On Sun, Feb 7, 2016 at 12:06 PM, Emmanuel Grumbach  
> wrote:
>> On Sun, Feb 7, 2016 at 11:08 AM, Felix Fietkau  wrote:
>>> On 2016-02-07 08:25, Emmanuel Grumbach wrote:
 On Fri, Feb 5, 2016 at 12:41 PM, Felix Fietkau  wrote:
> Requires software tx queueing support. frag_list support (for zero-copy)
> is optional.
>
> Signed-off-by: Felix Fietkau 

 Looks nice!
 This would allow us to create aggregates of TCP Acks, the problem is
 that when you are mostly receiving data, the hardware queues are
 pretty much empty (nothing besides the TCP Acks which should go out
 quickly) so that packets don't pile up in the software queues and
 hence you don't have enough material to build an A-MSDU.
 I guess that for AP oriented devices, this is ideal solution since you
 can't rely on TSO (packets are not locally generated) and this allows
 to build an A-MSDU without adding more latency since you build an
 A-MSDU with packets that are already in the queue waiting instead of
 delaying transmission of the first packet.
 IIRC, the latter was the approach chose by the new Marvell driver
 posted a few weeks ago. This approach is better in my eyes.
 For iwlwifi which is much more station oriented (of GO which is
 basically an AP with locally generated traffic), I took the TSO
 approach. I guess we could try to change iwlwifi to use your tx
 queues, and check how that works. This would allow us to have A-MSDU
 on bridged traffic as well, although this use case is much less common
 for Intel devices.
>>
>>
>>> Can the iwlwifi firmware maintain per-sta per-tid queues? Because that
>>> way you would get the most benefits from using that tx queueing
>>> infrastructure.
>>
>> Well... iwlwifi and athXk are very different. iwlwifi really has the
>> concept of queues and not flat descriptors.
>> Any Tx descriptor lives in the context of a Tx queue which is
>> per-sta-per tid when A-MPDUs is enabled.
>> We are now moving to a scheme in which we will have per-sta-per-tid
>> queue regardless of the A-MPDU state which will make much sense to tie
>> to the tx queueing infrastructure.
>> Thing is that in that case I am afraid we will not have enough packets
>> in the software tx queue to get A-MSDUs from your code. with TSO, it
>> is easier :) Still worth trying to work with this instead of TSO and
>> see how it goes. That won't happen anytime soon though.
>>
>>>
> +
> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data 
> *sdata,
> + struct sta_info *sta,
> + struct ieee80211_fast_tx *fast_tx,
> + struct sk_buff *skb)
> +{
> +   struct ieee80211_local *local = sdata->local;
> +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
> +   struct ieee80211_txq *txq = sta->sta.txq[tid];
> +   struct txq_info *txqi;
> +   struct sk_buff **frag_tail, *head;
> +   int subframe_len = skb->len - ETH_ALEN;
> +   int max_amsdu_len;
> +   __be16 len;
> +   void *data;
> +   bool ret = false;
> +   int n = 1;
> +
> +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
> +   return false;
> +
> +   if (!txq)
> +   return false;
> +
> +   txqi = to_txq_info(txq);
> +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
> +   return false;
> +
> +   /*
> +* A-MPDU limits maximum MPDU size to 4095 bytes. Since 
> aggregation
> +* sessions are started/stopped without txq flush, use the limit 
> here
> +* to avoid having to de-aggregate later.
> +*/
> +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);

 So you can't get 10K A-MSDUs? I don't see where you check that you
 have an A-MPDU session here. You seem to be applying the 4095 limit
 also for streams that are not an A-MPDU?
 I guess you could check if the sta is a VHT peer, in that case, no
 limit applies.
>>> The explanation for the missing A-MPDU change is in that comment -
>>> checking for an active A-MPDU session would make it unnecessarily complex.
>>> Good point about checking for VHT capabilities to remove this limit, I
>>> will add that.
> 
> Yes - I read the comment, but it seemed very sub-optimal to limit all
> the A-MSDUs to 4K. With TSO I can get up to 10K and it really helps
> TPT.
This was built with the assumption that most scenarios use A-MPDU anyway
and thus don't need really large A-MSDUs.

> One more point. In VHT, there may be a limit on the numbers of
> subframes in the A-MSDU. I don't see you handle that. Maybe I missed
> it?
I haven't 

Re: [RFC v2] mac80211: add A-MSDU tx support

[Emmanuel Grumbach]

On Sun, Feb 7, 2016 at 12:48 PM, Felix Fietkau  wrote:
> On 2016-02-07 11:22, Emmanuel Grumbach wrote:
>> On Sun, Feb 7, 2016 at 12:06 PM, Emmanuel Grumbach  
>> wrote:
>>> On Sun, Feb 7, 2016 at 11:08 AM, Felix Fietkau  wrote:
 On 2016-02-07 08:25, Emmanuel Grumbach wrote:
> On Fri, Feb 5, 2016 at 12:41 PM, Felix Fietkau  wrote:
>> Requires software tx queueing support. frag_list support (for zero-copy)
>> is optional.
>>
>> Signed-off-by: Felix Fietkau 
>
> Looks nice!
> This would allow us to create aggregates of TCP Acks, the problem is
> that when you are mostly receiving data, the hardware queues are
> pretty much empty (nothing besides the TCP Acks which should go out
> quickly) so that packets don't pile up in the software queues and
> hence you don't have enough material to build an A-MSDU.
> I guess that for AP oriented devices, this is ideal solution since you
> can't rely on TSO (packets are not locally generated) and this allows
> to build an A-MSDU without adding more latency since you build an
> A-MSDU with packets that are already in the queue waiting instead of
> delaying transmission of the first packet.
> IIRC, the latter was the approach chose by the new Marvell driver
> posted a few weeks ago. This approach is better in my eyes.
> For iwlwifi which is much more station oriented (of GO which is
> basically an AP with locally generated traffic), I took the TSO
> approach. I guess we could try to change iwlwifi to use your tx
> queues, and check how that works. This would allow us to have A-MSDU
> on bridged traffic as well, although this use case is much less common
> for Intel devices.
>>>
>>>
 Can the iwlwifi firmware maintain per-sta per-tid queues? Because that
 way you would get the most benefits from using that tx queueing
 infrastructure.
>>>
>>> Well... iwlwifi and athXk are very different. iwlwifi really has the
>>> concept of queues and not flat descriptors.
>>> Any Tx descriptor lives in the context of a Tx queue which is
>>> per-sta-per tid when A-MPDUs is enabled.
>>> We are now moving to a scheme in which we will have per-sta-per-tid
>>> queue regardless of the A-MPDU state which will make much sense to tie
>>> to the tx queueing infrastructure.
>>> Thing is that in that case I am afraid we will not have enough packets
>>> in the software tx queue to get A-MSDUs from your code. with TSO, it
>>> is easier :) Still worth trying to work with this instead of TSO and
>>> see how it goes. That won't happen anytime soon though.
>>>

>> +
>> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data 
>> *sdata,
>> + struct sta_info *sta,
>> + struct ieee80211_fast_tx *fast_tx,
>> + struct sk_buff *skb)
>> +{
>> +   struct ieee80211_local *local = sdata->local;
>> +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>> +   struct ieee80211_txq *txq = sta->sta.txq[tid];
>> +   struct txq_info *txqi;
>> +   struct sk_buff **frag_tail, *head;
>> +   int subframe_len = skb->len - ETH_ALEN;
>> +   int max_amsdu_len;
>> +   __be16 len;
>> +   void *data;
>> +   bool ret = false;
>> +   int n = 1;
>> +
>> +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
>> +   return false;
>> +
>> +   if (!txq)
>> +   return false;
>> +
>> +   txqi = to_txq_info(txq);
>> +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
>> +   return false;
>> +
>> +   /*
>> +* A-MPDU limits maximum MPDU size to 4095 bytes. Since 
>> aggregation
>> +* sessions are started/stopped without txq flush, use the limit 
>> here
>> +* to avoid having to de-aggregate later.
>> +*/
>> +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
>
> So you can't get 10K A-MSDUs? I don't see where you check that you
> have an A-MPDU session here. You seem to be applying the 4095 limit
> also for streams that are not an A-MPDU?
> I guess you could check if the sta is a VHT peer, in that case, no
> limit applies.
 The explanation for the missing A-MPDU change is in that comment -
 checking for an active A-MPDU session would make it unnecessarily complex.
 Good point about checking for VHT capabilities to remove this limit, I
 will add that.
>>
>> Yes - I read the comment, but it seemed very sub-optimal to limit all
>> the A-MSDUs to 4K. With TSO I can get up to 10K and it really helps
>> TPT.
> This was built with the assumption that most scenarios use A-MPDU anyway
> and thus don't need 

Re: [RFC RESEND] iwlwifi: pcie: transmit queue auto-sizing

[Grumbach, Emmanuel]

On 02/05/2016 12:06 AM, Dave Taht wrote:
> I am not on linux-wireless nor netdev presently, but...
>
> On Thu, Feb 4, 2016 at 12:16 PM, Emmanuel Grumbach
>  wrote:
>> As many (all?) WiFi devices, Intel WiFi devices have
>> transmit queues which have 256 transmit descriptors
>> each and each descriptor corresponds to an MPDU.
>> This means that when it is full, the queue contains
>> 256 * ~1500 bytes to be transmitted (if we don't have
>> A-MSDUs). The purpose of those queues is to have enough
>> packets to be ready for transmission so that when the device
>> gets an opportunity to transmit (TxOP), it can take as many
>> packets as the spec allows and aggregate them into one
>> A-MPDU or even several A-MPDUs if we are using bursts.
>>
>> The problem is that the packets that are in these queues are
>> already out of control of the Qdisc and can stay in those
>> queues for a fairly long time when the link condition is
>> not good. This leads to the well known bufferbloat problem.
>>
>> This patch adds a way to tune the size of the transmit queue
>> so that it won't cause excessive latency. When the link
>> condition is good, the packets will flow smoothly and the
>> transmit queue will grow quickly allowing A-MPDUs and
>> maximal throughput. When the link is not optimal, we will
>> have retransmissions, lower transmit rates or signal
>> detection (CCA) which will cause a delay in the packet
>> transmission. The driver will sense this higher latency
>> and will reduce the size of the transmit queue.
>> This means that the packets that continue to arrive
>> will pile up in the Qdisc rather than in the device
>> queues. The major advantage of this approach is that
>> codel can now do its work.
>>
>> The algorithm is really (too?) simple:
>> every 5 seconds, starts from a short queue again.
>> If a packet has been in the queue for less than 10ms,
>> allow 10 more MPDUs in.
>> If a packet has been in the queue for more than 20ms,
>> reduce by 10 the size of the transmit queue.
>>
>> The implementation is really naive and way too simple:
>>  * reading jiffies for every Tx / Tx status is not a
>>good idead.
>>  * jiffies are not fine-grained enough on all platforms
>>  * the constants chosen are really arbitrary and can't be
>>tuned.
>>  * This may be implemented in mac80211 probably and help
>>other drivers.
>>  * etc...
>>
>> But already this gives nice results. I ran a very simple
>> experiment: I put the device in a controlled environment
>> and ran traffic while running default sized ping in the
>> background. In this configuration, our device quickly
>> raises its transmission rate to the best rate.
>> Then, I force the device to use the lowest rate (6Mbps).
>> Of course, the throughput collapses, but the ping RTT
>> shoots up.
>> Using codel helps, but the latency is still high. Codel
>> with this patch gives much better results:
>>
>> pfifo_fast:
>> rtt min/avg/max/mdev = 1932.616/2393.284/2833.407/315.941 ms, pipe 3, 
>> ipg/ewma 2215.707/2446.884 ms
>>
>> fq_codel + Tx queue auto-sizing:
>> rtt min/avg/max/mdev = 13.541/32.396/54.791/9.610 ms, ipg/ewma 
>> 200.685/32.202 ms
>>
>> fq_codel without Tx queue auto-sizing:
>> rtt min/avg/max/mdev = 140.821/257.303/331.889/31.074 ms, pipe 2, ipg/ewma 
>> 258.147/252.847 ms
> This is a dramatic improvement. But I'm not sure what you are
> measuring. Is this the 6mbit test? What happens when you send traffic
> the other way (more pure acks, rather than big packets?)

Not tested. I only tested the part that I thought was most interesting:
lots of TCP traffic (charriot) with a very low rate and ping in the
background.

>
> I try to encourage folk to use flent whenever possible, for pretty
> graphs and long term measurements, so you can simultaneously measure
> both throughput and latency.
>
> flent.org's .14 release just shipped.


Ok - I hope I'll get some time to give it a try.

>> Clearly, there is more work to do to be able to merge this,
>> but it seems that the wireless problems mentioned in
>> https://lwn.net/Articles/616241/ may have a solution.
> I gave talks on the problems that wifi had with bufferbloat at the
> ieee 802.11 wg meeting a while back, and more recently it was filmed
> at battlemesh.
>
> https://www.youtube.com/watch?v=Rb-UnHDw02o
>
> I have spent my time since trying to raise sufficient resources
> (testbeds and test tools), orgs, people and money to tackle these
> problems at more depth. We made a bit of progress recently which I can
> talk about offline...
>
> In that talk I suggested that overall we move towards timestamping
> everything, that (at least in the case of the ath9k and mt72) we tie
> together aggregation with a byte based estimator similar to how BQL
> works, and I hoped that eventually - we'd be able to basically - at
> low rates, keep no more than one aggregate in the hardware, one in the
> driver queue, and one being assembled. The pending aggregate would be
> sent to the 

Re: [RFC v2] mac80211: add A-MSDU tx support

[Emmanuel Grumbach]

On Sun, Feb 7, 2016 at 1:49 PM, Felix Fietkau  wrote:
> On 2016-02-07 12:32, Emmanuel Grumbach wrote:
 +
 +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data 
 *sdata,
 + struct sta_info *sta,
 + struct ieee80211_fast_tx 
 *fast_tx,
 + struct sk_buff *skb)
 +{
 +   struct ieee80211_local *local = sdata->local;
 +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
 +   struct ieee80211_txq *txq = sta->sta.txq[tid];
 +   struct txq_info *txqi;
 +   struct sk_buff **frag_tail, *head;
 +   int subframe_len = skb->len - ETH_ALEN;
 +   int max_amsdu_len;
 +   __be16 len;
 +   void *data;
 +   bool ret = false;
 +   int n = 1;
 +
 +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
 +   return false;
 +
 +   if (!txq)
 +   return false;
 +
 +   txqi = to_txq_info(txq);
 +   if (test_bit(IEEE80211_TXQ_NO_AMSDU, >flags))
 +   return false;
 +
 +   /*
 +* A-MPDU limits maximum MPDU size to 4095 bytes. Since 
 aggregation
 +* sessions are started/stopped without txq flush, use the 
 limit here
 +* to avoid having to de-aggregate later.
 +*/
 +   max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
>>>
>>> So you can't get 10K A-MSDUs? I don't see where you check that you
>>> have an A-MPDU session here. You seem to be applying the 4095 limit
>>> also for streams that are not an A-MPDU?
>>> I guess you could check if the sta is a VHT peer, in that case, no
>>> limit applies.
>> The explanation for the missing A-MPDU change is in that comment -
>> checking for an active A-MPDU session would make it unnecessarily 
>> complex.
>> Good point about checking for VHT capabilities to remove this limit, I
>> will add that.

 Yes - I read the comment, but it seemed very sub-optimal to limit all
 the A-MSDUs to 4K. With TSO I can get up to 10K and it really helps
 TPT.
>>> This was built with the assumption that most scenarios use A-MPDU anyway
>>> and thus don't need really large A-MSDUs.
>>
>> Yes - so that's interesting. We can chose to have long A-MSDUs inside
>> a short (in terms of number of MPDUs) A-MPDU, of with shorter A-MSDU
>> and squeeze more of these into a single A-MDPU.
>> The first intuition says that we'd better have more MPDUs because of
>> the CRC check for each MPDU which doesn't exist in A-MSDU. OTOH, I
>> remember I could clearly see that I get a higher TPT with longer
>> A-MSDUs. Maybe I wasn't looking right at the size of the A-MPDU? I
>> guess I'd need to go back to the table with all the values we had, but
>> since we pretty much got what we wanted, I am not sure I will able to
>> find time for this :)
> I think it also depends on the environment. I'd guess that under very
> ideal conditions with very few retransmissions, really long A-MSDU might
> have some performance benefits, but I don't think that'll hold if the
> conditions are less than ideal and you have rate fluctuation and
> retransmissions.
>
 One more point. In VHT, there may be a limit on the numbers of
 subframes in the A-MSDU. I don't see you handle that. Maybe I missed
 it?
>>> I haven't looked at that much yet. Right now the driver can only specify
>>> a limit for the number of subframes.
>>
>> I am talking about a limitation the peer can advertise. Check this out:
>> https://git.kernel.org/cgit/linux/kernel/git/jberg/mac80211-next.git/tree/net/mac80211/cfg.c#n1134
>>
>> I couldn't see the limit the driver can specify in your code. I may
>> very well have missed it.
> I missed that one. Will add it in the next patch.
>
 This is an order 4 allocation, for each A-MSDU. Note
 that iwlwifi (and probably other drivers) can handle gather DMA in Tx,
 but they have a limited number of frags they can handle. iwlwifi e.g.
 can handle up to 20 frags, but 3 are taken for "paperwork". You'll
 have 2 frags per subframe at least (assuming that each subframe's
 payload is nicely contiguous and not on a page boundary). I think that
 it may be worthwhile to ask the driver how many frags it is supposed
 to handle. I can't promise iwlwifi will use it, but I guess it will be
 useful for someone.
>>> You mean an extra frag limit in addition to the driver subframe limit,
>>> in case individual subframes are fragmented as well?
>>>
>>
>> well.. Yes, you can't assume that you'll have one descriptor for one
>> MSDU payload  (unless the driver doesn't advertise SG to the
>> 

Re: wl12xx regression on 4.5 (was: Re: linux-4.5-rc1 TI pandboard-es wifi wlcore locks and reset)

[Emil Goode]

Hello,

On Tue, Feb 02, 2016 at 05:05:38PM +0100, Sebastian Reichel wrote:
> On Tue, Feb 02, 2016 at 04:41:13PM +1100, Ross Green wrote:
> > On Tue, Feb 2, 2016 at 3:34 PM, Sebastian Reichel  wrote:
> > > On Mon, Feb 01, 2016 at 11:38:38PM +1100, Ross Green wrote:
> > >> On Mon, Jan 25, 2016 at 11:47 PM, Ross Green  wrote:
> > >> > Just tried the new kernel release on faithful pandaboard es with the
> > >> > new 4.5-rc1 release.
> > >> >
> > >> > There is a problem with the wifi modules once the modules are loaded.
> > >> > Looks like the wifi firmware gets loaded put no response after that
> > >> > causing recovery action.
> > >> >
> > >> > the kernel 4.4 works quite happily with this board.
> > >> >
> > >> > Here is a dmesg dump in the attachment.
> > >> >
> > >> > Anyone have any ideas here?
> >
> > [...]
> > 
> > If I get time, this evening, I'll see if I can give a bisect a try.
> 
> On N950 [wl1271 via SPI, using extra patches to init from DT] I get wifi
> working again on 4.5-rc1 with 3719c17e1816 ("wlcore/wl18xx: fw logger
> over sdio") reverted.

Reverting the changes in wlcore_set_partition() as below seem to help.

Best regards,

Emil Goode

8<

--- a/drivers/net/wireless/ti/wlcore/io.c
+++ b/drivers/net/wireless/ti/wlcore/io.c
@@ -175,14 +175,15 @@ int wlcore_set_partition(struct wl1271 *wl,
if (ret < 0)
goto out;
 
+   /*
+* We don't need the size of the last partition, as it is
+* automatically calculated based on the total memory size and
+* the sizes of the previous partitions.
+*/
ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
if (ret < 0)
goto out;
 
-   ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
-   if (ret < 0)
-   goto out;
-
 out:
return ret;
 }
diff --git a/drivers/net/wireless/ti/wlcore/io.h 
b/drivers/net/wireless/ti/wlcore/io.h
index 6c257b5..10cf374 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -36,8 +36,8 @@
 #define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
 #define HW_PART2_SIZE_ADDR  (HW_PARTITION_REGISTERS_ADDR + 16)
 #define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
-#define HW_PART3_SIZE_ADDR  (HW_PARTITION_REGISTERS_ADDR + 24)
-#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
+#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
+
 #define HW_ACCESS_REGISTER_SIZE 4
 
 #define HW_ACCESS_PRAM_MAX_RANGE   0x3c000

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3] mac80211: add A-MSDU tx support

[Emmanuel Grumbach]

On Sun, Feb 7, 2016 at 3:21 PM, Felix Fietkau  wrote:
> Requires software tx queueing support. frag_list support (for zero-copy)
> is optional.
>
> Signed-off-by: Felix Fietkau 
> ---
>  include/net/mac80211.h |  17 +
>  net/mac80211/agg-tx.c  |   5 ++
>  net/mac80211/debugfs.c |   2 +
>  net/mac80211/ieee80211_i.h |   1 +
>  net/mac80211/tx.c  | 168 
> +
>  5 files changed, 193 insertions(+)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 5714774..31dca81 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -709,6 +709,7 @@ enum mac80211_tx_info_flags {
>   * @IEEE80211_TX_CTRL_PS_RESPONSE: This frame is a response to a poll
>   * frame (PS-Poll or uAPSD).
>   * @IEEE80211_TX_CTRL_RATE_INJECT: This frame is injected with rate 
> information
> + * @IEEE80211_TX_CTRL_AMSDU: This frame is an A-MSDU frame
>   *
>   * These flags are used in tx_info->control.flags.
>   */
> @@ -716,6 +717,7 @@ enum mac80211_tx_control_flags {
> IEEE80211_TX_CTRL_PORT_CTRL_PROTO   = BIT(0),
> IEEE80211_TX_CTRL_PS_RESPONSE   = BIT(1),
> IEEE80211_TX_CTRL_RATE_INJECT   = BIT(2),
> +   IEEE80211_TX_CTRL_AMSDU = BIT(3),
>  };
>
>  /*
> @@ -1964,6 +1966,12 @@ struct ieee80211_txq {
>   * order and does not need to manage its own reorder buffer or BA session
>   * timeout.
>   *
> + * @IEEE80211_HW_TX_AMSDU: Hardware (or driver) supports software aggregated
> + * A-MSDU frames. Requires software tx queueing support.
> + *
> + * @IEEE80211_HW_TX_FRAG_LIST: Hardware (or driver) supports sending 
> frag_list
> + * skbs, needed for zero-copy software A-MSDU.
> + *
>   * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
>   */
>  enum ieee80211_hw_flags {
> @@ -2001,6 +2009,8 @@ enum ieee80211_hw_flags {
> IEEE80211_HW_BEACON_TX_STATUS,
> IEEE80211_HW_NEEDS_UNIQUE_STA_ADDR,
> IEEE80211_HW_SUPPORTS_REORDERING_BUFFER,
> +   IEEE80211_HW_TX_AMSDU,
> +   IEEE80211_HW_TX_FRAG_LIST,
>
> /* keep last, obviously */
> NUM_IEEE80211_HW_FLAGS
> @@ -2073,6 +2083,11 @@ enum ieee80211_hw_flags {
>   * size is smaller (an example is LinkSys WRT120N with FW v1.0.07
>   * build 002 Jun 18 2012).
>   *
> + * @max_tx_amsdu_subframes: maximum number of subframes used in software
> + * A-MSDU aggregation
> + *
> + * @max_tx_fragments: maximum fragments per (A-)MSDU.
> + *
>   * @offchannel_tx_hw_queue: HW queue ID to use for offchannel TX
>   * (if %IEEE80211_HW_QUEUE_CONTROL is set)
>   *
> @@ -2127,6 +2142,8 @@ struct ieee80211_hw {
> u8 max_rate_tries;
> u8 max_rx_aggregation_subframes;
> u8 max_tx_aggregation_subframes;
> +   u8 max_tx_amsdu_subframes;
> +   u8 max_tx_fragments;

I have to see I still don't understand why drivers would want to limit
the number of subframes. Limiting the number of frags should be
enough, don't you think?

> u8 offchannel_tx_hw_queue;
> u8 radiotap_mcs_details;
> u16 radiotap_vht_details;
> diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
> index 4932e9f..42fa810 100644
> --- a/net/mac80211/agg-tx.c
> +++ b/net/mac80211/agg-taxx.c
> @@ -935,6 +935,7 @@ void ieee80211_process_addba_resp(struct ieee80211_local 
> *local,
>   size_t len)
>  {
> struct tid_ampdu_tx *tid_tx;
> +   struct ieee80211_txq *txq;
> u16 capab, tid;
> u8 buf_size;
> bool amsdu;
> @@ -945,6 +946,10 @@ void ieee80211_process_addba_resp(struct ieee80211_local 
> *local,
> buf_size = (capab & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK) >> 6;
> buf_size = min(buf_size, local->hw.max_tx_aggregation_subframes);
>
> +   txq = sta->sta.txq[tid];
> +   if (!amsdu && txq)
> +   set_bit(IEEE80211_TXQ_NO_AMSDU, _txq_info(txq)->flags);
> +
> mutex_lock(>ampdu_mlme.mtx);
>
> tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
> diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> index e433d0c..847779d 100644
> --- a/net/mac80211/debugfs.c
> +++ b/net/mac80211/debugfs.c
> @@ -127,6 +127,8 @@ static const char *hw_flag_names[NUM_IEEE80211_HW_FLAGS + 
> 1] = {
> FLAG(BEACON_TX_STATUS),
> FLAG(NEEDS_UNIQUE_STA_ADDR),
> FLAG(SUPPORTS_REORDERING_BUFFER),
> +   FLAG(TX_AMSDU),
> +   FLAG(TX_FRAG_LIST),
>
> /* keep last for the build bug below */
> (void *)0x1
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index a49c103..e68d8db 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -799,6 +799,7 @@ struct mac80211_qos_map {
>  enum txq_info_flags {
> IEEE80211_TXQ_STOP,
> IEEE80211_TXQ_AMPDU,
> +   IEEE80211_TXQ_NO_AMSDU,
>  };
>
>  struct txq_info {
> 

Re: [RFC v3] mac80211: add A-MSDU tx support

[Felix Fietkau]

On 2016-02-07 21:16, Emmanuel Grumbach wrote:
> On Sun, Feb 7, 2016 at 3:21 PM, Felix Fietkau  wrote:
>> Requires software tx queueing support. frag_list support (for zero-copy)
>> is optional.
>>
>> Signed-off-by: Felix Fietkau 
>> ---
>>  include/net/mac80211.h |  17 +
>>  net/mac80211/agg-tx.c  |   5 ++
>>  net/mac80211/debugfs.c |   2 +
>>  net/mac80211/ieee80211_i.h |   1 +
>>  net/mac80211/tx.c  | 168 
>> +
>>  5 files changed, 193 insertions(+)
>>
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index 5714774..31dca81 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -2127,6 +2142,8 @@ struct ieee80211_hw {
>> u8 max_rate_tries;
>> u8 max_rx_aggregation_subframes;
>> u8 max_tx_aggregation_subframes;
>> +   u8 max_tx_amsdu_subframes;
>> +   u8 max_tx_fragments;
> 
> I have to see I still don't understand why drivers would want to limit
> the number of subframes. Limiting the number of frags should be
> enough, don't you think?
Makes sense, I'll remove it.

>> u8 offchannel_tx_hw_queue;
>> u8 radiotap_mcs_details;
>> u16 radiotap_vht_details;

>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index a5aa275..f37f729 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1324,6 +1324,10 @@ struct sk_buff *ieee80211_tx_dequeue(struct 
>> ieee80211_hw *hw,
>>  out:
>> spin_unlock_bh(>queue.lock);
>>
>> +   if (skb && skb_has_frag_list(skb) &&
>> +   !ieee80211_hw_check(>hw, TX_FRAG_LIST))
>> +   skb_linearize(skb);
>> +
>> return skb;
>>  }
>>  EXPORT_SYMBOL(ieee80211_tx_dequeue);
>> @@ -2763,6 +2767,166 @@ void ieee80211_clear_fast_xmit(struct sta_info *sta)
>> kfree_rcu(fast_tx, rcu_head);
>>  }
>>
>> +static int ieee80211_amsdu_pad(struct sk_buff *skb, int subframe_len)
>> +{
>> +   int amsdu_len = subframe_len + sizeof(struct ethhdr);
>> +   int padding = (4 - amsdu_len) & 3;
>> +
>> +   if (padding)
>> +   memset(skb_put(skb, padding), 0, padding);
>> +
>> +   return padding;
>> +}
>> +
>> +static bool ieee80211_amsdu_prepare_head(struct ieee80211_sub_if_data 
>> *sdata,
>> +struct ieee80211_fast_tx *fast_tx,
>> +struct sk_buff *skb)
>> +{
>> +   struct ieee80211_local *local = sdata->local;
>> +   struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>> +   struct ieee80211_hdr *hdr;
>> +   struct ethhdr amsdu_hdr;
>> +   int hdr_len = fast_tx->hdr_len - sizeof(rfc1042_header);
>> +   int subframe_len = skb->len - hdr_len;
>> +   void *data;
>> +   u8 *qc;
>> +
>> +   if (info->control.flags & IEEE80211_TX_CTRL_AMSDU)
>> +   return true;
>> +
>> +   if (skb_headroom(skb) < sizeof(amsdu_hdr) || skb_tailroom(skb) < 3) {
>> +   I802_DEBUG_INC(local->tx_expand_skb_head);
>> +
>> +   if (pskb_expand_head(skb, sizeof(amsdu_hdr), 3, GFP_ATOMIC)) 
>> {
>> +   wiphy_debug(local->hw.wiphy,
>> +   "failed to reallocate TX buffer\n");
>> +   return false;
>> +   }
>> +   }
>> +
>> +   subframe_len += ieee80211_amsdu_pad(skb, subframe_len);
>> +
>> +   amsdu_hdr.h_proto = cpu_to_be16(subframe_len);
>> +   memcpy(amsdu_hdr.h_source, skb->data + fast_tx->sa_offs, ETH_ALEN);
>> +   memcpy(amsdu_hdr.h_dest, skb->data + fast_tx->da_offs, ETH_ALEN);
>> +
>> +   data = skb_push(skb, sizeof(amsdu_hdr));
>> +   memmove(data, data + sizeof(amsdu_hdr), hdr_len);
>> +   memcpy(data + hdr_len, _hdr, sizeof(amsdu_hdr));
>> +
>> +   hdr = data;
>> +   qc = ieee80211_get_qos_ctl(hdr);
>> +   *qc |= IEEE80211_QOS_CTL_A_MSDU_PRESENT;
>> +
>> +   info->control.flags |= IEEE80211_TX_CTRL_AMSDU;
>> +
>> +   return true;
>> +}
>> +
>> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
>> + struct sta_info *sta,
>> + struct ieee80211_fast_tx *fast_tx,
>> + struct sk_buff *skb)
>> +{
>> +   struct ieee80211_local *local = sdata->local;
>> +   u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>> +   struct ieee80211_txq *txq = sta->sta.txq[tid];
>> +   struct txq_info *txqi;
>> +   struct sk_buff **frag_tail, *head;
>> +   int subframe_len = skb->len - ETH_ALEN;
>> +   u8 max_subframes = 0xff;
>> +   int max_frags = local->hw.max_tx_fragments;
>> +   int max_amsdu_len;
>> +   __be16 len;
>> +   void *data;
>> +   bool ret = false;
>> +   int n = 1, nfrags;
>> +
>> +   if (!ieee80211_hw_check(>hw, TX_AMSDU))
>> +   return false;
>> +
>> +  

Re: [PATCH RESEND 40/70] staging: wilc1000: wilc_wlan.c: replaces PRINT_ER with netdev_err

[Greg KH]

On Thu, Feb 04, 2016 at 06:24:01PM +0900, Glen Lee wrote:
> From: Leo Kim 
> 
> This patches replaces PRINT_ER with netdev_err.

No, that's not what this patch does :(

> Removes unnecessary debug logs as well.

That's what this patch does.

Please be more careful.

I'm not going to apply this patch.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] staging: rtl8723au: whitespace and blank line cleaning

[Julian Calaby]

Hi Geliang,

On Sun, Feb 7, 2016 at 2:30 PM, Geliang Tang  wrote:
> This patch cleans whitespaces and blank lines surrounding
> list_for_each_entry*().

It does a lot more than this, including:
 - Removing some unnecessary brackets
 - Whitespace changes well away from the list_for_each_entry*() calls

You need to either specify all of these in the changelog or split it
up into multiple patches.

Thanks,

Julian Calaby


>
> Signed-off-by: Geliang Tang 
> ---
> Changes in v3:
>  - split it into three patches.
> Changes in v2:
>  - drop the coding style fixing in v1.
> ---
>  drivers/staging/rtl8723au/core/rtw_ap.c   | 41 
> ++-
>  drivers/staging/rtl8723au/core/rtw_mlme.c | 14 +---
>  drivers/staging/rtl8723au/core/rtw_mlme_ext.c |  3 +-
>  drivers/staging/rtl8723au/core/rtw_recv.c | 18 +++---
>  drivers/staging/rtl8723au/core/rtw_sta_mgt.c  | 11 +-
>  drivers/staging/rtl8723au/core/rtw_xmit.c | 30 -
>  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c |  8 -
>  7 files changed, 17 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c 
> b/drivers/staging/rtl8723au/core/rtw_ap.c
> index 4bcd617..ce4b589 100644
> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
> @@ -171,7 +171,7 @@ static u8 chk_sta_is_alive(struct sta_info *psta)
> return ret;
>  }
>
> -void   expire_timeout_chk23a(struct rtw_adapter *padapter)
> +void expire_timeout_chk23a(struct rtw_adapter *padapter)
>  {
> struct list_head *phead;
> u8 updated = 0;
> @@ -182,12 +182,9 @@ void   expire_timeout_chk23a(struct rtw_adapter 
> *padapter)
> int i;
>
> spin_lock_bh(>auth_list_lock);
> -
> phead = >auth_list;
> -
> /* check auth_queue */
> list_for_each_entry_safe(psta, ptmp, phead, auth_list) {
> -
> if (psta->expire_to > 0) {
> psta->expire_to--;
> if (psta->expire_to == 0) {
> @@ -205,18 +202,13 @@ void  expire_timeout_chk23a(struct rtw_adapter 
> *padapter)
> spin_lock_bh(>auth_list_lock);
> }
> }
> -
> }
> -
> spin_unlock_bh(>auth_list_lock);
>
> spin_lock_bh(>asoc_list_lock);
> -
> phead = >asoc_list;
> -
> /* check asoc_queue */
> list_for_each_entry_safe(psta, ptmp, phead, asoc_list) {
> -
> if (chk_sta_is_alive(psta) || !psta->expire_to) {
> psta->expire_to = pstapriv->expire_to;
> psta->keep_alive_trycnt = 0;
> @@ -281,7 +273,6 @@ voidexpire_timeout_chk23a(struct rtw_adapter 
> *padapter)
> }
> }
> }
> -
> spin_unlock_bh(>asoc_list_lock);
>
> if (chk_alive_num) {
> @@ -1071,11 +1062,8 @@ int rtw_acl_add_sta23a(struct rtw_adapter *padapter, 
> u8 *addr)
> return -1;
>
> spin_lock_bh(_node_q->lock);
> -
> phead = get_list_head(pacl_node_q);
> -
> list_for_each_entry(paclnode, phead, list) {
> -
> if (!memcmp(paclnode->addr, addr, ETH_ALEN)) {
> if (paclnode->valid == true) {
> added = true;
> @@ -1084,7 +1072,6 @@ int rtw_acl_add_sta23a(struct rtw_adapter *padapter, u8 
> *addr)
> }
> }
> }
> -
> spin_unlock_bh(_node_q->lock);
>
> if (added)
> @@ -1127,11 +1114,8 @@ int rtw_acl_remove_sta23a(struct rtw_adapter 
> *padapter, u8 *addr)
> DBG_8723A("%s(acl_num =%d) = %pM\n", __func__, pacl_list->num, addr);
>
> spin_lock_bh(_node_q->lock);
> -
> phead = get_list_head(pacl_node_q);
> -
> list_for_each_entry_safe(paclnode, ptmp, phead, list) {
> -
> if (!memcmp(paclnode->addr, addr, ETH_ALEN)) {
> if (paclnode->valid) {
> paclnode->valid = false;
> @@ -1142,7 +1126,6 @@ int rtw_acl_remove_sta23a(struct rtw_adapter *padapter, 
> u8 *addr)
> }
> }
> }
> -
> spin_unlock_bh(_node_q->lock);
>
> DBG_8723A("%s, acl_num =%d\n", __func__, pacl_list->num);
> @@ -1355,14 +1338,9 @@ void associated_clients_update23a(struct rtw_adapter 
> *padapter, u8 updated)
> struct sta_priv *pstapriv = >stapriv;
>
> spin_lock_bh(>asoc_list_lock);
> -
> phead = >asoc_list;
> -
> -   list_for_each_entry_safe(psta, ptmp, phead, asoc_list) {
> -
> +   list_for_each_entry_safe(psta, ptmp, phead, asoc_list)
> VCS_update23a(padapter, psta);
> -   }
> -
> spin_unlock_bh(>asoc_list_lock);
> }
>  }
> @@ -1637,9 

Re: [PATCH 5/8] staging: wilc1000: fix return type of wilc_send_config_pkt

[Greg KH]

On Fri, Feb 05, 2016 at 10:35:15AM +0900, Chaehyun Lim wrote:
> wilc_send_config_pkt is returned 0 or -ETIMEDOUT according to return
> value of wilc_wlan_cfg_get or wilc_wlan_cfg_set.
> It is better to use int type to represent linux standard error code.
> 
> Signed-off-by: Chaehyun Lim 
> ---
>  drivers/staging/wilc1000/coreconfigurator.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

This, and the remaining patches in the series, don't apply anymore due
to patches I took right before your series.  Can you refresh these and
resend?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] brcmfmac: Increase nr of supported flowrings.

[Rafał Miłecki]

From: Hante Meuleman 

New generation devices have firmware which has more than 256 flowrings.
E.g. following debugging message comes from 14e4:4365 BCM4366:
[  194.606245] brcmfmac: brcmf_pcie_init_ringbuffers Nr of flowrings is 264

At various code places (related to flowrings) we were using u8 which
could lead to storing wrong number or infinite loops when indexing with
this type. This issue was quite easy to spot in brcmf_flowring_detach
where it led to infinite loop e.g. on failed initialization.

This patch switches code to proper types and increases the maximum
number of supported flowrings to 512.

Originally this change was sent in September 2015, but back it was
causing a regression on BCM43602 resulting in:
Unable to handle kernel NULL pointer dereference at virtual address ...

The reason for this regression was missing update (s/u8/u16) of struct
brcmf_flowring_ring. This problem was handled in 9f64df9 ("brcmfmac: Fix
bug in flowring management."). Starting with that it's safe to apply
this original patch as it doesn't cause a regression anymore.

This patch fixes an infinite loop on BCM4366 which is supported since
4.4 so it makes sense to apply it to stable 4.4+.

Cc:  # 4.4+
Reviewed-by: Arend Van Spriel 
Reviewed-by: Franky (Zhenhui) Lin 
Reviewed-by: Pieter-Paul Giesberts 
Signed-off-by: Hante Meuleman 
Signed-off-by: Arend van Spriel 
Signed-off-by: Rafał Miłecki 
---
 .../broadcom/brcm80211/brcmfmac/flowring.c | 38 --
 .../broadcom/brcm80211/brcmfmac/flowring.h | 20 ++--
 .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 11 +--
 .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.h  |  2 +-
 4 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
index 2ca783f..7e269f9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
@@ -32,7 +32,7 @@
 #define BRCMF_FLOWRING_LOW (BRCMF_FLOWRING_HIGH - 256)
 #define BRCMF_FLOWRING_INVALID_IFIDX   0xff
 
-#define BRCMF_FLOWRING_HASH_AP(da, fifo, ifidx) (da[5] + fifo + ifidx * 16)
+#define BRCMF_FLOWRING_HASH_AP(da, fifo, ifidx) (da[5] * 2 + fifo + ifidx * 16)
 #define BRCMF_FLOWRING_HASH_STA(fifo, ifidx) (fifo + ifidx * 16)
 
 static const u8 brcmf_flowring_prio2fifo[] = {
@@ -68,7 +68,7 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
  u8 prio, u8 ifidx)
 {
struct brcmf_flowring_hash *hash;
-   u8 hash_idx;
+   u16 hash_idx;
u32 i;
bool found;
bool sta;
@@ -88,6 +88,7 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
}
hash_idx =  sta ? BRCMF_FLOWRING_HASH_STA(fifo, ifidx) :
  BRCMF_FLOWRING_HASH_AP(mac, fifo, ifidx);
+   hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
found = false;
hash = flow->hash;
for (i = 0; i < BRCMF_FLOWRING_HASHSIZE; i++) {
@@ -98,6 +99,7 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
break;
}
hash_idx++;
+   hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
}
if (found)
return hash[hash_idx].flowid;
@@ -111,7 +113,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
 {
struct brcmf_flowring_ring *ring;
struct brcmf_flowring_hash *hash;
-   u8 hash_idx;
+   u16 hash_idx;
u32 i;
bool found;
u8 fifo;
@@ -131,6 +133,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
}
hash_idx =  sta ? BRCMF_FLOWRING_HASH_STA(fifo, ifidx) :
  BRCMF_FLOWRING_HASH_AP(mac, fifo, ifidx);
+   hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
found = false;
hash = flow->hash;
for (i = 0; i < BRCMF_FLOWRING_HASHSIZE; i++) {
@@ -140,6 +143,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
break;
}
hash_idx++;
+   hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
}
if (found) {
for (i = 0; i < flow->nrofrings; i++) {
@@ -169,7 +173,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 
da[ETH_ALEN],
 }
 
 
-u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid)
+u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u16 flowid)
 {
struct brcmf_flowring_ring *ring;
 
@@ -179,7 +183,7 @@ u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 
flowid)
 }
 
 
-static void brcmf_flowring_block(struct brcmf_flowring *flow,