lkml.org 
[lkml]   [2013]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] ath6kl: sdio: fix system panic when doing wifi stress test
Date
> -----Original Message-----
> From: Kalle Valo [mailto:kvalo@qca.qualcomm.com]
> Sent: Tuesday, November 26, 2013 6:40 PM
> To: Liu Hui-R64343
> Cc: linux-arm-kernel@lists.infradead.org; linville@tuxdriver.com; linux-
> wireless@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; ath6kl-devel@qca.qualcomm.com
> Subject: Re: [PATCH] ath6kl: sdio: fix system panic when doing wifi
> stress test
>
> 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]?

Yes, this is what I assumed.

>
> 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.

Yes, Firstly, I have looked at the asm code and the compiler(gcc 4.8.1) works correctly after applying
the following patch. Secondly, I have tested the patch with compiler(gcc 4.8.1) on the real HW, and it
works fine too. Without the patch, the kernel crash will happen 100%.

Thus, for the patch:

Acked-by: Jason Liu <r64343@freescale.com>
Tested-by: Jason Liu <r64343@freescale.com>

Jason Liu

>
> --- 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




\
 
 \ /
  Last update: 2013-11-29 07:21    [W:6.687 / U:0.644 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site