diff mbox

iwlwifi A-MSDU tx

Message ID CANUX_P1zs7sPR5kGabX1kk2VPWQM=HRp0t3SeLz7U-EFdcZBkw@mail.gmail.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Emmanuel Grumbach Dec. 14, 2015, 12:42 p.m. UTC 
On Sat, Dec 12, 2015 at 10:54 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 10:02 PM, Stefan Sperling <stsp@openbsd.org> wrote:
>> Hi Emmanuel,
>>
>> Thanks a bunch, this worked out quite well for me.
>> I got what I was aiming for out of this already but I still have some
>> additional questions if you don't mind.
>
> Happy to hear that.
>
>>
>> Do you know of a good way to generate A-MSDUs?
>> One way I found is to run:
>>   top -d .01
>> on the AP and watching this top over SSH from my associated OpenBSD client.
>
> iperf? :)
>
> You can also load iwlwifi with 11n_disable=2. This will prevent A-MPDU
> if you don't want to have A-MSDU in A-MPDU at that stage.
>
>>
>> It seems only locally generated packets will trigger A-MSDUs.
>> I suppose forwarded frames always fit the interface MTU and don't
>> trigger A-MSDUs. Is there a way to test this with forwarded traffic?
>
> Hmm. Good question. A-MSDUs are created each time we get a large send
> from the network stack. I think that this relies on the socket
> occupancy which means that you are right. I don't think we can test
> this with forwarded traffic for now.
>
>>
>> One issue I discovered is that OpenBSD (with my partly uncommitted 11n
>> patch set) cannot decrypt encrypted A-MSDUs sent by iwlwifi.
>> Frames carrying normal MSDUs have a CCMP header and are decrypted correctly.
>> But frames carrying A-MSDUs have a wep/tkip header (wireshark shows
>> "WEP parameters" instead of CCMP parameters") and decryption fails on
>> OpenBSD in ieee80211_ccmp_decrypt() because the ExtIV bit is not set.
>> It doesn't look like the source of this problem is at OpenBSD's end since
>> CCMP is required for HT. So in case the firmware doesn't produce CCMP for
>> A-MSDUs, this would be a problem in the firmware. Can you confirm this?
>> I also tried running iwlwifi in software crypto mode but the AP would
>> not send A-MSDUs at all in that case.
>
> SW crypto won't work for A-MSDU since it'll refuse LSO which in turn
> disables A-MSDU.
> I don't really touch the wifi header, I just duplicate it if needed,
> so I don't see off the top of my head where the problem could be.
> Thanks for reporting it.
>

I found the problem. Patch attached.

>>
>> On the receiving OpenBSD side I'm currently limited to 4k A-MSDU and it
>> turns out the AP won't send more than 2 subframes per A-MSDU in this case.
>
> with default MTU, you won't get more than that.
>
>> To test with more than 2 subframes I applied the following crude patch which
>> makes the AP send more subframes but crashes the firmware after the frame
>> is sent. The driver recovers fine and traffic keeps flowing but that's
>> not pretty. Do you have a better idea? If not, no problem. I mainly did
>> this to confirm that OpenBSD won't crash handling A-MSDU with more than
>> 2 subframes, which it won't.
>
> ouch. No :)  What you should really do is to reduce the MTU to 500 bytes.
> iperf -M will do.
>
>>
>> Thanks again,
>> Stefan
>>
>> diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c
>> index 7ece5c1..b3d562d 100644
>> --- a/drivers/net/wireless/iwlwifi/mvm/tx.c
>> +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c
>> @@ -448,7 +448,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb,
>>         struct iwl_mvm_sta *mvmsta = iwl_mvm_sta_from_mac80211(sta);
>>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>>         struct ieee80211_hdr *hdr = (void *)skb->data;
>> -       unsigned int mss = skb_shinfo(skb)->gso_size;
>> +       unsigned int mss = skb_shinfo(skb)->gso_size / 2;
>>         struct sk_buff *tmp, *next;
>>         char cb[sizeof(skb->cb)];
>>         unsigned int num_subframes, tcp_payload_len, subf_len, max_amsdu_len;
>>
>> On Fri, Dec 04, 2015 at 12:30:52PM +0200, Emmanuel Grumbach wrote:
>>> Adding the right mailing list this time.
>>>
>>> On Fri, Dec 4, 2015 at 10:18 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>>> > On Fri, Dec 4, 2015 at 9:35 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>>> >> Hi,
>>> >>
>>> >> On Fri, Dec 4, 2015 at 12:05 AM, Stefan Sperling <stsp@openbsd.org> wrote:
>>> >>> Hi Emmanuel,
>>> >>>
>>> >>> As part of implementing 802.11n support in OpenBSD I'm looking for
>>> >>> an AP which sends A-MSDUs. Preferrably under software control rather
>>> >>> than firmware.
>>> >>>
>>> >>> I found your iwlwifi A-MSDU patches at
>>> >>> http://marc.info/?l=linux-wireless&m=144553311122495&w=2
>>> >>> and http://marc.info/?l=linux-wireless&m=144553311822496&w=2
>>> >>>
>>> >>> Would these patches make it possible to run an AP with an 5300 or 7260
>>> >>> card and send A-MSDUs? That would be great as I have the necessary hardware.
>>> >>
>>> >> 7260 will go. Not 5300.
>>> >> Note that this code simulates Tx TCP Csum offload. I did that to write
>>> >> the code while we still don't have the hardware that has this
>>> >> capability. But for testing purposes, it should do the work. The
>>> >> testing teams have reported that AP mode didn't work quite well with
>>> >> this and I haven't taken the time to look into yet, but if you see
>>> >> issues, please report them or even better: fix them :)
>>> >>
>>> >>>
>>> >>> Which Linux tree do these patches apply to? I've tried the following
>>> >>> but had no luck in getting these patches applied without rejects:
>>> >>>
>>> >>> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>> >>> git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git
>>> >>>
>>> >>
>>> >> Right. These patches had a long internal review cycle. They are now
>>> >> merge in our internal tree.
>>> >> You can find them in our internal tree [1] (backport based). I will
>>> >> push them soon in iwlwifi-next.git.
>>> >
>>> > I forgot to mention that you need to change the define of
>>> > IWL_MVM_SW_TX_CSUM_OFFLOAD to 1.
>>> >
>>> >>
>>> >> [1] - https://git.kernel.org/cgit/linux/kernel/git/iwlwifi/backport-iwlwifi.git/
>>> >> Please read https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/core_release#how_to_install_the_driver.
>>> >>
>>> >>> Thanks,
>>> >>> Stefan
diff mbox

