[PATCH] of/platform: Initialise AMBA default DMA masks

[PATCH] of/platform: Initialise AMBA default DMA masks

[Linus Walleij]

commit a5516219b102 ("of/platform: Initialise default DMA masks")
sets up the coherent_dma_mask of platform devices created
from the device tree, but fails to do the same for AMBA
(PrimeCell) devices.

This leads to a regression in kernel v4.19-rc1 triggering the
WARN_ONCE() in kernel/dma/coherent.c, dma_alloc_attrs()
WARN_ON_ONCE(dev && !dev->coherent_dma_mask):

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at ../include/linux/dma-mapping.h:522 drm_gem_cma_create+0x1dc/0x21c
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.19.0-rc1+ #15
Hardware name: ARM-Versatile (Device Tree Support)
[<c001c558>] (unwind_backtrace) from [<c00191f8>] (show_stack+0x10/0x14)
[<c00191f8>] (show_stack) from [<c00246a0>] (__warn+0xcc/0xf4)
[<c00246a0>] (__warn) from [<c00247dc>] (warn_slowpath_null+0x3c/0x48)
[<c00247dc>] (warn_slowpath_null) from [<c025cef0>] (drm_gem_cma_create+0x1dc/0x21c)
[<c025cef0>] (drm_gem_cma_create) from [<c025d3fc>] (drm_gem_cma_dumb_create+0x44/0x98)
[<c025d3fc>] (drm_gem_cma_dumb_create) from [<c025c648>] (drm_client_framebuffer_create+0x80/0x204)
[<c025c648>] (drm_client_framebuffer_create) from [<c0234a20>] (drm_fb_helper_generic_probe+0x4c/0x200)
[<c0234a20>] (drm_fb_helper_generic_probe) from [<c0235b14>] (__drm_fb_helper_initial_config_and_unlock+0x1cc/0x454)
[<c0235b14>] (__drm_fb_helper_initial_config_and_unlock) from [<c0235ea0>] (drm_fb_helper_fbdev_setup+0x104/0x218)
[<c0235ea0>] (drm_fb_helper_fbdev_setup) from [<c0236410>] (drm_fbdev_cma_init+0x7c/0xac)
[<c0236410>] (drm_fbdev_cma_init) from [<c0236448>] (drm_fb_cma_fbdev_init+0x8/0x14)
[<c0236448>] (drm_fb_cma_fbdev_init) from [<c0260d90>] (pl111_amba_probe+0x3c8/0x4a4)
[<c0260d90>] (pl111_amba_probe) from [<c01f479c>] (amba_probe+0xd8/0x154)
[<c01f479c>] (amba_probe) from [<c0267398>] (really_probe+0x200/0x2ac)
[<c0267398>] (really_probe) from [<c02675a4>] (driver_probe_device+0x5c/0x168)
[<c02675a4>] (driver_probe_device) from [<c0267780>] (__driver_attach+0xd0/0xd4)
[<c0267780>] (__driver_attach) from [<c02656c0>] (bus_for_each_dev+0x70/0xb4)
[<c02656c0>] (bus_for_each_dev) from [<c0266808>] (bus_add_driver+0x170/0x204)
[<c0266808>] (bus_add_driver) from [<c0268090>] (driver_register+0x74/0x108)
[<c0268090>] (driver_register) from [<c000ac20>] (do_one_initcall+0x48/0x1a0)
[<c000ac20>] (do_one_initcall) from [<c0507dc0>] (kernel_init_freeable+0x104/0x1c4)
[<c0507dc0>] (kernel_init_freeable) from [<c03e6ee4>] (kernel_init+0x8/0xf0)
[<c03e6ee4>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34)
Exception stack(0xc781ffb0 to 0xc781fff8)
ffa0:                                     00000000 00000000 00000000 00000000
ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 2dc47eb796bde006 ]---

This regresses the PL111 DRM driver in drivers/gpu/drm/pl111
that uses the AMBA PrimeCell to instantiate the frame buffer
device, as it cannot allocate a chunk of coherent memory
anymore due to the missing mask.

Fixes: a5516219b102 ("of/platform: Initialise default DMA masks")
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Anholt <eric@anholt.net>
Cc: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I don't know which tree Robins patch came in from, but I assume
Christoph's, so can you carry this patch as well?
---
 drivers/of/platform.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 7ba90c290a42..7435c79ca56d 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -242,6 +242,9 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 		goto err_clear_flag;
 
 	/* setup generic device info */
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	if (!dev->dev.dma_mask)
+		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
 	dev->dev.of_node = of_node_get(node);
 	dev->dev.fwnode = &node->fwnode;
 	dev->dev.parent = parent ? : &platform_bus;
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Christoph Hellwig]

>  	/* setup generic device info */
> +	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +	if (!dev->dev.dma_mask)
> +		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;

