[PATCH 1/6] ath10k: sdio: workaround firmware UART pin configuration bug

[PATCH 1/6] ath10k: sdio: workaround firmware UART pin configuration bug

[Kalle Valo]

From: Wen Gong <wgong@codeaurora.org>

On QCA6174 SDIO devices the SDIO interrupt will fail if UART is
disabled from ath10k. SDIO firmware enables UART printouts by
default. If ath10k will try to enable UART again the firmware
will configure it's GPIO line incorrectly and SDIO interrupts
won't work anymore. The workaround is to set UART pin again (19
for QCA6174 SDIO) if uart_print is 0.

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00007-QCARMSWP-1.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/core.c |   10 +++++++++-
 drivers/net/wireless/ath/ath10k/hw.h   |    5 +++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 835b8de92d55..cfd7bb29a1ec 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -2065,8 +2065,16 @@ static int ath10k_init_uart(struct ath10k *ar)
 		return ret;
 	}
 
-	if (!uart_print)
+	if (!uart_print && ar->hw_params.uart_pin_workaround) {
+		ret = ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
+					 ar->hw_params.uart_pin);
+		if (ret) {
+			ath10k_warn(ar, "failed to set UART TX pin: %d", ret);
+			return ret;
+		}
+
 		return 0;
+	}
 
 	ret = ath10k_bmi_write32(ar, hi_dbg_uart_txpin, ar->hw_params.uart_pin);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 71314999aa24..a92fce1bb6f0 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -606,6 +606,11 @@ struct ath10k_hw_params {
 
 	/* target supporting fw download via diag ce */
 	bool fw_diag_ce_download;
+
+	/* need to set uart pin if disable uart print, workaround for a
+	 * firmware bug
+	 */
+	bool uart_pin_workaround;
 };
 
 struct htt_rx_desc;

[PATCH 2/6] ath10k: don't disable interrupts in ath10k_sdio_remove()

[Kalle Valo]

From: Wen Gong <wgong@codeaurora.org>

Disabling interrupts this early meant WMI communication was not working anymore
when the SDIO device was removed. But we call ath10k_core_unregister() that
will eventually call ath10k_sdio_hif_stop(), which disables the interrupts. So
there's actually no need to disable interrupts here.

Also remove cancel_work_sync() as it's also called in ath10k_sdio_hif_stop().

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00007-QCARMSWP-1.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/sdio.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index fae56c67766f..d573d24624c0 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2073,8 +2073,6 @@ static void ath10k_sdio_remove(struct sdio_func *func)
 		   "sdio removed func %d vendor 0x%x device 0x%x\n",
 		   func->num, func->vendor, func->device);
 
-	(void)ath10k_sdio_hif_disable_intrs(ar);
-	cancel_work_sync(&ar_sdio->wr_async_work);
 	ath10k_core_unregister(ar);
 	ath10k_core_destroy(ar);
 }

[PATCH 3/6] ath10k: htt: don't use txdone_fifo with SDIO

[Kalle Valo]

From: Alagu Sankar <alagusankar@silex-india.com>

HTT High Latency (ATH10K_DEV_TYPE_HL) does not use txdone_fifo at all, we don't
even initialise it by skipping ath10k_htt_tx_alloc_buf() in
ath10k_htt_tx_start(). Because of this using QCA6174 SDIO
ath10k_htt_rx_tx_compl_ind() will crash when it accesses unitialised
txdone_fifo. So skip txdone_fifo when using High Latency mode.

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00007-QCARMSWP-1.

