From patchwork Mon Dec 3 17:48:05 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Lamparter X-Patchwork-Id: 1834941 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id C147C3FC5A for ; Mon, 3 Dec 2012 18:58:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751728Ab2LCS6y (ORCPT ); Mon, 3 Dec 2012 13:58:54 -0500 Received: from mail-bk0-f46.google.com ([209.85.214.46]:43988 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751375Ab2LCS6x (ORCPT ); Mon, 3 Dec 2012 13:58:53 -0500 Received: by mail-bk0-f46.google.com with SMTP id q16so1242960bkw.19 for ; Mon, 03 Dec 2012 10:58:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:date:subject:to:cc:message-id; bh=9M6uXrshusBLroMUju3xi46E5sNM3oge25qrE5rg2z0=; b=bennOIcxDlHF1bLtxu4u/mNpwGJe0C+7SZWD5Ib5FpFHVnM5ewzmWStYq0tBwx/5Rn 8lvDRP50It7HUIz0codY1gwWJlJV05c1Cu3HpgGaEtRwLueWgHRaAY5qgF3Mf+yV8UwL I4dYYLHJrV7gzJAvPwQW01/FKUx82hpSWFAmlkE1M2hH7wAqFIfXKwSfjyZb535e40/v a3wQAJFPZJyh6Il6EYXFmSXJ9sBw2nktD27gqKVMlf/Akz1I1XAL49ryS1iywIEli2IT 8zp3FBftbKoxtTzm5Tx8xlOpzk9k0D0oav4/jQvii/wd95/V8+uIyfI5JXxoTF8ZSaNX Oc8Q== Received: by 10.204.149.11 with SMTP id r11mr3117266bkv.93.1354561132293; Mon, 03 Dec 2012 10:58:52 -0800 (PST) Received: from blech.mobile (f053219209.adsl.alicedsl.de. [78.53.219.209]) by mx.google.com with ESMTPS id y11sm8245942bkw.8.2012.12.03.10.58.49 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 03 Dec 2012 10:58:50 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by blech.mobile (Postfix) with ESMTP id 8941E100406; Mon, 3 Dec 2012 19:58:48 +0100 (CET) Received: from blech.mobile ([127.0.0.1]) by localhost (localhost [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id feAD-A4v0vmo; Mon, 3 Dec 2012 19:58:39 +0100 (CET) Received: from blech.localnet (localhost [127.0.0.1]) by blech.mobile (Postfix) with ESMTP id E2A80100286; Mon, 3 Dec 2012 19:58:38 +0100 (CET) From: Christian Lamparter Date: Mon, 3 Dec 2012 18:48:05 +0100 Subject: [PATCH] carl9170: explain why sta cannot be NULL for ampdus To: linux-wireless@vger.kernel.org Cc: linville@tuxdriver.com, dan.carpenter@oracle.com Message-Id: <20121203185838.E2A80100286@blech.mobile> Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org Dan Carpenter reported that smatch detected a potential problem with the code [1]: drivers/net/wireless/ath/carl9170/tx.c:1488 carl9170_op_tx() error: we previously assumed 'sta' could be null (see line 1482) drivers/net/wireless/ath/carl9170/tx.c 1482 if (sta) { ^^^^^ New check. [...] 1485 } 1487 if (info->flags & IEEE80211_TX_CTL_AMPDU) { 1488 run = carl9170_tx_ampdu_queue(ar, sta, skb); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Old dereference of "sta" inside the call to carl9170_tx_ampdu_queue(). A range of solutions have been discussed in [2] and we agreed on the following: " > we might as well add a comment to carl9170_tx_ampdu_queue > and explain the situation [in a way that's obvious to a > human reader]. This way we can save the "if"... which is > a small win since carl9170_op_tx is sort of a hot-path. Putting a comment there is fine. Without the comment it's easy for a human reader to get confused why the check is there. So long as humans can read the code, that's all that matters." [1] [2] Reported-by: Dan Carpenter Signed-off-by: Christian Lamparter --- drivers/net/wireless/ath/carl9170/tx.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c index 6a86814..45c696b 100644 --- a/drivers/net/wireless/ath/carl9170/tx.c +++ b/drivers/net/wireless/ath/carl9170/tx.c @@ -1485,6 +1485,13 @@ void carl9170_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb) } if (info->flags & IEEE80211_TX_CTL_AMPDU) { + /* to static code analyzers and reviewers: + * mac80211 guarantees that a valid "sta" + * reference is present, if a frame is to + * be part of an ampdu. Hence any extra + * sta == NULL checks are redundant in this + * special case. + */ run = carl9170_tx_ampdu_queue(ar, sta, skb); if (run) carl9170_tx_ampdu(ar);