We should never set dma_mask to point to the coherent_dma_mask,
as that will cause problems with devices that have different
mask for the two.

How about something like this?

---
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 7ba90c290a42..c04ed124305c 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -242,6 +242,9 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 		goto err_clear_flag;
 
 	/* setup generic device info */
+	dev->dma_mask = DMA_BIT_MASK(32);
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.of_node = of_node_get(node);
 	dev->dev.fwnode = &node->fwnode;
 	dev->dev.parent = parent ? : &platform_bus;
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index d143c13bed26..fbc7adf3ca54 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -34,6 +34,7 @@ struct amba_device {
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
 	char			*driver_override;
+	u64			dma_mask;
 };
 
 struct amba_driver {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Linus Walleij]

On Tue, Aug 28, 2018 at 11:21 AM Christoph Hellwig <hch@lst.de> wrote:

> > +     dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> > +     if (!dev->dev.dma_mask)
> > +             dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>
> We should never set dma_mask to point to the coherent_dma_mask,
> as that will cause problems with devices that have different
> mask for the two.
>
> How about something like this?
(...)
> +       u64                     dma_mask;

We did that before, so this becomes effectively a revert of:
commit 446b2a9380b64b9d7410d86ee8226031e03645cf

    DMA-API: amba: get rid of separate dma_mask

    AMBA Primecell devices always treat streaming and coherent DMA exactly
    the same, so there's no point in having the masks separated.

So there is some history around this.

There is also some code in drivers/amba/bus.c affected by that
and I need to look over all static amba device allocations if we
reintroduce this.

That said, the remaining static allocations (a.k.a. boardfiles) appear
to be very few and very limited, almost all systems use device tree
or ACPI or iterative dynamic allocation (from amba/bus.c functions)
of the amba devices now.

Do you think we can proceed with this patch or do you want me to
revert the split back?

FWIW the platform devices have the same problem, but I know
I know, two wrongs does not make one right :/

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Russell King - ARM Linux]

On Tue, Aug 28, 2018 at 03:25:55PM +0200, Linus Walleij wrote:
> On Tue, Aug 28, 2018 at 11:21 AM Christoph Hellwig <hch@lst.de> wrote:
> 
> > > +     dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> > > +     if (!dev->dev.dma_mask)
> > > +             dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> >
> > We should never set dma_mask to point to the coherent_dma_mask,
> > as that will cause problems with devices that have different
> > mask for the two.
> >
> > How about something like this?
> (...)
> > +       u64                     dma_mask;
> 
> We did that before, so this becomes effectively a revert of:
> commit 446b2a9380b64b9d7410d86ee8226031e03645cf
> 
>     DMA-API: amba: get rid of separate dma_mask
> 
>     AMBA Primecell devices always treat streaming and coherent DMA exactly
>     the same, so there's no point in having the masks separated.
> 
> So there is some history around this.
> 
> There is also some code in drivers/amba/bus.c affected by that
> and I need to look over all static amba device allocations if we
> reintroduce this.

I don't have the rest of this thread to read...

But yes, the fundamental fact is that AMBA devices don't have any
care about the differences between coherent and streaming DMA.  The
distinction that we make in the kernel is purely a software one when
it comes to these devices.

Most AMBA devices themselves are not DMA capable, as they are only
connected to the APB (Amba peripheral bus) and they rely on a
separate DMA engine for their DMA.  APB devices should not have DMA
masks - their DMA capabilities are entirely down to the DMA controller.
So, the majority of AMBA devices should not have any DMA masks.

Only those connected to a bus that they can master on (eg AXI) should
have DMA masks - things like the PL08x DMA controllers, PL11x LCD
controllers, etc.  As I've said above, there is no difference between
streaming and coherent DMA for these devices.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Christoph Hellwig]

On Tue, Aug 28, 2018 at 03:14:14PM +0100, Russell King - ARM Linux wrote:
> But yes, the fundamental fact is that AMBA devices don't have any
> care about the differences between coherent and streaming DMA.  The
> distinction that we make in the kernel is purely a software one when
> it comes to these devices.
> 
> Most AMBA devices themselves are not DMA capable, as they are only
> connected to the APB (Amba peripheral bus) and they rely on a
> separate DMA engine for their DMA.  APB devices should not have DMA
> masks - their DMA capabilities are entirely down to the DMA controller.
> So, the majority of AMBA devices should not have any DMA masks.
> 
> Only those connected to a bus that they can master on (eg AXI) should
> have DMA masks - things like the PL08x DMA controllers, PL11x LCD
> controllers, etc.  As I've said above, there is no difference between
> streaming and coherent DMA for these devices.

So for now I plan to apply the patch from Linus to just set a dma
mask, as that gets back the previous behavior where dma did just
work (as it did without a mask).

