From patchwork Fri Aug 22 20:38:08 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Lamparter X-Patchwork-Id: 382350 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 1CE0D1400EA for ; Sat, 23 Aug 2014 06:38:40 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751853AbaHVUiW (ORCPT ); Fri, 22 Aug 2014 16:38:22 -0400 Received: from mail-lb0-f176.google.com ([209.85.217.176]:39511 "EHLO mail-lb0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751234AbaHVUiN (ORCPT ); Fri, 22 Aug 2014 16:38:13 -0400 Received: by mail-lb0-f176.google.com with SMTP id u10so9848200lbd.21 for ; Fri, 22 Aug 2014 13:38:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding:content-type; bh=VUnAKZyZTAUSXz/X/1s9ctll5m53bBL1SDS9YBocOAM=; b=M4Odl6O0N82AfjC4RwQkkNbc2O3rzn7iE6bYrXepr+ucUQ8Aan+kKJGGr8IVgXQBSd HjpIVHLiFKZ4M5HpWgM0252BozFM8WYk1X1eMBBjM9dohriYm+I0OLetuMsjfEuIIwWI lWnVwVzGGO2p0o3RFu2YG2L5y0kXFZzW2rqqHcCRT82reBFmZKW4amBa7ht/ey8Oh+W7 uMUauDk/Bs2xCk7xfPggdpGWzfhrprO7eD27svx0wm13BVmRdwAT2/54bNvnOlG9KfY1 QTBkF7E7fCA3vhe4J65+yiW2mc/OOelJ/h0kZeq5qfAeBEVCfJIzIcLcyO2BJHjgiwcJ 0PJw== X-Received: by 10.152.44.129 with SMTP id e1mr4422252lam.91.1408739891499; Fri, 22 Aug 2014 13:38:11 -0700 (PDT) Received: from debian64.daheim (p5B2E73F2.dip0.t-ipconnect.de. [91.46.115.242]) by mx.google.com with ESMTPSA id th7sm28439954lbb.36.2014.08.22.13.38.09 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 22 Aug 2014 13:38:10 -0700 (PDT) Received: from localhost ([127.0.0.1] helo=debian64.localnet) by debian64.daheim with esmtps (TLS1.0:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1XKvb6-0000xr-HJ; Fri, 22 Aug 2014 22:38:08 +0200 From: Christian Lamparter To: Andreea-Cristina Bernat Cc: linville@tuxdriver.com, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, paulmck@linux.vnet.ibm.com Subject: Re: [PATCH v2] carl9170: Remove redundant protection check Date: Fri, 22 Aug 2014 22:38:08 +0200 Message-ID: <1679858.MgakDZE2Vr@debian64> User-Agent: KMail/4.13.3 (Linux/3.17.0-rc1-wl+; KDE/4.14.0; x86_64; ; ) In-Reply-To: <20140822191431.GA5827@ada> References: <20140822191431.GA5827@ada> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Friday, August 22, 2014 10:14:31 PM Andreea-Cristina Bernat wrote: > The carl9170_op_ampdu_action() function is used only by the mac80211 > framework. Since the mac80211 already takes care of checks and > properly serializing calls to the driver's function there is no > need for the driver to do the same thing. > > Signed-off-by: Andreea-Cristina Bernat > --- > Changes in v2: > - Change subject line from > "carl9170: Replace rcu_dereference() with rcu_access_pointer()" > to > "carl9170: Remove redundant protection check" > - Update the commit message according to the modifications > - Delete the lines of interest at the suggestion and explanations of > Christian Lamparter > > drivers/net/wireless/ath/carl9170/main.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > index 12018ff..6758b9a 100644 > --- a/drivers/net/wireless/ath/carl9170/main.c > +++ b/drivers/net/wireless/ath/carl9170/main.c > @@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, > if (!sta_info->ht_sta) > return -EOPNOTSUPP; > > - rcu_read_lock(); > - if (rcu_access_pointer(sta_info->agg[tid])) { > - rcu_read_unlock(); > - return -EBUSY; > - } > - > tid_info = kzalloc(sizeof(struct carl9170_sta_tid), > GFP_ATOMIC); > if (!tid_info) { > sparse [0] hit a bug when testing the patch: drivers/net/wireless/ath/carl9170/main.c:1440:17: warning: context imbalance in 'carl9170_op_ampdu_action' - unexpected unlock This warning is caused by the remaining, stray rcu_read protection in the code below (Sorry! Guess RCU needed a bit more explanation in the previous post. If you are looking for *pointers*, there are excellent resources available in Documentation/RCU/ [1]). I've attached a full patch (see below) with all changes so far and tested if the device/driver still behaves ;-). This patch applies cleanly on top of wireless-testing. @John, can you please take it? --- From: Andreea-Cristina Bernat The carl9170_op_ampdu_action() function is used only by the mac80211 framework. Since the mac80211 already takes care of checks and properly serializing calls to the driver's function there is no need for the driver to do the same thing. Signed-off-by: Andreea-Cristina Bernat [chunkeey@googlemail.com: remove two stray rcu_read_unlock()] Signed-off-by: Christian Lamparter --- --- Regards Christian [0] [1] -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index f8ded84..ef5b6dc 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, if (!sta_info->ht_sta) return -EOPNOTSUPP; - rcu_read_lock(); - if (rcu_dereference(sta_info->agg[tid])) { - rcu_read_unlock(); - return -EBUSY; - } - tid_info = kzalloc(sizeof(struct carl9170_sta_tid), GFP_ATOMIC); - if (!tid_info) { - rcu_read_unlock(); + if (!tid_info) return -ENOMEM; - } tid_info->hsn = tid_info->bsn = tid_info->snx = (*ssn); tid_info->state = CARL9170_TID_STATE_PROGRESS; @@ -1460,7 +1452,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, list_add_tail_rcu(&tid_info->list, &ar->tx_ampdu_list); rcu_assign_pointer(sta_info->agg[tid], tid_info); spin_unlock_bh(&ar->tx_ampdu_list_lock); - rcu_read_unlock(); ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); break;