From: Ard Biesheuvel <ard.biesheuvel@linaro.org> To: Robin Murphy <robin.murphy@arm.com> Cc: Shawn Lin <shawn.lin@rock-chips.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Mark Rutland <mark.rutland@arm.com> Subject: Re: [RFC] Kernel panic down to swiotlb when doing insmod a simple driver Date: Fri, 13 Jan 2017 11:25:52 +0000 [thread overview] Message-ID: <CAKv+Gu-aaGuHmshi=aiexUD=dxuia7tHKm3LQt3RG-7D_3v9Mw@mail.gmail.com> (raw) In-Reply-To: <99472068-069a-7759-8d6e-019fd875264d@arm.com> On 13 January 2017 at 11:03, Robin Murphy <robin.murphy@arm.com> wrote: > On 13/01/17 10:00, Shawn Lin wrote: >> Hi, >> >> Sorry for sending this RFC for help as I couldn't find some useful hint >> to slove my issue by git-log the swiotlb commit from kernel v4.4 to >> v4.9 and I'm also not familar with these stuff. So could you kindly >> point me to the right direction to debug it? Thanks. :) >> >> -------------------------------------- >> We just have a very simple wifi driver *built as ko module* which only >> have a probe function to do the basic init work and call SDIO API to >> transfer some bytes. >> >> Env: kernel 4.4 stable tree, ARM64(rk3399) >> >> Two cases are included: > > And they are both wrong :) > >> The crash case: >> >> u8 __aligned(32) buf[PAGE_SIZE]; //global here in ko driver file > > It is only valid to do DMA from linear map addresses - I'm not sure if > the modules area was in the linear map before, but either way it > probably isn't now (Ard, Mark?). Either way, I don't believe static data > honours ARCH_DMA_MINALIGN in general, so it's still highly inadvisable. > The __aligned() modifier should work fine: the alignment is propagated to the ELF section alignment, which in turn is honoured by the module loader. The problem is that '32' is too low for non-coherent DMA to be safe. In general, alignments up to 4 KB should work everywhere. I am surprised though that this ever worked as a module, given that modules are (and have always been) loaded in the vmalloc area, which means VA to PA translations performed in the DMA layer on the addresses of statically allocated buffers are unlikely to return correct values (as your panic log proves) >> static int wifi_probe(struct sdio_func *func, const struct >> sdio_device_id *id) >> { >> // prepare some SDIO work before >> printk("wifi_probe: buf = 0x%x\n", buf); >> sdio_memcpy_toio(func, 0, buf, 200); >> } >> >> The workable case: >> >> static int wifi_probe(struct sdio_func *func, const struct >> sdio_device_id *id) >> { >> >> u8 __aligned(32) buf[PAGE_SIZE]; //move inside the probe function > > No. DMA from the stack is right out, both for the aforementioned > alignment reasons, and the fact that we now have (or will have) > virtually-mapped stacks. One of the benefits of the latter is that it > catches bugs like this ;) > Actually, aligned stack variables also work fine. But DMA involving the stack is not, so that is not really relevant. > Get your buffer from kmalloc() or a page allocation, and everything > should be correct. > Agreed.
WARNING: multiple messages have this Message-ID (diff)
From: ard.biesheuvel@linaro.org (Ard Biesheuvel) To: linux-arm-kernel@lists.infradead.org Subject: [RFC] Kernel panic down to swiotlb when doing insmod a simple driver Date: Fri, 13 Jan 2017 11:25:52 +0000 [thread overview] Message-ID: <CAKv+Gu-aaGuHmshi=aiexUD=dxuia7tHKm3LQt3RG-7D_3v9Mw@mail.gmail.com> (raw) In-Reply-To: <99472068-069a-7759-8d6e-019fd875264d@arm.com> On 13 January 2017 at 11:03, Robin Murphy <robin.murphy@arm.com> wrote: > On 13/01/17 10:00, Shawn Lin wrote: >> Hi, >> >> Sorry for sending this RFC for help as I couldn't find some useful hint >> to slove my issue by git-log the swiotlb commit from kernel v4.4 to >> v4.9 and I'm also not familar with these stuff. So could you kindly >> point me to the right direction to debug it? Thanks. :) >> >> -------------------------------------- >> We just have a very simple wifi driver *built as ko module* which only >> have a probe function to do the basic init work and call SDIO API to >> transfer some bytes. >> >> Env: kernel 4.4 stable tree, ARM64(rk3399) >> >> Two cases are included: > > And they are both wrong :) > >> The crash case: >> >> u8 __aligned(32) buf[PAGE_SIZE]; //global here in ko driver file > > It is only valid to do DMA from linear map addresses - I'm not sure if > the modules area was in the linear map before, but either way it > probably isn't now (Ard, Mark?). Either way, I don't believe static data > honours ARCH_DMA_MINALIGN in general, so it's still highly inadvisable. > The __aligned() modifier should work fine: the alignment is propagated to the ELF section alignment, which in turn is honoured by the module loader. The problem is that '32' is too low for non-coherent DMA to be safe. In general, alignments up to 4 KB should work everywhere. I am surprised though that this ever worked as a module, given that modules are (and have always been) loaded in the vmalloc area, which means VA to PA translations performed in the DMA layer on the addresses of statically allocated buffers are unlikely to return correct values (as your panic log proves) >> static int wifi_probe(struct sdio_func *func, const struct >> sdio_device_id *id) >> { >> // prepare some SDIO work before >> printk("wifi_probe: buf = 0x%x\n", buf); >> sdio_memcpy_toio(func, 0, buf, 200); >> } >> >> The workable case: >> >> static int wifi_probe(struct sdio_func *func, const struct >> sdio_device_id *id) >> { >> >> u8 __aligned(32) buf[PAGE_SIZE]; //move inside the probe function > > No. DMA from the stack is right out, both for the aforementioned > alignment reasons, and the fact that we now have (or will have) > virtually-mapped stacks. One of the benefits of the latter is that it > catches bugs like this ;) > Actually, aligned stack variables also work fine. But DMA involving the stack is not, so that is not really relevant. > Get your buffer from kmalloc() or a page allocation, and everything > should be correct. > Agreed.
next prev parent reply other threads:[~2017-01-13 11:25 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-13 10:00 [RFC] Kernel panic down to swiotlb when doing insmod a simple driver Shawn Lin 2017-01-13 10:00 ` Shawn Lin 2017-01-13 11:03 ` Robin Murphy 2017-01-13 11:03 ` Robin Murphy 2017-01-13 11:25 ` Ard Biesheuvel [this message] 2017-01-13 11:25 ` Ard Biesheuvel 2017-01-13 11:47 ` Robin Murphy 2017-01-13 11:47 ` Robin Murphy 2017-01-13 11:49 ` Ard Biesheuvel 2017-01-13 11:49 ` Ard Biesheuvel 2017-01-13 11:52 ` Robin Murphy 2017-01-13 11:52 ` Robin Murphy 2017-01-13 11:54 ` Ard Biesheuvel 2017-01-13 11:54 ` Ard Biesheuvel 2017-01-13 12:05 ` Robin Murphy 2017-01-13 12:05 ` Robin Murphy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKv+Gu-aaGuHmshi=aiexUD=dxuia7tHKm3LQt3RG-7D_3v9Mw@mail.gmail.com' \ --to=ard.biesheuvel@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robin.murphy@arm.com \ --cc=shawn.lin@rock-chips.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.