Patch

From 9ef0db4a69297bcc001c8d2588801bc489834b59 Mon Sep 17 00:00:00 2001
From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Date: Mon, 14 Dec 2015 14:31:59 +0200
Subject: [PATCH] [BUGFIX] iwlwifi: pcie: correctly handle the IV in A-MSDU
 [SQUASH]

The IV needs to be copied before it is pulled from the skb.
This prevented TSO to work properly with iv_len != 0.

type=bugfix
bug=not-tracked
fixes=I9d02be5b8349120e73d8997d91182aec2f42290f

Change-Id: Ieab88fdc18cd90066ff571ea2bd11ba79ea6f040
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
 drivers/net/wireless/iwlwifi/pcie/tx.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/pcie/tx.c b/drivers/net/wireless/iwlwifi/pcie/tx.c
index fd36eea..14d5ffd 100644
--- a/drivers/net/wireless/iwlwifi/pcie/tx.c
+++ b/drivers/net/wireless/iwlwifi/pcie/tx.c
@@ -1977,15 +1977,10 @@  static int iwl_fill_data_tbs_amsdu(struct iwl_trans *trans, struct sk_buff *skb,
 			     &dev_cmd->hdr, IWL_HCMD_SCRATCHBUF_SIZE + tb1_len,
 			     NULL, 0);
 
-	/*
-	 * Pull the ieee80211 header + IV to be able to use TSO core,
-	 * we will restore it for the tx_status flow.
-	 */
-	skb_pull(skb, hdr_len + iv_len);
 	ip_hdrlen = skb_transport_header(skb) - skb_network_header(skb);
 	snap_ip_tcp_hdrlen =
 		IEEE80211_CCMP_HDR_LEN + ip_hdrlen + tcp_hdrlen(skb);
-	total_len = skb->len - snap_ip_tcp_hdrlen;
+	total_len = skb->len - snap_ip_tcp_hdrlen - hdr_len - iv_len;
 	amsdu_pad = 0;
 
 	/* total amount of header we may need for this A-MSDU */
@@ -2000,9 +1995,15 @@  static int iwl_fill_data_tbs_amsdu(struct iwl_trans *trans, struct sk_buff *skb,
 	get_page(hdr_page->page);
 	start_hdr = hdr_page->pos;
 	info->driver_data[IWL_TRANS_FIRST_DRIVER_DATA] = hdr_page->page;
-	memcpy(hdr_page->pos, skb->data, iv_len);
+	memcpy(hdr_page->pos, skb->data + hdr_len, iv_len);
 	hdr_page->pos += iv_len;
 
+	/*
+	 * Pull the ieee80211 header + IV to be able to use TSO core,
+	 * we will restore it for the tx_status flow.
+	 */
+	skb_pull(skb, hdr_len + iv_len);
+
 	tso_start(skb, &tso);
 
 	while (total_len) {
-- 
2.5.0