Co-developed-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Alagu Sankar <alagusankar@silex-india.com>
Signed-off-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a20ea270d519..8d13d0c3e85e 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -2277,7 +2277,9 @@ static void ath10k_htt_rx_tx_compl_ind(struct ath10k *ar,
 		 *  Note that with only one concurrent reader and one concurrent
 		 *  writer, you don't need extra locking to use these macro.
 		 */
-		if (!kfifo_put(&htt->txdone_fifo, tx_done)) {
+		if (ar->bus_param.dev_type == ATH10K_DEV_TYPE_HL) {
+			ath10k_txrx_tx_unref(htt, &tx_done);
+		} else if (!kfifo_put(&htt->txdone_fifo, tx_done)) {
 			ath10k_warn(ar, "txdone fifo overrun, msdu_id %d status %d\n",
 				    tx_done.msdu_id, tx_done.status);
 			ath10k_txrx_tx_unref(htt, &tx_done);

[PATCH 4/6] ath10k: initialise struct ath10k_bus params to zero

[Kalle Valo]

This way we don't need to set every variable and give them to default, which is
zero. This is also safer in case we forgot to initalise a new field in some of
the bus modules.

Compile tested only.

Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/ahb.c  |    2 +-
 drivers/net/wireless/ath/ath10k/pci.c  |    2 +-
 drivers/net/wireless/ath/ath10k/sdio.c |    2 +-
 drivers/net/wireless/ath/ath10k/snoc.c |    2 +-
 drivers/net/wireless/ath/ath10k/usb.c  |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index 0bf726c55736..f80854180e21 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -740,7 +740,7 @@ static int ath10k_ahb_probe(struct platform_device *pdev)
 	enum ath10k_hw_rev hw_rev;
 	size_t size;
 	int ret;
-	struct ath10k_bus_params bus_params;
+	struct ath10k_bus_params bus_params = {};
 
 	of_id = of_match_device(ath10k_ahb_of_match, &pdev->dev);
 	if (!of_id) {
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 271f92c24d44..13c24cd977d2 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -3478,7 +3478,7 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
 	struct ath10k *ar;
 	struct ath10k_pci *ar_pci;
 	enum ath10k_hw_rev hw_rev;
-	struct ath10k_bus_params bus_params;
+	struct ath10k_bus_params bus_params = {};
 	bool pci_ps;
 	int (*pci_soft_reset)(struct ath10k *ar);
 	int (*pci_hard_reset)(struct ath10k *ar);
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index d573d24624c0..bbfdc4d7cf5f 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1954,7 +1954,7 @@ static int ath10k_sdio_probe(struct sdio_func *func,
 	struct ath10k *ar;
 	enum ath10k_hw_rev hw_rev;
 	u32 dev_id_base;
-	struct ath10k_bus_params bus_params;
+	struct ath10k_bus_params bus_params = {};
 	int ret, i;
 
 	/* Assumption: All SDIO based chipsets (so far) are QCA6174 based.
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 873cb4ce419b..845914f2eaf8 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1249,7 +1249,7 @@ static int ath10k_snoc_resource_init(struct ath10k *ar)
 int ath10k_snoc_fw_indication(struct ath10k *ar, u64 type)
 {
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
-	struct ath10k_bus_params bus_params;
+	struct ath10k_bus_params bus_params = {};
 	int ret;
 
 	if (test_bit(ATH10K_SNOC_FLAG_UNREGISTERING, &ar_snoc->flags))
diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
index 970cf69ac35f..2fb0fb40e86d 100644
--- a/drivers/net/wireless/ath/ath10k/usb.c
+++ b/drivers/net/wireless/ath/ath10k/usb.c
@@ -973,7 +973,7 @@ static int ath10k_usb_probe(struct usb_interface *interface,
 	struct usb_device *dev = interface_to_usbdev(interface);
 	int ret, vendor_id, product_id;
 	enum ath10k_hw_rev hw_rev;
-	struct ath10k_bus_params bus_params;
+	struct ath10k_bus_params bus_params = {};
 
 	/* Assumption: All USB based chipsets (so far) are QCA9377 based.
 	 * If there will be newer chipsets that does not use the hw reg

[PATCH 5/6] ath10k: htt: support MSDU ids with SDIO

[Kalle Valo]

From: Alagu Sankar <alagusankar@silex-india.com>

Transmit completion for SDIO is similar to PCIe, modify the high
latency path to allow SDIO modules to use the msdu id.

kvalo: the original patch from Alagu enabled this only for SDIO but I'm not
sure should we also enable this with USB. I'll use bus params to enable this
for so that it's easy to enable also for USB later.

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00007-QCARMSWP-1.

Co-developed-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Alagu Sankar <alagusankar@silex-india.com>
Signed-off-by: Wen Gong <wgong@codeaurora.org>.
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/core.h   |    1 +
 drivers/net/wireless/ath/ath10k/htt_tx.c |   13 ++++++++++++-
 drivers/net/wireless/ath/ath10k/sdio.c   |    2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index e08a17b01e03..f22cb3f49034 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -919,6 +919,7 @@ struct ath10k_bus_params {
 	u32 chip_id;
 	enum ath10k_dev_type dev_type;
 	bool link_can_suspend;
+	bool hl_msdu_ids;
 };
 
 struct ath10k {
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index d8e9cc0bb772..88f880b601d8 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -1244,6 +1244,7 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
 	u8 tid = ath10k_htt_tx_get_tid(msdu, is_eth);
 	u8 flags0 = 0;
 	u16 flags1 = 0;
+	u16 msdu_id = 0;
 
 	data_len = msdu->len;
 
@@ -1291,6 +1292,16 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
 		}
 	}
 
+	if (ar->bus_param.hl_msdu_ids) {
+		flags1 |= HTT_DATA_TX_DESC_FLAGS1_POSTPONED;
+		res = ath10k_htt_tx_alloc_msdu_id(htt, msdu);
+		if (res < 0) {
+			ath10k_err(ar, "msdu_id allocation failed %d\n", res);
+			goto out;
+		}
+		msdu_id = res;
+	}
+
 	skb_push(msdu, sizeof(*cmd_hdr));
 	skb_push(msdu, sizeof(*tx_desc));
 	cmd_hdr = (struct htt_cmd_hdr *)msdu->data;
@@ -1300,7 +1311,7 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
 	tx_desc->flags0 = flags0;
 	tx_desc->flags1 = __cpu_to_le16(flags1);
 	tx_desc->len = __cpu_to_le16(data_len);
-	tx_desc->id = 0;
+	tx_desc->id = __cpu_to_le16(msdu_id);
 	tx_desc->frags_paddr = 0; /* always zero */
 	/* Initialize peer_id to INVALID_PEER because this is NOT
 	 * Reinjection path
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index bbfdc4d7cf5f..d5073fac9509 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2045,6 +2045,8 @@ static int ath10k_sdio_probe(struct sdio_func *func,
 	bus_params.dev_type = ATH10K_DEV_TYPE_HL;
 	/* TODO: don't know yet how to get chip_id with SDIO */
 	bus_params.chip_id = 0;
+	bus_params.hl_msdu_ids = true;
+
 	ret = ath10k_core_register(ar, &bus_params);
 	if (ret) {
 		ath10k_err(ar, "failed to register driver core: %d\n", ret);

[PATCH 6/6] ath10k: fix use-after-free on SDIO data frames

[Kalle Valo]

With SDIO there's a use after free after a data frame is transfered, call stack
below. This happens because ath10k_htt_tx_hl() directly transmits the skb
provided by mac80211 using ath10k_htc_send(), all other HTT functions use
separate skb created with ath10k_htc_alloc_skb() to transmit the HTC packet.
After the packet is transmitted mac80211 frees the skb in ieee80211_tx_status()
but HTT layer expects  that it still owns the skb, and frees it in
ath10k_htt_htc_tx_complete().

To fix this take a reference of skb before sending it to HTC layer to make sure
we still own the skb.

Tested on QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00007-QCARMSWP-1.
ath10k_htt_tx_hl() is only used by SDIO and USB so other busses (PCI, AHB and
SNOC) should be unaffected.

call stack of use-after-free:
dump_backtrace+0x0/0x2d8
show_stack+0x20/0x2c
__dump_stack+0x20/0x28
dump_stack+0xc8/0xec
print_address_description+0x74/0x240
kasan_report+0x258/0x274
__asan_report_load4_noabort+0x20/0x28
skb_pull+0xbc/0x114
ath10k_htc_notify_tx_completion+0x190/0x2a4 [ath10k_core]
ath10k_sdio_write_async_work+0x1e4/0x2c4 [ath10k_sdio]
process_one_work+0x3d8/0x8b0
worker_thread+0x518/0x7e0
kthread+0x260/0x278
ret_from_fork+0x10/0x18

Allocated by one task:
kasan_kmalloc+0xa0/0x13c
kasan_slab_alloc+0x14/0x1c
kmem_cache_alloc+0x144/0x208
__alloc_skb+0xec/0x394
alloc_skb_with_frags+0x8c/0x374
sock_alloc_send_pskb+0x520/0x5d4
sock_alloc_send_skb+0x40/0x50
__ip_append_data+0xf5c/0x1858
ip_make_skb+0x194/0x1d4
udp_sendmsg+0xf24/0x1ab8
inet_sendmsg+0x1b0/0x2e0
sock_sendmsg+0x88/0xa0
__sys_sendto+0x220/0x3a8
__arm64_sys_sendto+0x78/0x80
el0_svc_common+0x120/0x1e0
el0_svc_compat_handler+0x64/0x80
el0_svc_compat+0x8/0x18

Freed by another task:
__kasan_slab_free+0x120/0x1d4
kasan_slab_free+0x10/0x1c
kmem_cache_free+0x74/0x504
kfree_skbmem+0x88/0xc8
__kfree_skb+0x24/0x2c
consume_skb+0x114/0x18c
__ieee80211_tx_status+0xb7c/0xf60 [mac80211]
ieee80211_tx_status+0x224/0x270 [mac80211]
ath10k_txrx_tx_unref+0x564/0x950 [ath10k_core]
ath10k_htt_t2h_msg_handler+0x178c/0x2a38 [ath10k_core]
ath10k_htt_htc_t2h_msg_handler+0x20/0x30 [ath10k_core]
ath10k_sdio_irq_handler+0xcc0/0x1654 [ath10k_sdio]
process_sdio_pending_irqs+0xec/0x358
sdio_run_irqs+0x68/0xe4
sdio_irq_work+0x1c/0x28
process_one_work+0x3d8/0x8b0
worker_thread+0x518/0x7e0
kthread+0x260/0x278
ret_from_fork+0x10/0x18

Reported-by: Wen Gong <wgong@codeaurora.org>
Tested-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/htt_tx.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 88f880b601d8..78f07b7a3b21 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -1302,6 +1302,13 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
 		msdu_id = res;
 	}
 
+	/* As msdu is freed by mac80211 (in ieee80211_tx_status()) and by
+	 * ath10k (in ath10k_htt_htc_tx_complete()) we have to increase
+	 * reference by one to avoid a use-after-free case and a double
+	 * free.
+	 */
+	skb_get(msdu);
+
 	skb_push(msdu, sizeof(*cmd_hdr));
 	skb_push(msdu, sizeof(*tx_desc));
 	cmd_hdr = (struct htt_cmd_hdr *)msdu->data;

Re: [PATCH 1/6] ath10k: sdio: workaround firmware UART pin configuration bug

[Kalle Valo]

Kalle Valo <kvalo@codeaurora.org> wrote:

> On QCA6174 SDIO devices the SDIO interrupt will fail if UART is
> disabled from ath10k. SDIO firmware enables UART printouts by
> default. If ath10k will try to enable UART again the firmware
> will configure it's GPIO line incorrectly and SDIO interrupts
> won't work anymore. The workaround is to set UART pin again (19
> for QCA6174 SDIO) if uart_print is 0.
> 
> Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00007-QCARMSWP-1.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

6 patches applied to ath-next branch of ath.git, thanks.

4504f0e5b571 ath10k: sdio: workaround firmware UART pin configuration bug
70736b9753fd ath10k: don't disable interrupts in ath10k_sdio_remove()
e2a6b711282a ath10k: htt: don't use txdone_fifo with SDIO
6d084ac27ab4 ath10k: initialise struct ath10k_bus params to zero
8ea51e409eb0 ath10k: htt: support MSDU ids with SDIO
30382dd1cf3a ath10k: fix use-after-free on SDIO data frames

-- 
https://patchwork.kernel.org/patch/10898285/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches