Hi Niklas,
(CC Laurent)
On Wed, Apr 5, 2017 at 11:14 AM, Niklas Söderlund
<niklas.soderlund@xxxxxxxxxxxx> wrote:
> On 2017-04-05 08:55:31 +0530, Vinod Koul wrote:
>> On Thu, Mar 30, 2017 at 09:38:39AM +0200, Niklas Söderlund wrote:
>> > On 2017-03-29 15:30:42 +0200, Niklas Söderlund wrote:
>> > > On 2017-03-29 14:31:33 +0200, Geert Uytterhoeven wrote:
>> > > > On Wed, Mar 29, 2017 at 12:40 AM, Niklas Söderlund
>> > > > <niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
>> > > > > This fixes a race condition where the channel resources could be freed
>> > > > > before the ISR had finished running resulting in a NULL pointer
>> > > > > reference from the ISR.
>> > > > >
>> > > > > [ 167.148934] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> > > > > [ 167.157051] pgd = ffff80003c641000
>> > > > > [ 167.160449] [00000000] *pgd=000000007c507003, *pud=000000007c4ff003, *pmd=0000000000000000
>> > > > > [ 167.168719] Internal error: Oops: 96000046 [#1] PREEMPT SMP
>> > > > > [ 167.174289] Modules linked in:
>> > > > > [ 167.177348] CPU: 3 PID: 10547 Comm: dma_ioctl Not tainted 4.11.0-rc1-00001-g8d92afddc2f6633a #73
>> > > > > [ 167.186131] Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
>> > > > > [ 167.192917] task: ffff80003a411a00 task.stack: ffff80003bcd4000
>> > > > > [ 167.198850] PC is at rcar_dmac_chan_prep_sg+0xe0/0x400
>> > > > > [ 167.203985] LR is at rcar_dmac_chan_prep_sg+0x48/0x400
>> > > >
>> > > > Do you have a test case to trigger this?
>> > >
>> > > Yes I have a testcase, it's rather complex and involves both a kernel
>> > > module and a userspaces application to stress the rcar-dmac. I'm
>> > > checking if I can share this publicly or not, please hold :-)
>> >
>> > I have now received feedback that I'm unfortunately not allowed to share
>> > the test case :-(
>> >
>> > The big picture in how to trigger this problem is that you start a DMA
>> > transfer like this:
>> >
>> > struct dma_async_tx_descriptor *tx = ...;
>> >
>> > ...
>> >
>> > tx->tx_submit(tx);
>> >
>> > And then you directly call dma_release_channel() on this channel without
>> > making sure the completion callback ran or anything. Now if you are
>> > unlucky the ISR have not finished running for the DMA when
>> > dma_release_channel() starts to clean up resources. The synchronisation
>> > point in the dma_release_channel() call path fixes this.
>>
>> Well the API expectation would be you abort the txn before calling release.
>> So the expected order should be:
>>
>> dmaengine_terminate_all();
>> dma_release_channel();
>
> Agree this is the correct way and in this case patch 3/3 in this series
> could be dropped. Then device_synchronize() would added to rcar-dmac,
> dmaengine_terminate_all() would turn of the IRQ and
> dma_release_channel() would ensure that device_synchronize() is called
> prior to calling rcar-dmac device_free_chan_resources().
>
>>
>> Terminate should then stop the channel, ie abort the pending descriptors..
>>
>
> However for reasons unknown to me the rcar-dmac
> device_free_chan_resources() implementation implements logic to turn of
> IRQs before it frees the resources. And it's because of this patch 3/3
> is needed so that it can be sure no ISR is running before it frees
> resources.
>
> I don't know how to best proceed here. I agree it feels a bit odd that
> device_free_chan_resources() is dealing with the IRQs as such things
> should be done before it's called. But on the other hand that code has
> been part of the driver since it was added upstream. I feel a bit
> uncomfortable just removing that part from the
> device_free_chan_resources() since the driver have been in use with it
> for such a long time.
>
> How would you prefer I try and resolve this?
Perhaps Laurent knows why it was implemented this way?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
