Re: [PATCH] ath6kl: sdio: fix system panic when doing wifi stress test

[Kalle Valo <kvalo@qca.qualcomm.com>]

Hi Jason,

Jason Liu <r64343@freescale.com> writes:

> When did the wifi iperf test, meet one following kernel panic:
> command: iperf -c $TARGET_IP -i 5 -t 50 -w 1M
>
> Unable to handle kernel paging request at virtual address 1a480000
> pgd = 80004000
> [1a480000] *pgd=00000000
> Internal error: Oops: 805 [#1] SMP ARM

[...]

> The kernel panic is caused by the sg_buf is not set correctly with the
> following code when compiled with Yocto GCC 4.8.1:
>
> drivers/net/wireless/ath/ath6kl/hif.h:
> struct hif_scatter_req {
>         struct list_head list;
>         /* address for the read/write operation */
>         u32 addr;
> 	...
>
>         /* bounce buffer for upper layers to copy to/from */
>         u8 *virt_dma_buf;
>
>         struct hif_scatter_item scat_list[1];
>
>         u32 scat_q_depth;
> };
>
> (Note: the scat_req.scat_list[] will dynamiclly grow with run-time)

There's actually a major bug right there, scat_list can corrupt scat_q_depth.

> The GCC 4.8.1 compiler will not do the for-loop till scat_entries, instead,
> it only run one round loop. This may be caused by that the GCC 4.8.1 thought
> that the scat_list only have one item and then no need to do full iteration,
> but this is simply wrong by looking at the assebly code. This will cause the
> sg buffer not get set when scat_entries > 1 and thus lead to kernel panic.
>
> This patch is a workaround to the GCC 4.8.1 complier issue by passing the
> entry address of the scat_req->scat_list to the for-loop and interate it,
> then, GCC 4.8.1 will do the full for-loop correctly.
> (Note: This issue not observed with GCC 4.7.2, only found on the GCC 4.8.1)
>
> This patch does not change any function logic and no any performance downgrade.

[...]

> +	scat_list = &scat_req->scat_list[0];
> +
>  	/* assemble SG list */
> -	for (i = 0; i < scat_req->scat_entries; i++, sg++) {
> +	for (i = 0; i < scat_req->scat_entries; i++, sg++, scat_list++) {
>  		ath6kl_dbg(ATH6KL_DBG_SCATTER, "%d: addr:0x%p, len:%d\n",
> -			   i, scat_req->scat_list[i].buf,
> -			   scat_req->scat_list[i].len);
> +			   i, scat_list->buf, scat_list->len);
>  
> -		sg_set_buf(sg, scat_req->scat_list[i].buf,
> -			   scat_req->scat_list[i].len);
> +		sg_set_buf(sg, scat_list->buf, scat_list->len);
>  	}

Working around the problem by adding a temporary variable makes me a bit
worried, I would rather fix the root cause. Is the root cause by that we
define the field with scat_list[1]?

Does the patch below help? It would also fix the corruption with
scat_q_depth. Please note that I have only compile tested it. And I
might have also missed something important, so please review it
carefully.

--- a/drivers/net/wireless/ath/ath6kl/hif.h
+++ b/drivers/net/wireless/ath/ath6kl/hif.h
@@ -197,9 +197,9 @@ struct hif_scatter_req {
 	/* bounce buffer for upper layers to copy to/from */
 	u8 *virt_dma_buf;
 
-	struct hif_scatter_item scat_list[1];
-
 	u32 scat_q_depth;
+
+	struct hif_scatter_item scat_list[0];
 };
 
 struct ath6kl_irq_proc_registers {
diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 7126bdd..6bf15a3 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -348,7 +348,7 @@ static int ath6kl_sdio_alloc_prep_scat_req(struct ath6kl_sdio *ar_sdio,
 	int i, scat_req_sz, scat_list_sz, size;
 	u8 *virt_buf;
 
-	scat_list_sz = (n_scat_entry - 1) * sizeof(struct hif_scatter_item);
+	scat_list_sz = n_scat_entry * sizeof(struct hif_scatter_item);
 	scat_req_sz = sizeof(*s_req) + scat_list_sz;
 
 	if (!virt_scat)


-- 
Kalle Valo