From patchwork Wed Jul 6 10:40:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luca Coelho X-Patchwork-Id: 9216125 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4F23F60752 for ; Wed, 6 Jul 2016 10:44:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3FD9D28819 for ; Wed, 6 Jul 2016 10:44:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3471B2881B; Wed, 6 Jul 2016 10:44:58 +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=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9A0B228819 for ; Wed, 6 Jul 2016 10:44:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753771AbcGFKoz (ORCPT ); Wed, 6 Jul 2016 06:44:55 -0400 Received: from paleale.coelho.fi ([176.9.41.70]:33476 "EHLO farmhouse.coelho.fi" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753746AbcGFKoy (ORCPT ); Wed, 6 Jul 2016 06:44:54 -0400 Received: from a88-113-65-232.elisa-laajakaista.fi ([88.113.65.232] helo=dubbel.ger.corp.intel.com) by farmhouse.coelho.fi with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1bKkHD-0002BB-Dx; Wed, 06 Jul 2016 13:41:56 +0300 From: Luca Coelho To: linux-wireless@vger.kernel.org Cc: Emmanuel Grumbach , "[4.5+]" , Luca Coelho Date: Wed, 6 Jul 2016 13:40:43 +0300 Message-Id: <1467801651-1816-48-git-send-email-luca@coelho.fi> X-Mailer: git-send-email 2.8.1 In-Reply-To: <1467801452.25088.20.camel@coelho.fi> References: <1467801452.25088.20.camel@coelho.fi> X-SA-Exim-Connect-IP: 88.113.65.232 X-SA-Exim-Mail-From: luca@coelho.fi Subject: [PATCH 48/56] iwlwifi: pcie: fix a race in firmware loading flow X-SA-Exim-Version: 4.2.1 (built Mon, 06 Jul 2015 07:28:29 +0000) X-SA-Exim-Scanned: Yes (on farmhouse.coelho.fi) Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Emmanuel Grumbach Upon firmware load interrupt (FH_TX), the ISR re-enables the firmware load interrupt only to avoid races with other flows as described in the commit below. When the firmware is completely loaded, the thread that is loading the firmware will enable all the interrupts to make sure that the driver gets the ALIVE interrupt. The problem with that is that the thread that is loading the firmware is actually racing against the ISR and we can get to the following situation: CPU0 CPU1 iwl_pcie_load_given_ucode ... iwl_pcie_load_firmware_chunk wait_for_interrupt ISR handles CSR_INT_BIT_FH_TX ISR wakes up the thread on CPU0 /* enable all the interrupts * to get the ALIVE interrupt */ iwl_enable_interrupts ISR re-enables CSR_INT_BIT_FH_TX only /* start the firmware */ iwl_write32(trans, CSR_RESET, 0); BUG! ALIVE interrupt will never arrive since it has been masked by CPU1. In order to fix that, change the ISR to first check if STATUS_INT_ENABLED is set. If so, re-enable all the interrupts. If STATUS_INT_ENABLED is clear, then we can check what specific interrupt happened and re-enable only that specific interrupt (RFKILL or FH_TX). All the credit for the analysis goes to Kirtika who did the actual debugging work. Cc: [4.5+] Fixes: a6bd005fe92 ("iwlwifi: pcie: fix RF-Kill vs. firmware load race") Signed-off-by: Luca Coelho --- drivers/net/wireless/intel/iwlwifi/pcie/internal.h | 21 +++++++++++++++++++-- drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 16 +++++++++------- drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 8 -------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h index f684b9d..54af3da 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h +++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h @@ -500,7 +500,7 @@ void iwl_pcie_dump_csr(struct iwl_trans *trans); /***************************************************** * Helpers ******************************************************/ -static inline void iwl_disable_interrupts(struct iwl_trans *trans) +static inline void _iwl_disable_interrupts(struct iwl_trans *trans) { struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans); @@ -523,7 +523,16 @@ static inline void iwl_disable_interrupts(struct iwl_trans *trans) IWL_DEBUG_ISR(trans, "Disabled interrupts\n"); } -static inline void iwl_enable_interrupts(struct iwl_trans *trans) +static inline void iwl_disable_interrupts(struct iwl_trans *trans) +{ + struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans); + + spin_lock(&trans_pcie->irq_lock); + _iwl_disable_interrupts(trans); + spin_unlock(&trans_pcie->irq_lock); +} + +static inline void _iwl_enable_interrupts(struct iwl_trans *trans) { struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans); @@ -546,6 +555,14 @@ static inline void iwl_enable_interrupts(struct iwl_trans *trans) } } +static inline void iwl_enable_interrupts(struct iwl_trans *trans) +{ + struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans); + + spin_lock(&trans_pcie->irq_lock); + _iwl_enable_interrupts(trans); + spin_unlock(&trans_pcie->irq_lock); +} static inline void iwl_enable_hw_int_msk_msix(struct iwl_trans *trans, u32 msk) { struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans); diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c index 0296c29..45f1b7e 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c @@ -1535,7 +1535,7 @@ irqreturn_t iwl_pcie_irq_handler(int irq, void *dev_id) * have anything to service */ if (test_bit(STATUS_INT_ENABLED, &trans->status)) - iwl_enable_interrupts(trans); + _iwl_enable_interrupts(trans); spin_unlock(&trans_pcie->irq_lock); lock_map_release(&trans->sync_cmd_lockdep_map); return IRQ_NONE; @@ -1727,15 +1727,17 @@ irqreturn_t iwl_pcie_irq_handler(int irq, void *dev_id) inta & ~trans_pcie->inta_mask); } + spin_lock(&trans_pcie->irq_lock); + /* only Re-enable all interrupt if disabled by irq */ + if (test_bit(STATUS_INT_ENABLED, &trans->status)) + _iwl_enable_interrupts(trans); /* we are loading the firmware, enable FH_TX interrupt only */ - if (handled & CSR_INT_BIT_FH_TX) + else if (handled & CSR_INT_BIT_FH_TX) iwl_enable_fw_load_int(trans); - /* only Re-enable all interrupt if disabled by irq */ - else if (test_bit(STATUS_INT_ENABLED, &trans->status)) - iwl_enable_interrupts(trans); /* Re-enable RF_KILL if it occurred */ else if (handled & CSR_INT_BIT_RF_KILL) iwl_enable_rfkill_int(trans); + spin_unlock(&trans_pcie->irq_lock); out: lock_map_release(&trans->sync_cmd_lockdep_map); @@ -1799,7 +1801,7 @@ void iwl_pcie_reset_ict(struct iwl_trans *trans) return; spin_lock(&trans_pcie->irq_lock); - iwl_disable_interrupts(trans); + _iwl_disable_interrupts(trans); memset(trans_pcie->ict_tbl, 0, ICT_SIZE); @@ -1815,7 +1817,7 @@ void iwl_pcie_reset_ict(struct iwl_trans *trans) trans_pcie->use_ict = true; trans_pcie->ict_index = 0; iwl_write32(trans, CSR_INT, trans_pcie->inta_mask); - iwl_enable_interrupts(trans); + _iwl_enable_interrupts(trans); spin_unlock(&trans_pcie->irq_lock); } diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c index 3b7a414..9e953a4 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c @@ -1037,9 +1037,7 @@ static void _iwl_trans_pcie_stop_device(struct iwl_trans *trans, bool low_power) was_hw_rfkill = iwl_is_rfkill_set(trans); /* tell the device to stop sending interrupts */ - spin_lock(&trans_pcie->irq_lock); iwl_disable_interrupts(trans); - spin_unlock(&trans_pcie->irq_lock); /* device going down, Stop using ICT table */ iwl_pcie_disable_ict(trans); @@ -1083,9 +1081,7 @@ static void _iwl_trans_pcie_stop_device(struct iwl_trans *trans, bool low_power) * the time, unless the interrupt is ACKed even if the interrupt * should be masked. Re-ACK all the interrupts here. */ - spin_lock(&trans_pcie->irq_lock); iwl_disable_interrupts(trans); - spin_unlock(&trans_pcie->irq_lock); /* clear all status bits */ clear_bit(STATUS_SYNC_HCMD_ACTIVE, &trans->status); @@ -1578,15 +1574,11 @@ static void iwl_trans_pcie_op_mode_leave(struct iwl_trans *trans) mutex_lock(&trans_pcie->mutex); /* disable interrupts - don't enable HW RF kill interrupt */ - spin_lock(&trans_pcie->irq_lock); iwl_disable_interrupts(trans); - spin_unlock(&trans_pcie->irq_lock); iwl_pcie_apm_stop(trans, true); - spin_lock(&trans_pcie->irq_lock); iwl_disable_interrupts(trans); - spin_unlock(&trans_pcie->irq_lock); iwl_pcie_disable_ict(trans);