dmaengine pl08x dt crash

dmaengine pl08x dt crash

[Johannes Stezenbach]

Hi,

I'm trying to use the new devicetree support for pl08x
added in commit aa4734da667442
(dmaengine: pl08x: support dt channel assignment).

It causes crash in the pl011 serial driver when it calls
dma_chan_name() because the dma_chan->dev->device is NULL.
I think the reason is dma_async_device_register() is called
during pl08x_probe(), but at this time the number of slave
channels is still 0 as these get allocated later in
pl08x_of_xlate() when clients request the channels.

[    0.420835] pl08xdmac dma8ch0: initialized 8 virtual memcpy channels
[    0.427380] pl08xdmac dma8ch0: initialized 0 virtual slave channels
[    0.436185] pl08xdmac dma8ch0: DMA: PL080s rev1 at 0xfec00000 irq 8
...
[    1.268140] Unable to handle kernel NULL pointer dereference at virtual address 00000034
...
[    1.306397] PC is at pl011_dma_probe+0x114/0x31c
[    1.311018] LR is at pl011_dma_probe+0x104/0x31c
...
[    1.630388] Backtrace:
[    1.632856] [<c01f1784>] (pl011_dma_probe) from [<c01f2cd4>] (pl011_startup+0xd4/0x2b8)
[    1.640856]  r7:00000000 r6:c78b3610 r5:00000000 r4:c78b3610
[    1.646559] [<c01f2c00>] (pl011_startup) from [<c01ef5bc>] (uart_startup.part.3+0x84/0x164)
[    1.654905]  r8:00000000 r7:00000000 r6:c78b3610 r5:c7b09400 r4:c78172e0 r3:c01f2c00
[    1.662701] [<c01ef538>] (uart_startup.part.3) from [<c01ef7c4>] (uart_open+0x128/0x16c)
[    1.670787]  r8:c786da00 r7:c7817358 r6:00000000 r5:c7b09400 r4:c78172e0 r3:00000000
[    1.678583] [<c01ef69c>] (uart_open) from [<c01e32a0>] (tty_open+0x338/0x550)


Johannes

dmaengine pl08x dt crash

[Linus Walleij]

On Wed, Mar 23, 2016 at 4:40 PM, Johannes Stezenbach <js@sig21.net> wrote:

> I'm trying to use the new devicetree support for pl08x
> added in commit aa4734da667442
> (dmaengine: pl08x: support dt channel assignment).
>
> It causes crash in the pl011 serial driver when it calls
> dma_chan_name() because the dma_chan->dev->device is NULL.
> I think the reason is dma_async_device_register() is called
> during pl08x_probe(), but at this time the number of slave
> channels is still 0 as these get allocated later in
> pl08x_of_xlate() when clients request the channels.
>
> [    0.420835] pl08xdmac dma8ch0: initialized 8 virtual memcpy channels
> [    0.427380] pl08xdmac dma8ch0: initialized 0 virtual slave channels
> [    0.436185] pl08xdmac dma8ch0: DMA: PL080s rev1 at 0xfec00000 irq 8

So you are trying to use it on the Samsung S3C?

I have it working on the Nomadik, but for platforms with fixed signal
assignement I had to apply these patches:
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-nomadik.git/log/?h=pl08x-dma

dma: pl08x: support fixed signal assignment
dma: pl08x: allocate OF slave channel data at probe time

And the example:
ARM: nomadik: add DMA engine and some channelspl08x-dma

I have meant to follow up on it but haven't gotten around to.

If the Samsung also has fixed channels (no mux in front of the
DMA lines) I think this might be what you need. They are not
yet merged because Joachim reported a crash with the second
patch and I haven't yet figured out why.

Yours,
Linus Walleij

dmaengine pl08x dt crash

[Johannes Stezenbach]

On Wed, Mar 23, 2016 at 05:12:13PM +0100, Linus Walleij wrote:
> On Wed, Mar 23, 2016 at 4:40 PM, Johannes Stezenbach <js@sig21.net> wrote:
> 
> > I'm trying to use the new devicetree support for pl08x
> > added in commit aa4734da667442
> > (dmaengine: pl08x: support dt channel assignment).
> >
> > It causes crash in the pl011 serial driver when it calls
> > dma_chan_name() because the dma_chan->dev->device is NULL.
> > I think the reason is dma_async_device_register() is called
> > during pl08x_probe(), but at this time the number of slave
> > channels is still 0 as these get allocated later in
> > pl08x_of_xlate() when clients request the channels.
> >
> > [    0.420835] pl08xdmac dma8ch0: initialized 8 virtual memcpy channels
> > [    0.427380] pl08xdmac dma8ch0: initialized 0 virtual slave channels
> > [    0.436185] pl08xdmac dma8ch0: DMA: PL080s rev1 at 0xfec00000 irq 8
> 
> So you are trying to use it on the Samsung S3C?

The platform is an internal development board with ARM11 based SoC.

> I have it working on the Nomadik, but for platforms with fixed signal
> assignement I had to apply these patches:
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-nomadik.git/log/?h=pl08x-dma
> 
> dma: pl08x: support fixed signal assignment
> dma: pl08x: allocate OF slave channel data at probe time
> 
> And the example:
> ARM: nomadik: add DMA engine and some channelspl08x-dma
> 
> I have meant to follow up on it but haven't gotten around to.
> 
> If the Samsung also has fixed channels (no mux in front of the
> DMA lines) I think this might be what you need. They are not
> yet merged because Joachim reported a crash with the second
> patch and I haven't yet figured out why.

I'll give it a try.  My platform also has fixed channels,
previously it used platform data but I always thought it is
ugly, so I tried the new dt support.

Thanks for quick reply.


Johannes

dmaengine pl08x dt crash

[Johannes Stezenbach]

On Wed, Mar 23, 2016 at 05:23:09PM +0100, Johannes Stezenbach wrote:
> On Wed, Mar 23, 2016 at 05:12:13PM +0100, Linus Walleij wrote:
> > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-nomadik.git/log/?h=pl08x-dma
> > 
> > dma: pl08x: support fixed signal assignment

This method of using vendor_data for the fixed_signals flag
doesn't help me much, as my IP is a plain ARM pl081.

I'm also not sure how it would help, since pl08x_of_probe()
is only called for !pl08x->pd (no platform_data) and thus
pl08x_dma_init_virtual_channels() sees 0 channels.
And with platform_data the driver worked fine with fixed channels.

> > dma: pl08x: allocate OF slave channel data at probe time

This looks ore promising, I'm checking it.

> I'll give it a try.  My platform also has fixed channels,
> previously it used platform data but I always thought it is
> ugly, so I tried the new dt support.

Of course I neglected to mention I'm using linux-4.5,
in case you were wondering.


Johannes

dmaengine pl08x dt crash

[Johannes Stezenbach]

On Wed, Mar 23, 2016 at 05:35:17PM +0100, Johannes Stezenbach wrote:
> On Wed, Mar 23, 2016 at 05:23:09PM +0100, Johannes Stezenbach wrote:
> > On Wed, Mar 23, 2016 at 05:12:13PM +0100, Linus Walleij wrote:
> > > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-nomadik.git/log/?h=pl08x-dma
> > > 
> > > dma: pl08x: support fixed signal assignment
> 
> This method of using vendor_data for the fixed_signals flag
> doesn't help me much, as my IP is a plain ARM pl081.
> 
> I'm also not sure how it would help, since pl08x_of_probe()
> is only called for !pl08x->pd (no platform_data) and thus
> pl08x_dma_init_virtual_channels() sees 0 channels.
> And with platform_data the driver worked fine with fixed channels.
> 
> > > dma: pl08x: allocate OF slave channel data at probe time
> 
> This looks ore promising, I'm checking it.

Now I see the need for the first patch, but I wonder
why not just amend the second one with this instead:

@@ -1916,6 +1916,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,

                if (slave) {
                        chan->cd = &pl08x->pd->slave_channels[i];
+                       chan->signal = i;
                        pl08x_dma_slave_init(chan);
                } else {
                        chan->cd = &pl08x->pd->memcpy_channel;

(The assignment of chan->name = chan->cd->bus_id is
already done by pl08x_dma_slave_init().)

If the platform is using a mux then pd->get_xfer_signal()
would replace the signal, so this initialization wouldn't
do any harm, right?

Also, I wonder if "#define PL08X_DMA_SIGNALS 32" is an issue
with pl081 which only has 16 signals?  But I guess not since
nothing would request them.


Johannes

dmaengine pl08x dt crash

[Linus Walleij]

On Wed, Mar 23, 2016 at 6:26 PM, Johannes Stezenbach <js@sig21.net> wrote:

> Now I see the need for the first patch, but I wonder
> why not just amend the second one with this instead:
>
> @@ -1916,6 +1916,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
>
>                 if (slave) {
>                         chan->cd = &pl08x->pd->slave_channels[i];
> +                       chan->signal = i;
>                         pl08x_dma_slave_init(chan);
>                 } else {
>                         chan->cd = &pl08x->pd->memcpy_channel;
>
> (The assignment of chan->name = chan->cd->bus_id is
> already done by pl08x_dma_slave_init().)
>
> If the platform is using a mux then pd->get_xfer_signal()
> would replace the signal, so this initialization wouldn't
> do any harm, right?

I agress that seems simpler. I'll squash it like so and send it out
so we get some rotation on this patch set.

> Also, I wonder if "#define PL08X_DMA_SIGNALS 32" is an issue
> with pl081 which only has 16 signals?  But I guess not since
> nothing would request them.

PL080 also just has 16 channels. But the extended version on
the Nomadik uses all 32 bits in the register. The assumption is
simply that one 32bit register will house all signals.

Yours,
Linus Walleij

dmaengine pl08x dt crash

[Linus Walleij]

On Wed, Mar 23, 2016 at 4:40 PM, Johannes Stezenbach <js@sig21.net> wrote:

> I'm trying to use the new devicetree support for pl08x
> added in commit aa4734da667442
> (dmaengine: pl08x: support dt channel assignment).
>
> It causes crash in the pl011 serial driver when it calls
> dma_chan_name() because the dma_chan->dev->device is NULL.
> I think the reason is dma_async_device_register() is called
> during pl08x_probe(), but at this time the number of slave
> channels is still 0 as these get allocated later in
> pl08x_of_xlate() when clients request the channels.

The more I look at this the more broken it looks.

The channels NEED to be present when
dma_async_device_register() is called or their
sysfs devices will never get initialized, this is used by
the core code and thus we get this NULL dereference
from dma_chan_name().

I think the only system currently using this is the LPCs,
and it works because they are using a mux
(drivers/dma/lpc18xx-dmamux.c) in front of the DMA
engine. Thus the dma_chan_name() will go to the
mux intermediary channel, not to the actual channel.

I sort of feel like the mux code should just call
dma_chan_name() on the channels it handles to
make a point...

I think the patch I made entitled
"dma: pl08x: allocate OF slave channel data at probe time"
must be applied, and a harsher version at that: one
that DELETE the dynamic channel allocation in pl08x_of_xlate(),
obviously the dmaengine core is not supposed to be used
like that. If we want to create slave channels on-the-fly, the code
inside dma_async_device_register() that create sysfs entries
etc need to be factored out and made use of from the xlate().

Yours,
Linus Walleij