From patchwork Thu Aug 10 16:29:39 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Drake X-Patchwork-Id: 800269 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=endlessm-com.20150623.gappssmtp.com header.i=@endlessm-com.20150623.gappssmtp.com header.b="LUoWT18a"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3xStsC1ZXXz9rxj for ; Fri, 11 Aug 2017 02:30:07 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753217AbdHJQaF (ORCPT ); Thu, 10 Aug 2017 12:30:05 -0400 Received: from mail-wm0-f52.google.com ([74.125.82.52]:37805 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752859AbdHJQaD (ORCPT ); Thu, 10 Aug 2017 12:30:03 -0400 Received: by mail-wm0-f52.google.com with SMTP id i66so27151451wmg.0 for ; Thu, 10 Aug 2017 09:30:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=endlessm-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=Bsddly5cjpKd6BySHB7uBt+iarWx+nXfwwi4h5PH59E=; b=LUoWT18adSZitbo4rwJI+3uY2x/99IPcLoF9XyU1G2XIjfcUvWHcqsXjLLxgY65hKN nP2AsV8ANXeIXQHQ7uf/eB2y6hNjDm3zbQ0TMYx4kMnnZqAypaMvY3zJzLNa6NZhlFW3 gtA1SaPpa9n0gijje30iNYzzmIg7mGNc8LD3atgKkZyd2DXqaHKfAkt04MHWgdnLVHxh af/3+Fm0/2h6dbfo/3+VMBt53eRf6GHSBpC2y86znHbk5uo+Zd/ODZLC/O4X07YG99QH XErKWDD5UUvv8Cq42JNMwLRQQIFnufAYx1OkiOwSciISbKYBlSpbumGqy1vUN8i2VpP+ dPcw== 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=Bsddly5cjpKd6BySHB7uBt+iarWx+nXfwwi4h5PH59E=; b=htjDTi3+gBQOkMDTaeXdal0zUM/nl5RGaN0uzkyUAoDZhNficPxqwyRfepiMKlfcbh kN0Yhd9+OUE8g5nzJUDfODCHvt4ZNXmzhmqsM8nv7uWOj0JgCsUbifZioELNE5MSo2SL jkd6snl2vnywySjkULDqGfQsI2Y/3e4qM9s+gXaUQg/5PK5+X8GlElBjwrF/XQUY0k7G psLUN6gtmbHhZnSDBc2Zo8oYGrPu2AhIf9jjXMT3cfu9YT/cIlFkqBsm7nEbtIx8/Zbq PwR3dCy610b92W/Ypj3XkAGS0uEDtFvlKn3bPf/AsAwJ+vWF+D53A5g91wrxVPbOaBIf GY0g== X-Gm-Message-State: AHYfb5guG7AZPKsyeRV73tZ9oEsDC8/qNdEUCxLmmjYccoZ7kl9LYXq8 pziiTBFaxSCzJuEVjb4= X-Received: by 10.28.57.4 with SMTP id g4mr7463222wma.170.1502382601457; Thu, 10 Aug 2017 09:30:01 -0700 (PDT) Received: from localhost.localdomain (host86-187-2-178.range86-187.btcentralplus.com. [86.187.2.178]) by smtp.gmail.com with ESMTPSA id 76sm6308749wmm.39.2017.08.10.09.29.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Aug 2017 09:30:00 -0700 (PDT) From: Daniel Drake To: linux-wireless@vger.kernel.org, ath9k-devel@qca.qualcomm.com, linux-pci@vger.kernel.org Cc: linux@endlessm.com, chiu@endlessm.com Subject: ath9k hardware corrupts MSI Message Data, raises wrong interrupt Date: Thu, 10 Aug 2017 17:29:39 +0100 Message-Id: <20170810162939.9880-1-drake@endlessm.com> X-Mailer: git-send-email 2.11.0 MIME-Version: 1.0 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Hi, The ath9k wireless driver in mainline currently does not have support for PCI MSI interrupts, it uses legacy interrupts instead. However we are working with a number of 3rd party laptop models based on Intel Apollo Lake which will soon be available on the consumer market. They all appear to have broken legacy interrupt wiring for the wifi card. Unfortunately the hardware can't be changed so we are instead looking at making ath9k use MSI interrupts which is what we believe they are doing on Windows. To recap what MSI is: The host OS can configure a Message Address value and a Message Data value within the device's PCI configuration space. When the device wishes to interrupt the host, instead of pulsing a logic level on the legacy interrupt pin, it will instead write the value of Message Data into the address specified in Message Address. This write will then trigger interrupt handling mechanisms within the kernel. The code below can be used to tell the ath9k hardware to use MSI interrupts instead of legacy interrupts (sorry that it's a bit unclean). However, it is not working, as reproduced on multiple devices. No interrupts are counted against the ath9k MSI IRQ, and we get messages like these spammed in the kernel logs: do_IRQ: 0.64 No irq handler for vector The device does not appear to be MSI-X capable. Configuration dump for the device at this point: 02:00.0 Network controller: Qualcomm Atheros QCA9565 / AR9565 Wireless Network Adapter (rev 01) Subsystem: AzureWave Device 218d Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR- 64, 99 -> 96). Why might it be doing this? My guess, looking at the PCI specs: The Multiple Message Enable field (bits 6-4 of the Message Control register) defines the number of low order message data bits the function is permitted to modify to generate its system software allocated vectors. For example, a Multiple Message Enable encoding of “010” indicates the function has been allocated four vectors and is permitted to modify message data bits 1 and 0 (a function modifies the lower message data bits to generate the allocated number of vectors). If the Multiple Message Enable field is “000”, the function is not permitted to modify the message data. Linux is not working with Multiple Messages and has written the 000 value as described. However, I suspect the device is not fully following the spec here and is effectively taking ownership of the 2 lower bits, and setting them to 00 to indicate that it is working with the first of the 4 possible MSI IRQ vectors. I thought about modifying Linux's vector-assignment algorithm to consider this special case and only assign a single vector number with the low bits already set as 00, but that seems like a hairy topic and that code is distanced from the driver too. The algorithm in question is __assign_irq_vector() in arch/x86/kernel/apic/msi.c Similarly the idea of adding support for MSI_FLAG_MULTI_PCI_MSI to the PCI-MSI adapter would encounter similar challenges where ultimately we'd need to allocate 4 contiguous vectors and that is not really in agreement with the design of __assign_irq_vector(). I'd appreciate any suggestions for next steps here. Do any ath9k developers have datasheet or vendor contacts that might shine light on the behaviour I suspect here where the Message Data bits are being incorrectly zeroed out? Any PCI experts that have any bright ideas for how we could introduce a workaround for this possibly broken hardware in upstreamable form? Thanks Daniel --- drivers/net/wireless/ath/ath9k/hw.c | 34 ++++++++++++++++++++++------ drivers/net/wireless/ath/ath9k/hw.h | 3 +++ drivers/net/wireless/ath/ath9k/init.c | 4 ++++ drivers/net/wireless/ath/ath9k/mac.c | 42 +++++++++++++++++++++++++++++++++++ drivers/net/wireless/ath/ath9k/pci.c | 20 ++++++++++++++++- drivers/net/wireless/ath/ath9k/reg.h | 15 +++++++++++++ 6 files changed, 110 insertions(+), 8 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c index 8c5c2dd8fa7f..8c25d14cd9fc 100644 --- a/drivers/net/wireless/ath/ath9k/hw.c +++ b/drivers/net/wireless/ath/ath9k/hw.c @@ -922,6 +922,7 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah, AR_IMR_RXERR | AR_IMR_RXORN | AR_IMR_BCNMISC; + u32 msi_cfg = 0; if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah)) @@ -929,22 +930,33 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah, if (AR_SREV_9300_20_OR_LATER(ah)) { imr_reg |= AR_IMR_RXOK_HP; - if (ah->config.rx_intr_mitigation) + if (ah->config.rx_intr_mitigation) { imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR; - else + msi_cfg |= AR_INTCFG_MSI_RXINTM | AR_INTCFG_MSI_RXMINTR; + } + else { imr_reg |= AR_IMR_RXOK_LP; - + msi_cfg |= AR_INTCFG_MSI_RXOK ; + } } else { - if (ah->config.rx_intr_mitigation) + if (ah->config.rx_intr_mitigation) { imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR; - else + msi_cfg |= AR_INTCFG_MSI_RXINTM | AR_INTCFG_MSI_RXMINTR; + } + else { imr_reg |= AR_IMR_RXOK; + msi_cfg |= AR_INTCFG_MSI_RXOK ; + } } - if (ah->config.tx_intr_mitigation) + if (ah->config.tx_intr_mitigation) { imr_reg |= AR_IMR_TXINTM | AR_IMR_TXMINTR; - else + msi_cfg |= AR_INTCFG_MSI_TXINTM | AR_INTCFG_MSI_TXMINTR ; + } + else { imr_reg |= AR_IMR_TXOK; + msi_cfg |= AR_INTCFG_MSI_TXOK; + } ENABLE_REGWRITE_BUFFER(ah); @@ -952,6 +964,14 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah, ah->imrs2_reg |= AR_IMR_S2_GTT; REG_WRITE(ah, AR_IMR_S2, ah->imrs2_reg); + if(ah->msi_enabled) { + ah->msi_reg = REG_READ(ah, AR_PCIE_MSI); + ah->msi_reg |= AR_PCIE_MSI_HW_DBI_WR_EN ; + ah->msi_reg &= AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64; + REG_WRITE(ah, AR_INTCFG, msi_cfg); + ath_info(ath9k_hw_common(ah), "value of AR_INTCFG=0x%X, msi_cfg=0x%X\n", REG_READ(ah, AR_INTCFG), msi_cfg); + } + if (!AR_SREV_9100(ah)) { REG_WRITE(ah, AR_INTR_SYNC_CAUSE, 0xFFFFFFFF); REG_WRITE(ah, AR_INTR_SYNC_ENABLE, sync_default); diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h index 9cbca1229bac..7f9215976d8f 100644 --- a/drivers/net/wireless/ath/ath9k/hw.h +++ b/drivers/net/wireless/ath/ath9k/hw.h @@ -976,6 +976,9 @@ struct ath_hw { bool tpc_enabled; u8 tx_power[Ar5416RateSize]; u8 tx_power_stbc[Ar5416RateSize]; + bool msi_enabled; + u32 msi_mask; + u32 msi_reg; }; struct ath_bus_ops { diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c index fa4b3cc1ba22..1d205d961fce 100644 --- a/drivers/net/wireless/ath/ath9k/init.c +++ b/drivers/net/wireless/ath/ath9k/init.c @@ -75,6 +75,10 @@ MODULE_PARM_DESC(use_chanctx, "Enable channel context for concurrency"); #endif /* CONFIG_ATH9K_CHANNEL_CONTEXT */ +int ath9k_use_msi = 1 ; +module_param_named(use_msi, ath9k_use_msi, int, 0444); +MODULE_PARM_DESC(use_msi, "Use MSI instead of INTx if possible"); + bool is_ath9k_unloaded; #ifdef CONFIG_MAC80211_LEDS diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c index d937c39b3a0b..045581ede475 100644 --- a/drivers/net/wireless/ath/ath9k/mac.c +++ b/drivers/net/wireless/ath/ath9k/mac.c @@ -831,6 +831,38 @@ static void __ath9k_hw_enable_interrupts(struct ath_hw *ah) } ath_dbg(common, INTERRUPT, "AR_IMR 0x%x IER 0x%x\n", REG_READ(ah, AR_IMR), REG_READ(ah, AR_IER)); + + if(ah->msi_enabled) { + u32 _msi_reg = 0; + u32 i=0; + u32 msi_pend_addr_mask = AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64; + + ath_info(ath9k_hw_common(ah), "Enabling MSI, msi_mask=0x%X\n", ah->msi_mask); + + REG_WRITE(ah, AR_INTR_PRIO_ASYNC_ENABLE, ah->msi_mask); + REG_WRITE(ah, AR_INTR_PRIO_ASYNC_MASK, ah->msi_mask); + ath_info(ath9k_hw_common(ah), + "AR_INTR_PRIO_ASYNC_ENABLE=0x%X, AR_INTR_PRIO_ASYNC_MASK=0x%X\n", + REG_READ(ah, AR_INTR_PRIO_ASYNC_ENABLE), + REG_READ(ah, AR_INTR_PRIO_ASYNC_MASK)); + + if (ah->msi_reg == 0) + ah->msi_reg = REG_READ(ah, AR_PCIE_MSI); + + ath_info(ath9k_hw_common(ah), "AR_PCIE_MSI=0x%X, ah->msi_reg = 0x%X\n", AR_PCIE_MSI, ah->msi_reg); + + i=0; + do { + REG_WRITE(ah, AR_PCIE_MSI, (ah->msi_reg | AR_PCIE_MSI_ENABLE) & msi_pend_addr_mask); + _msi_reg = REG_READ(ah, AR_PCIE_MSI); + i++; + } while((_msi_reg & AR_PCIE_MSI_ENABLE) == 0 && i < 200); + + if(i >= 200) + ath_err(ath9k_hw_common(ah), + "%s: _msi_reg = 0x%X\n", + __func__, _msi_reg); + } } void ath9k_hw_resume_interrupts(struct ath_hw *ah) @@ -877,12 +909,21 @@ void ath9k_hw_set_interrupts(struct ath_hw *ah) if (!(ints & ATH9K_INT_GLOBAL)) ath9k_hw_disable_interrupts(ah); + if(ah->msi_enabled) { + ath_info(common, "Clearing AR_INTR_PRIO_ASYNC_ENABLE\n"); + + REG_WRITE(ah, AR_INTR_PRIO_ASYNC_ENABLE, 0); + REG_READ(ah, AR_INTR_PRIO_ASYNC_ENABLE); + } + ath_dbg(common, INTERRUPT, "New interrupt mask 0x%x\n", ints); mask = ints & ATH9K_INT_COMMON; mask2 = 0; + ah->msi_mask =0; if (ints & ATH9K_INT_TX) { + ah->msi_mask |= AR_INTR_PRIO_TX; if (ah->config.tx_intr_mitigation) mask |= AR_IMR_TXMINTR | AR_IMR_TXINTM; else { @@ -897,6 +938,7 @@ void ath9k_hw_set_interrupts(struct ath_hw *ah) mask |= AR_IMR_TXEOL; } if (ints & ATH9K_INT_RX) { + ah->msi_mask |= AR_INTR_PRIO_RXLP | AR_INTR_PRIO_RXHP; if (AR_SREV_9300_20_OR_LATER(ah)) { mask |= AR_IMR_RXERR | AR_IMR_RXOK_HP; if (ah->config.rx_intr_mitigation) { diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c index aff473dfa10d..94cf34a62578 100644 --- a/drivers/net/wireless/ath/ath9k/pci.c +++ b/drivers/net/wireless/ath/ath9k/pci.c @@ -22,6 +22,8 @@ #include #include "ath9k.h" +extern int ath9k_use_msi ; + static const struct pci_device_id ath_pci_id_table[] = { { PCI_VDEVICE(ATHEROS, 0x0023) }, /* PCI */ { PCI_VDEVICE(ATHEROS, 0x0024) }, /* PCI-E */ @@ -879,6 +881,7 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) u32 val; int ret = 0; char hw_name[64]; + int msi_enabled = 0; if (pcim_enable_device(pdev)) return -EIO; @@ -950,7 +953,19 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) sc->mem = pcim_iomap_table(pdev)[0]; sc->driver_data = id->driver_data; - ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc); + if(ath9k_use_msi) { + if(pci_enable_msi(pdev) == 0) { + msi_enabled = 1; + dev_err(&pdev->dev, "Using MSI\n"); + } + else dev_err(&pdev->dev, "Using INTx\n"); + } + + if(!msi_enabled) + ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc); + else + ret = request_irq(pdev->irq, ath_isr, 0, "ath9k", sc); + if (ret) { dev_err(&pdev->dev, "request_irq failed\n"); goto err_irq; @@ -964,6 +979,9 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) goto err_init; } + sc->sc_ah->msi_enabled = msi_enabled ; + sc->sc_ah->msi_reg = 0 ; + ath9k_hw_name(sc->sc_ah, hw_name, sizeof(hw_name)); wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n", hw_name, (unsigned long)sc->mem, pdev->irq); diff --git a/drivers/net/wireless/ath/ath9k/reg.h b/drivers/net/wireless/ath/ath9k/reg.h index 80ff69f99229..dc54d62495de 100644 --- a/drivers/net/wireless/ath/ath9k/reg.h +++ b/drivers/net/wireless/ath/ath9k/reg.h @@ -146,6 +146,14 @@ #define AR_MACMISC_MISC_OBS_BUS_MSB_S 15 #define AR_MACMISC_MISC_OBS_BUS_1 1 +#define AR_INTCFG 0x005C +#define AR_INTCFG_MSI_RXOK 0x00000000 +#define AR_INTCFG_MSI_RXINTM 0x00000004 +#define AR_INTCFG_MSI_RXMINTR 0x00000006 +#define AR_INTCFG_MSI_TXOK 0x00000000 +#define AR_INTCFG_MSI_TXINTM 0x00000010 +#define AR_INTCFG_MSI_TXMINTR 0x00000018 + #define AR_DATABUF_SIZE 0x0060 #define AR_DATABUF_SIZE_MASK 0x00000FFF @@ -1256,6 +1264,13 @@ enum { #define AR_PCIE_MSI (AR_SREV_9340(ah) ? 0x40d8 : \ (AR_SREV_9300_20_OR_LATER(ah) ? 0x40a4 : 0x4094)) #define AR_PCIE_MSI_ENABLE 0x00000001 +#define AR_PCIE_MSI_HW_DBI_WR_EN 0x02000000 +#define AR_PCIE_MSI_HW_INT_PENDING_ADDR 0xFFA0C1FF // bits 8..11: value must be 0x5060 +#define AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64 0xFFA0C9FF // bits 8..11: value must be 0x5064 + +#define AR_INTR_PRIO_TX 0x00000001 +#define AR_INTR_PRIO_RXLP 0x00000002 +#define AR_INTR_PRIO_RXHP 0x00000004 #define AR_INTR_PRIO_SYNC_ENABLE (AR_SREV_9340(ah) ? 0x4088 : 0x40c4) #define AR_INTR_PRIO_ASYNC_MASK (AR_SREV_9340(ah) ? 0x408c : 0x40c8)