From patchwork Mon Oct 5 08:01:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hante Meuleman X-Patchwork-Id: 7325221 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 7E8329F1D5 for ; Mon, 5 Oct 2015 08:02:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8FB48206D0 for ; Mon, 5 Oct 2015 08:02:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 905FF206EB for ; Mon, 5 Oct 2015 08:02:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752135AbbJEIBt (ORCPT ); Mon, 5 Oct 2015 04:01:49 -0400 Received: from mail-gw1-out.broadcom.com ([216.31.210.62]:22331 "EHLO mail-gw1-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751517AbbJEIBr convert rfc822-to-8bit (ORCPT ); Mon, 5 Oct 2015 04:01:47 -0400 X-IronPort-AV: E=Sophos;i="5.17,637,1437462000"; d="scan'208";a="76825587" Received: from irvexchcas06.broadcom.com (HELO IRVEXCHCAS06.corp.ad.broadcom.com) ([10.9.208.53]) by mail-gw1-out.broadcom.com with ESMTP; 05 Oct 2015 02:50:10 -0700 Received: from SJEXCHCAS06.corp.ad.broadcom.com (10.16.203.14) by IRVEXCHCAS06.corp.ad.broadcom.com (10.9.208.53) with Microsoft SMTP Server (TLS) id 14.3.235.1; Mon, 5 Oct 2015 01:01:47 -0700 Received: from SJEXCHMB15.corp.ad.broadcom.com ([fe80::ade7:c955:4805:f845]) by SJEXCHCAS06.corp.ad.broadcom.com ([::1]) with mapi id 14.03.0235.001; Mon, 5 Oct 2015 01:01:46 -0700 From: Hante Meuleman To: Nicholas Krause , Brett Rudley CC: Arend Van Spriel , "Franky (Zhenhui) Lin" , "kvalo@codeaurora.org" , Pieter-Paul Giesberts , "linux-wireless@vger.kernel.org" , brcm80211-dev-list , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCHv2] brcm80211:Fix locking region in the function brcmf_sdio_sendfromq Thread-Topic: [PATCHv2] brcm80211:Fix locking region in the function brcmf_sdio_sendfromq Thread-Index: AQHQ/vn8ezR6UKnciU2IGDSFiUUHbZ5ch6Tw Date: Mon, 5 Oct 2015 08:01:45 +0000 Message-ID: References: <1444000277-25694-1-git-send-email-xerofoify@gmail.com> In-Reply-To: <1444000277-25694-1-git-send-email-xerofoify@gmail.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.16.203.100] MIME-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hello Nicholas, I can understand that if you read the comment that there is supposed to be a lock taken, but why would you assume that it is the txq_lock? Two things: 1) The comment that a look should have been taken is wrong. Once upon a time a packet could be transmitted/handled from two paths. To protect for concurrency the callee of brcmf_sdio_txpkt was assumed to have take some sort of lock to protect against this. 2) Nowadays all frames are put on the txq and then pulled from that queue in the DPC. This queue is protected with a lock and therefor a lock is taken around the dequeueing of txq in dpc. Also the reason for the chosen var name txq_lock. This lock protects for concurrent access to the txq and the lock should be taken as short as possible, and therefor this patch could possible result in throughput degradation and does not solve anything. Please do not submit this patch, if anything then remove the comment from the function brcmf_sdio_txpkt. Regards, Hante -----Original Message----- From: Nicholas Krause [mailto:xerofoify@gmail.com] Sent: Monday, October 05, 2015 1:11 AM To: Brett Rudley Cc: Arend Van Spriel; Franky (Zhenhui) Lin; Hante Meuleman; kvalo@codeaurora.org; Pieter-Paul Giesberts; linux-wireless@vger.kernel.org; brcm80211-dev-list; netdev@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [PATCHv2] brcm80211:Fix locking region in the function brcmf_sdio_sendfromq This fixes the locking region in the function brcmf_sdio_sendfromq to properly lock around the call to the function brcmrf_sdio_txpkt in order to avoid concurrent access issues when calling this function as stated in the function's comments about the caller being required to lock around the call to this particular function. Signed-off-by: Nicholas Krause --- v2 Fix issue with deadlock occurrence due to not unlocking before break in for loop if the variable i is equal to zero drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c index f990e3d..260f063 100644 --- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c @@ -2388,11 +2388,13 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sdio *bus, uint maxframes) break; __skb_queue_tail(&pktq, pkt); } - spin_unlock_bh(&bus->txq_lock); - if (i == 0) + if (i == 0) { + spin_unlock_bh(&bus->txq_lock); break; + } ret = brcmf_sdio_txpkt(bus, &pktq, SDPCM_DATA_CHANNEL); + spin_unlock_bh(&bus->txq_lock); cnt += i;