But if Linus, you or someone else familiar with amba would like to
add an explicit opt-in into dma support eventually that would be
even better.

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Russell King - ARM Linux]

On Wed, Aug 29, 2018 at 07:55:21AM +0200, Christoph Hellwig wrote:
> On Tue, Aug 28, 2018 at 03:14:14PM +0100, Russell King - ARM Linux wrote:
> > But yes, the fundamental fact is that AMBA devices don't have any
> > care about the differences between coherent and streaming DMA.  The
> > distinction that we make in the kernel is purely a software one when
> > it comes to these devices.
> > 
> > Most AMBA devices themselves are not DMA capable, as they are only
> > connected to the APB (Amba peripheral bus) and they rely on a
> > separate DMA engine for their DMA.  APB devices should not have DMA
> > masks - their DMA capabilities are entirely down to the DMA controller.
> > So, the majority of AMBA devices should not have any DMA masks.
> > 
> > Only those connected to a bus that they can master on (eg AXI) should
> > have DMA masks - things like the PL08x DMA controllers, PL11x LCD
> > controllers, etc.  As I've said above, there is no difference between
> > streaming and coherent DMA for these devices.
> 
> So for now I plan to apply the patch from Linus to just set a dma
> mask, as that gets back the previous behavior where dma did just
> work (as it did without a mask).

NAK on that at the moment.

> But if Linus, you or someone else familiar with amba would like to
> add an explicit opt-in into dma support eventually that would be
> even better.

Well, as I've no idea what the issue is here, I can't do anything or
make any suggestions.  I wasn't copied on the initial part of the
thread.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Linus Walleij]

On Thu, Aug 30, 2018 at 10:40 AM Russell King - ARM Linux
<linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> wrote:

> Well, as I've no idea what the issue is here, I can't do anything or
> make any suggestions.  I wasn't copied on the initial part of the
> thread.

Sorry about that, it was because the original patch only hit in
drivers/of/*.

I will send you a copy of the thread.

Yours,
Linus Walleij

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Russell King - ARM Linux]

On Thu, Aug 30, 2018 at 10:45:11AM +0200, Linus Walleij wrote:
> On Thu, Aug 30, 2018 at 10:40 AM Russell King - ARM Linux
> <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> wrote:
> 
> > Well, as I've no idea what the issue is here, I can't do anything or
> > make any suggestions.  I wasn't copied on the initial part of the
> > thread.
> 
> Sorry about that, it was because the original patch only hit in
> drivers/of/*.
> 
> I will send you a copy of the thread.

Thanks.

>From the original patch description:
> commit a5516219b102 ("of/platform: Initialise default DMA masks")
> sets up the coherent_dma_mask of platform devices created
> from the device tree, but fails to do the same for AMBA
> (PrimeCell) devices.
> 
> This leads to a regression in kernel v4.19-rc1 triggering the
> WARN_ONCE() in kernel/dma/coherent.c, dma_alloc_attrs()
> WARN_ON_ONCE(dev && !dev->coherent_dma_mask):

This description is very misleading.  It makes it sound like commit
a5516219b102 caused the problem.  Maybe someone would like to explain
how that can be the case - this commit just touches the DT platform
device code, and thus can not itself cause this regression.

Second reason it is misleading is that it claims that there is a
regression in 4.19-rc1 with a WARN in kernel/dma/coherent.c.
Looking at Linus' tip, kernel/dma/coherent.c does not contain any
WARNings except for one to do with dma_reserved_default_memory.
Looking at the history of that file in Linus' tree, there is only
one commit which touches this file, and that is Christoph's commit
which creates it.

So, this patch description needs much improvement, and AFAICS there
is no regression in Linus' kernel.  There may be a regression in
-next, but that is not as urgent as if it were in Linus' tree - iow,
we have time to fix this properly.


Now, from the AMBA perspective, we do not want every AMBA device to
appear to be DMA capable - we only want the ones which are to be so.

>From the backtrace, it seems to be a PL111 causing the issue - this
device has an AXI bus interface as well as an APB bus interface, so
is one of the few that are capable of DMA.  It's only restriction
is that it is limited to 32 bits of DMA address, and it doesn't care
about Linux's distinction between coherent and streaming DMA.

So, I'd suggest that we arrange for the DT code to setup the DMA mask
for the device only if it has an appropriate property (dma-ranges?),
and we don't need to re-introduce the separate mask for coherent DMA.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH] of/platform: Initialise AMBA default DMA masks

[Christoph Hellwig]

On Tue, Aug 28, 2018 at 03:25:55PM +0200, Linus Walleij wrote:
> Do you think we can proceed with this patch or do you want me to
> revert the split back?

I'll apply this patch (probably with a little common in the source
explaining the situation), based on the feedback from you and Russell.

> 
> FWIW the platform devices have the same problem, but I know
> I know, two wrongs does not make one right :/

I have a patch pending for that..
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel