From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sipsolutions.net (s3.sipsolutions.net [5.9.151.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 471753B25D for ; Wed, 31 Aug 2016 17:06:44 -0400 (EDT) Received: by sipsolutions.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1bfCiW-0001fi-E8; Wed, 31 Aug 2016 23:06:40 +0200 Message-ID: <1472677599.5470.13.camel@sipsolutions.net> From: Johannes Berg To: Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= , make-wifi-fast@lists.bufferbloat.net, linux-wireless@vger.kernel.org Date: Wed, 31 Aug 2016 23:06:39 +0200 In-Reply-To: <20160830131548.6014-1-toke@toke.dk> (sfid-20160830_151618_124950_CD061443) References: <20160824162015.29933-1-toke@toke.dk> <20160830131548.6014-1-toke@toke.dk> (sfid-20160830_151618_124950_CD061443) Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.4-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: Re: [Make-wifi-fast] [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue. X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Aug 2016 21:06:44 -0000 On Tue, 2016-08-30 at 15:15 +0200, Toke Høiland-Jørgensen wrote: > @@ -829,10 +844,16 @@ ieee80211_tx_h_sequence(struct > ieee80211_tx_data *tx) >    */ >   if (!ieee80211_is_data_qos(hdr->frame_control) || >       is_multicast_ether_addr(hdr->addr1)) { > - /* driver should assign sequence number */ > - info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ; > - /* for pure STA mode without beacons, we can do it > */ > - hdr->seq_ctrl = cpu_to_le16(tx->sdata- > >sequence_number); > + fragnum = 0; > + seq = cpu_to_le16(tx->sdata->sequence_number); > + skb_queue_walk(&tx->skbs, skb) { > + info = IEEE80211_SKB_CB(skb); > + hdr = (struct ieee80211_hdr *)skb->data; > + /* driver should assign sequence number */ > + info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ; > + /* for pure STA mode without beacons, we can > do it */ > + hdr->seq_ctrl = seq | fragnum++; I would very much prefer you kept fragnum assignment in the fragmentation handler. Also, you just broke this on big endian, please run sparse on your patches if you don't see these things directly. > + if (!fast_tx || > +     !ieee80211_xmit_fast_finish(sta->sdata, sta, > fast_tx, skb, > + false)) { > + /* fast xmit was started, but fails to > finish */ > + ieee80211_free_txskb(hw, skb); > + goto begin; > + } That obviously cannot happen, it can't fail to finish. See the comments in xmit_fast() and the return values ... > +static int invoke_tx_handlers(struct ieee80211_tx_data *tx) > +{ > + return invoke_tx_handlers_early(tx) || > invoke_tx_handlers_late(tx); > +} Ugh, please, no, don't be tricky where it's not necessary. Now every person reading this has to first look up the return type, and then the return value, and make sure they understand that success is actually the value 0 ... that's way too much to ask.   > +ieee80211_tx_result > +ieee80211_tx_h_michael_mic_add(struct ieee80211_tx_data *tx) > +{ > + struct sk_buff *skb; > + ieee80211_tx_result r; > + > + skb_queue_walk(&tx->skbs, skb) { > + r = ieee80211_tx_h_michael_mic_add_skb(tx, skb); > + if (r != TX_CONTINUE) > + return r; > + } > + return TX_CONTINUE; > +} You just broke TKIP completely again. Adding the MMIC and fragmentation are not commutative operations. johannes