Return-path: Received: from mail-ea0-f174.google.com ([209.85.215.174]:46384 "EHLO mail-ea0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750798AbaB1JPV convert rfc822-to-8bit (ORCPT ); Fri, 28 Feb 2014 04:15:21 -0500 Received: by mail-ea0-f174.google.com with SMTP id h14so2410858eaj.5 for ; Fri, 28 Feb 2014 01:15:20 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <8738j3d2t2.fsf@kamboji.qca.qualcomm.com> References: <1392629563-31046-1-git-send-email-michal.kazior@tieto.com> <1393485587-16879-1-git-send-email-michal.kazior@tieto.com> <1393485587-16879-5-git-send-email-michal.kazior@tieto.com> <8738j3d2t2.fsf@kamboji.qca.qualcomm.com> Date: Fri, 28 Feb 2014 10:15:20 +0100 Message-ID: (sfid-20140228_101529_636778_F0B2FC00) Subject: Re: [PATCH v3 4/8] ath10k: bypass htc for htt tx path From: Michal Kazior To: Kalle Valo Cc: "ath10k@lists.infradead.org" , linux-wireless Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 28 February 2014 10:06, Kalle Valo wrote: > Michal Kazior writes: > >> Going through full htc tx path for htt tx is a >> waste of resources. By skipping it it's possible >> to easily submit scatter-gather to the pci hif for >> reduced host cpu load and improved performance. >> >> The new approach uses dma pool to store the >> following metadata for each tx request: >> * msdu fragment list >> * htc header >> * htt tx command >> >> The htt tx command contains a msdu prefetch. >> Instead of copying it original mapped msdu address >> is used to submit a second scatter-gather item to >> hif to make a complete htt tx command. >> >> The htt tx command itself hands over dma mapped >> pointers to msdus and completion of the command >> itself doesn't mean the frame has been sent and >> can be unmapped/freed. This is why htc tx >> completion is skipped for htt tx as all tx related >> resources are freed upon htt tx completion >> indication event (which also implicitly means htt >> tx command itself was completed). >> >> Since now each htt tx request effectively consists >> of 2 copy engine items CE_HTT_H2T_MSG_SRC_NENTRIES >> is updated to allow maximum of >> TARGET_10X_NUM_MSDU_DESC msdus being queued. This >> keeps the tx path resource management simple. >> >> Signed-off-by: Michal Kazior >> --- >> v2: >> * improve commit log >> * improve comment in code >> * fix sparse/checkpatch/buildbot warnings > > [...] > >> --- a/drivers/net/wireless/ath/ath10k/htc.c >> +++ b/drivers/net/wireless/ath/ath10k/htc.c >> @@ -202,10 +202,8 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar, >> struct ath10k_htc *htc = &ar->htc; >> struct ath10k_htc_ep *ep = &htc->endpoint[eid]; >> >> - if (!skb) { >> - ath10k_warn("invalid sk_buff completion - NULL pointer. firmware crashed?\n"); >> + if (WARN_ON(!skb)) >> return 0; >> - } > > WARN_ON() is a bit dangerous here as it might cause excessive spamming. > Why did you want to change this? I think either ath10k_warn() or > WARN_ON_ONCE() would be safer, but not sure which one to use. After the scatter-gather patch no NULL skb should be ever passed to tx completion handler as those are ignored by hif/pci. Perhaps the hunk should be moved from this patch to the scatter-gather one. Perhaps WARN_ON() is a bit over the top here, but since this is now more of a logic issue rather than runtime issue I decided to change it from ath10k_warn to WARN_ON(). It's probably still a good idea to make it _ONCE generally, although if you actually get skbuff it's already too late or it should be screaming loudly because someone must've changed the code in an incorrect/incomplete way. Michał