| Message ID | 20180828084442.30176-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | of/platform: Initialise AMBA default DMA masks | expand |
> /* 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 {
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
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.
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.
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..
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.
On Thu, Aug 30, 2018 at 10:40 AM Russell King - ARM Linux <linux@armlinux.org.uk> 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
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@armlinux.org.uk> 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.
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;
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(+)