From patchwork Thu Feb 7 01:41:43 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Norris X-Patchwork-Id: 10800345 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 19A8913B5 for ; Thu, 7 Feb 2019 01:41:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 094C92D49B for ; Thu, 7 Feb 2019 01:41:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F10D92D4B7; Thu, 7 Feb 2019 01:41:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6F2552D49B for ; Thu, 7 Feb 2019 01:41:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-Id:Date:Subject:To :From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=2oj/dCta1X+qDGQDsWR1G6oZEVjZlHMKSfT3g5sinhs=; b=BgFFTsaXvMGRTG eD/69TCQ3k4v2Afpew6/3HHwrkp9fV8Ca7JgrUMl2P9FiA0htR/CRzgYZjDUlj4xEFNmmnRarnK7w T+esh2Wm3SeVLAFrpkfAXjgU9CLt9nGr11D1H8pfv2RJWoRuoW6e3eSv5bu28K9KYWWn3yMrOwqbR 5AOPUMWAis6pl7Pi0Tb2Mwq7DYPN0IWXcbih5gVkggx8+/+VHMElfV97nNVngd4FCfr/1TjQML2DM Yk8ZpSd3l4f8niufM3/b72xBdZaKLqOZlCJABIETehFKcBD+Tf2ZLEZ9uoqQJ3P72WfenVmwjQFr2 zHNilMungqHei4Q0H8eg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1grYhO-0002Tg-TH; Thu, 07 Feb 2019 01:41:54 +0000 Received: from mail-pf1-x442.google.com ([2607:f8b0:4864:20::442]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1grYhL-0002TF-EG for ath10k@lists.infradead.org; Thu, 07 Feb 2019 01:41:53 +0000 Received: by mail-pf1-x442.google.com with SMTP id u6so4007065pfh.11 for ; Wed, 06 Feb 2019 17:41:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=AH/cPCNiW2VWKL5BecCmjq4BeZ2v1KefStsAqZ/Gd0U=; b=MtPf3lF4+DkY74Y7AzdGz7fIQ791q9WmA1p9KQepcICaqMu3tHc81jxFerWp3+qDNm RczNZK01yDe/Ffq14hXp6HnY1q2RPYKh1PBdIy+q8lfNmYWFBt/fE2niNERE5aXPVlBm 40bgcXli2+hxdM/0zBvPrjYRq/FenhljBYANI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=AH/cPCNiW2VWKL5BecCmjq4BeZ2v1KefStsAqZ/Gd0U=; b=kK0qKVj3kDUhyIDIXvk/jXrrqyBQkHaWQBHusDcu4oFXkFJbNL4F9DksrGJFUhEolb EdzqsMjkkWuKzcj64lu9N0IDa9aGFbon7IyHgXrA3UfQVdURCqWrGk/yC+zYly2krJpo k0WvJGYy8DSl1WADDdYZDZSkytR0ToAvVpeGXM8zT/sqxi62byqqovLGxF0acWhtyfPO L5SZwulYNULQfl3ojCSm6nDOhZ9Cn+4QOof6CRQjtn0zcOMDp777wStbbgpWEo0YIn/R a/n+dYUiWD1hcM9MJdxhEKe0P+aDmxfu70LCJBkGne2xD/UiQn60IWXxZ6gHKjJxl78N tqSg== X-Gm-Message-State: AHQUAub6ASxwqrR8DM+PlswPWCV2UXKnjEd091Y9fCsHboNLMlHsNtnt 3WPd9+BQbfjcHfkk855FLM1uCA== X-Google-Smtp-Source: AHgI3IZONWRhLbhL04+se2hDX/fga7cvaujTSBExDoZJN1MZWjkl4g81TydybcZOVFFa0e6S0fLLIw== X-Received: by 2002:a63:a30a:: with SMTP id s10mr11812991pge.234.1549503709800; Wed, 06 Feb 2019 17:41:49 -0800 (PST) Received: from smtp.gmail.com ([2620:15c:202:1:534:b7c0:a63c:460c]) by smtp.gmail.com with ESMTPSA id 128sm12826922pfu.129.2019.02.06.17.41.48 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 06 Feb 2019 17:41:48 -0800 (PST) From: Brian Norris To: linux-wireless@vger.kernel.org Subject: [PATCH] ath10k: pci: use mutex for diagnostic window CE polling Date: Wed, 6 Feb 2019 17:41:43 -0800 Message-Id: <20190207014143.41529-1-briannorris@chromium.org> X-Mailer: git-send-email 2.20.1.611.gfbb209baf1-goog MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190206_174151_503759_95814718 X-CRM114-Status: GOOD ( 15.11 ) X-BeenThere: ath10k@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Brian Norris , Carl Huang , ath10k@lists.infradead.org, Wen Gong Sender: "ath10k" Errors-To: ath10k-bounces+patchwork-ath10k=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP The DIAG copy engine is only used via polling, but it holds a spinlock with softirqs disabled. Each iteration of our read/write loops can theoretically take 20ms (two 10ms timeout loops), and this loop can be run an unbounded number of times while holding the spinlock -- dependent on the request size given by the caller. As of commit 39501ea64116 ("ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377."), we transfer large chunks of firmware memory using this mechanism. With large enough firmware segments, this becomes an exceedingly long period for disabling soft IRQs. For example, with a 500KiB firmware segment, in testing QCA6174A, I see 200 loop iterations of about 50-100us each, which can total about 10-20ms. In reality, we don't really need to block softirqs for this duration. The DIAG CE is only used in polling mode, and we only need to hold ce_lock to make sure any CE bookkeeping is done without screwing up another CE. Otherwise, we only need to ensure exclusion between ath10k_pci_diag_{read,write}_mem() contexts. This patch moves to use fine-grained locking for the shared ce_lock, while adding a new mutex just to ensure mutual exclusion of diag read/write operations. Tested on QCA6174A, firmware version WLAN.RM.4.4.1-00132-QCARMSWPZ-1. Fixes: 39501ea64116 ("ath10k: download firmware via diag Copy Engine for QCA6174 and QCA9377.") Signed-off-by: Brian Norris --- I'm not quite sure if this is -stable material. It's technically an existing bug, but it looks like previously, the DIAG interface was only exposed via debugfs -- you could block softirqs by playing with /sys/kernel/debug/ieee80211/phyX/ath10k/mem_value. The new usage (for loading firmware, on QCA6174 and QCA9377) might constitute a significant regression. Also, I'd appreciate some review from Qualcomm folks as always. --- drivers/net/wireless/ath/ath10k/pci.c | 41 ++++++++++----------------- drivers/net/wireless/ath/ath10k/pci.h | 3 ++ 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index 39e0b1cc2a12..f8356b3bf150 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -913,7 +913,6 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data, int nbytes) { struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); - struct ath10k_ce *ce = ath10k_ce_priv(ar); int ret = 0; u32 *buf; unsigned int completed_nbytes, alloc_nbytes, remaining_bytes; @@ -924,8 +923,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data, void *data_buf = NULL; int i; - spin_lock_bh(&ce->ce_lock); - + mutex_lock(&ar_pci->ce_diag_mutex); ce_diag = ar_pci->ce_diag; /* @@ -960,19 +958,17 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data, nbytes = min_t(unsigned int, remaining_bytes, DIAG_TRANSFER_LIMIT); - ret = ce_diag->ops->ce_rx_post_buf(ce_diag, &ce_data, ce_data); + ret = ath10k_ce_rx_post_buf(ce_diag, &ce_data, ce_data); if (ret != 0) goto done; /* Request CE to send from Target(!) address to Host buffer */ - ret = ath10k_ce_send_nolock(ce_diag, NULL, (u32)address, nbytes, 0, - 0); + ret = ath10k_ce_send(ce_diag, NULL, (u32)address, nbytes, 0, 0); if (ret) goto done; i = 0; - while (ath10k_ce_completed_send_next_nolock(ce_diag, - NULL) != 0) { + while (ath10k_ce_completed_send_next(ce_diag, NULL) != 0) { udelay(DIAG_ACCESS_CE_WAIT_US); i += DIAG_ACCESS_CE_WAIT_US; @@ -983,10 +979,8 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data, } i = 0; - while (ath10k_ce_completed_recv_next_nolock(ce_diag, - (void **)&buf, - &completed_nbytes) - != 0) { + while (ath10k_ce_completed_recv_next(ce_diag, (void **)&buf, + &completed_nbytes) != 0) { udelay(DIAG_ACCESS_CE_WAIT_US); i += DIAG_ACCESS_CE_WAIT_US; @@ -1019,7 +1013,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data, dma_free_coherent(ar->dev, alloc_nbytes, data_buf, ce_data_base); - spin_unlock_bh(&ce->ce_lock); + mutex_unlock(&ar_pci->ce_diag_mutex); return ret; } @@ -1067,7 +1061,6 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address, const void *data, int nbytes) { struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); - struct ath10k_ce *ce = ath10k_ce_priv(ar); int ret = 0; u32 *buf; unsigned int completed_nbytes, alloc_nbytes, remaining_bytes; @@ -1076,8 +1069,7 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address, dma_addr_t ce_data_base = 0; int i; - spin_lock_bh(&ce->ce_lock); - + mutex_lock(&ar_pci->ce_diag_mutex); ce_diag = ar_pci->ce_diag; /* @@ -1118,7 +1110,7 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address, memcpy(data_buf, data, nbytes); /* Set up to receive directly into Target(!) address */ - ret = ce_diag->ops->ce_rx_post_buf(ce_diag, &address, address); + ret = ath10k_ce_rx_post_buf(ce_diag, &address, address); if (ret != 0) goto done; @@ -1126,14 +1118,12 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address, * Request CE to send caller-supplied data that * was copied to bounce buffer to Target(!) address. */ - ret = ath10k_ce_send_nolock(ce_diag, NULL, ce_data_base, - nbytes, 0, 0); + ret = ath10k_ce_send(ce_diag, NULL, ce_data_base, nbytes, 0, 0); if (ret != 0) goto done; i = 0; - while (ath10k_ce_completed_send_next_nolock(ce_diag, - NULL) != 0) { + while (ath10k_ce_completed_send_next(ce_diag, NULL) != 0) { udelay(DIAG_ACCESS_CE_WAIT_US); i += DIAG_ACCESS_CE_WAIT_US; @@ -1144,10 +1134,8 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address, } i = 0; - while (ath10k_ce_completed_recv_next_nolock(ce_diag, - (void **)&buf, - &completed_nbytes) - != 0) { + while (ath10k_ce_completed_recv_next(ce_diag, (void **)&buf, + &completed_nbytes) != 0) { udelay(DIAG_ACCESS_CE_WAIT_US); i += DIAG_ACCESS_CE_WAIT_US; @@ -1182,7 +1170,7 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address, ath10k_warn(ar, "failed to write diag value at 0x%x: %d\n", address, ret); - spin_unlock_bh(&ce->ce_lock); + mutex_unlock(&ar_pci->ce_diag_mutex); return ret; } @@ -3462,6 +3450,7 @@ int ath10k_pci_setup_resource(struct ath10k *ar) spin_lock_init(&ce->ce_lock); spin_lock_init(&ar_pci->ps_lock); + mutex_init(&ar_pci->ce_diag_mutex); timer_setup(&ar_pci->rx_post_retry, ath10k_pci_rx_replenish_retry, 0); diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h index e8d86331c539..a9270fa6463c 100644 --- a/drivers/net/wireless/ath/ath10k/pci.h +++ b/drivers/net/wireless/ath/ath10k/pci.h @@ -19,6 +19,7 @@ #define _PCI_H_ #include +#include #include "hw.h" #include "ce.h" @@ -128,6 +129,8 @@ struct ath10k_pci { /* Copy Engine used for Diagnostic Accesses */ struct ath10k_ce_pipe *ce_diag; + /* For protecting ce_diag */ + struct mutex ce_diag_mutex; struct ath10k_ce ce; struct timer_list rx_post_retry;