* HDLCD crashes with 6d910bfa809e
@ 2016-06-07 12:06 Robin Murphy
2016-06-07 13:35 ` liviu.dudau at arm.com
2016-06-07 14:40 ` Daniel Vetter
0 siblings, 2 replies; 17+ messages in thread
From: Robin Murphy @ 2016-06-07 12:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi Daniel, Liviu,
Having just inadvertently merged -next into my working branch, I find
dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback")
adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a
framebuffer, drm_gem_cma_create() fails, unconditionally calls the
now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery
death ensues...
Regards,
Robin.
^ permalink raw reply [flat|nested] 17+ messages in thread* HDLCD crashes with 6d910bfa809e 2016-06-07 12:06 HDLCD crashes with 6d910bfa809e Robin Murphy @ 2016-06-07 13:35 ` liviu.dudau at arm.com 2016-06-07 14:11 ` Robin Murphy 2016-06-07 14:40 ` Daniel Vetter 1 sibling, 1 reply; 17+ messages in thread From: liviu.dudau at arm.com @ 2016-06-07 13:35 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: > Hi Daniel, Liviu, Hi Robin, > > Having just inadvertently merged -next into my working branch, I find > dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely > affecting my board's ability to boot ;) > > Since I (intentionally) don't have sufficient CMA to create a framebuffer, > drm_gem_cma_create() fails, unconditionally calls the now-NULL > drm->driver->gem_free_object() in its cleanup path, and fiery death > ensues... Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this? Best regards, Liviu > > Regards, > Robin. > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 13:35 ` liviu.dudau at arm.com @ 2016-06-07 14:11 ` Robin Murphy 2016-06-07 14:14 ` liviu.dudau at arm.com 2016-06-07 14:34 ` liviu.dudau at arm.com 0 siblings, 2 replies; 17+ messages in thread From: Robin Murphy @ 2016-06-07 14:11 UTC (permalink / raw) To: linux-arm-kernel Hi Liviu, On 07/06/16 14:35, liviu.dudau at arm.com wrote: > On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: >> Having just inadvertently merged -next into my working branch, I find >> dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely >> affecting my board's ability to boot ;) >> >> Since I (intentionally) don't have sufficient CMA to create a framebuffer, >> drm_gem_cma_create() fails, unconditionally calls the now-NULL >> drm->driver->gem_free_object() in its cleanup path, and fiery death >> ensues... > > Thanks for reporting this. What other changes other than reducing the CMA > allocation size do you have that I might need in order to reproduce this? I've just confirmed a plain checkout of next-20160602, using arm64 defconfig + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the job: [ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on. Today's -next, on the other hand, dodges the bullet entirely: [ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22 Oh well... Robin. ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 14:11 ` Robin Murphy @ 2016-06-07 14:14 ` liviu.dudau at arm.com 2016-06-07 14:15 ` liviu.dudau at arm.com 2016-06-07 14:34 ` liviu.dudau at arm.com 1 sibling, 1 reply; 17+ messages in thread From: liviu.dudau at arm.com @ 2016-06-07 14:14 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote: > Hi Liviu, > > On 07/06/16 14:35, liviu.dudau at arm.com wrote: > >On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: > >>Having just inadvertently merged -next into my working branch, I find > >>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely > >>affecting my board's ability to boot ;) > >> > >>Since I (intentionally) don't have sufficient CMA to create a framebuffer, > >>drm_gem_cma_create() fails, unconditionally calls the now-NULL > >>drm->driver->gem_free_object() in its cleanup path, and fiery death > >>ensues... > > > >Thanks for reporting this. What other changes other than reducing the CMA > >allocation size do you have that I might need in order to reproduce this? > > I've just confirmed a plain checkout of next-20160602, using arm64 defconfig > + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the > job: > > [ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) > [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 3.044970] [drm] No driver support for vblank timestamp query. > [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size > 7680000 > [ 3.084081] Bad mode in Synchronous Abort handler detected, code > 0x86000004 -- IABT (current EL) > [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted > 4.7.0-rc1-next-20160602 #686 > [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) > [ 3.106567] Workqueue: deferwq deferred_probe_work_func > [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: > ffff8009768e8000 > [ 3.119198] PC is at 0x0 > [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 > ...and so on. > > Today's -next, on the other hand, dodges the bullet entirely: > > [ 2.903645] [drm] found ARM HDLCD version r0p0 > [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 > [ 2.913505] tda998x: probe of 0-0070 failed with error -22 > [ 2.919141] [drm] found ARM HDLCD version r0p0 > [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 > [ 2.928991] tda998x: probe of 0-0071 failed with error -22 Yes, if fails in of_reserved_mem_device_init(). Tracking now to see which of the conditions at line 311 are the culprits. Best regards, Liviu > > Oh well... > > Robin. > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 14:14 ` liviu.dudau at arm.com @ 2016-06-07 14:15 ` liviu.dudau at arm.com 0 siblings, 0 replies; 17+ messages in thread From: liviu.dudau at arm.com @ 2016-06-07 14:15 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jun 07, 2016 at 03:14:04PM +0100, liviu.dudau at arm.com wrote: > On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote: > > Hi Liviu, > > > > On 07/06/16 14:35, liviu.dudau at arm.com wrote: > > >On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: > > >>Having just inadvertently merged -next into my working branch, I find > > >>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely > > >>affecting my board's ability to boot ;) > > >> > > >>Since I (intentionally) don't have sufficient CMA to create a framebuffer, > > >>drm_gem_cma_create() fails, unconditionally calls the now-NULL > > >>drm->driver->gem_free_object() in its cleanup path, and fiery death > > >>ensues... > > > > > >Thanks for reporting this. What other changes other than reducing the CMA > > >allocation size do you have that I might need in order to reproduce this? > > > > I've just confirmed a plain checkout of next-20160602, using arm64 defconfig > > + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the > > job: > > > > [ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) > > [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > > [ 3.044970] [drm] No driver support for vblank timestamp query. > > [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size > > 7680000 > > [ 3.084081] Bad mode in Synchronous Abort handler detected, code > > 0x86000004 -- IABT (current EL) > > [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted > > 4.7.0-rc1-next-20160602 #686 > > [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) > > [ 3.106567] Workqueue: deferwq deferred_probe_work_func > > [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: > > ffff8009768e8000 > > [ 3.119198] PC is at 0x0 > > [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 > > ...and so on. > > > > Today's -next, on the other hand, dodges the bullet entirely: > > > > [ 2.903645] [drm] found ARM HDLCD version r0p0 > > [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 > > [ 2.913505] tda998x: probe of 0-0070 failed with error -22 > > [ 2.919141] [drm] found ARM HDLCD version r0p0 > > [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 > > [ 2.928991] tda998x: probe of 0-0071 failed with error -22 > > Yes, if fails in of_reserved_mem_device_init(). Tracking now to see > which of the conditions at line 311 are the culprits. Bah, discard the line number, old cached version in my editor. Liviu > > Best regards, > Liviu > > > > > Oh well... > > > > Robin. > > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 14:11 ` Robin Murphy 2016-06-07 14:14 ` liviu.dudau at arm.com @ 2016-06-07 14:34 ` liviu.dudau at arm.com 2016-06-08 6:51 ` [PATCH] of: reserved_mem: restore old behavior when no region is defined Marek Szyprowski 2016-06-08 6:58 ` HDLCD crashes with 6d910bfa809e Marek Szyprowski 1 sibling, 2 replies; 17+ messages in thread From: liviu.dudau at arm.com @ 2016-06-07 14:34 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote: > Hi Liviu, > > On 07/06/16 14:35, liviu.dudau at arm.com wrote: > >On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: > >>Having just inadvertently merged -next into my working branch, I find > >>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely > >>affecting my board's ability to boot ;) > >> > >>Since I (intentionally) don't have sufficient CMA to create a framebuffer, > >>drm_gem_cma_create() fails, unconditionally calls the now-NULL > >>drm->driver->gem_free_object() in its cleanup path, and fiery death > >>ensues... > > > >Thanks for reporting this. What other changes other than reducing the CMA > >allocation size do you have that I might need in order to reproduce this? > > I've just confirmed a plain checkout of next-20160602, using arm64 defconfig > + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the > job: > > [ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) > [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 3.044970] [drm] No driver support for vblank timestamp query. > [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size > 7680000 > [ 3.084081] Bad mode in Synchronous Abort handler detected, code > 0x86000004 -- IABT (current EL) > [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted > 4.7.0-rc1-next-20160602 #686 > [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) > [ 3.106567] Workqueue: deferwq deferred_probe_work_func > [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: > ffff8009768e8000 > [ 3.119198] PC is at 0x0 > [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 > ...and so on. > > Today's -next, on the other hand, dodges the bullet entirely: > > [ 2.903645] [drm] found ARM HDLCD version r0p0 > [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 > [ 2.913505] tda998x: probe of 0-0070 failed with error -22 > [ 2.919141] [drm] found ARM HDLCD version r0p0 > [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 > [ 2.928991] tda998x: probe of 0-0071 failed with error -22 OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL. Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there? Until then I suggest reverting the 59ce4039727ef40 commit. Best regards, Liviu > > Oh well... > > Robin. > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] of: reserved_mem: restore old behavior when no region is defined 2016-06-07 14:34 ` liviu.dudau at arm.com @ 2016-06-08 6:51 ` Marek Szyprowski 2016-06-08 7:35 ` Sylwester Nawrocki ` (2 more replies) 2016-06-08 6:58 ` HDLCD crashes with 6d910bfa809e Marek Szyprowski 1 sibling, 3 replies; 17+ messages in thread From: Marek Szyprowski @ 2016-06-08 6:51 UTC (permalink / raw) To: linux-arm-kernel Change return value back to -ENODEV when no region is defined for given device. This restores old behavior of this function, as some drivers rely on such error code. Reported-by: Liviu Dudau <liviu.dudau@arm.com> Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than one region for given device") Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/of/of_reserved_mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 3cf129f..06af99f 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -334,7 +334,7 @@ int of_reserved_mem_device_init_by_idx(struct device *dev, target = of_parse_phandle(np, "memory-region", idx); if (!target) - return -EINVAL; + return -ENODEV; rmem = __find_rmem(target); of_node_put(target); -- 1.9.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] of: reserved_mem: restore old behavior when no region is defined 2016-06-08 6:51 ` [PATCH] of: reserved_mem: restore old behavior when no region is defined Marek Szyprowski @ 2016-06-08 7:35 ` Sylwester Nawrocki 2016-06-08 10:49 ` Liviu Dudau 2016-06-08 13:05 ` Rob Herring 2 siblings, 0 replies; 17+ messages in thread From: Sylwester Nawrocki @ 2016-06-08 7:35 UTC (permalink / raw) To: linux-arm-kernel On 06/08/2016 08:51 AM, Marek Szyprowski wrote: > Change return value back to -ENODEV when no region is defined for given > device. This restores old behavior of this function, as some drivers rely > on such error code. > > Reported-by: Liviu Dudau <liviu.dudau@arm.com> > Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than > one region for given device") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> I think this needs to be added to the media tree, where the original patch it fixes was applied. -- Thanks, Sylwester ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] of: reserved_mem: restore old behavior when no region is defined 2016-06-08 6:51 ` [PATCH] of: reserved_mem: restore old behavior when no region is defined Marek Szyprowski 2016-06-08 7:35 ` Sylwester Nawrocki @ 2016-06-08 10:49 ` Liviu Dudau 2016-06-08 13:05 ` Rob Herring 2 siblings, 0 replies; 17+ messages in thread From: Liviu Dudau @ 2016-06-08 10:49 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 08, 2016 at 08:51:53AM +0200, Marek Szyprowski wrote: > Change return value back to -ENODEV when no region is defined for given > device. This restores old behavior of this function, as some drivers rely > on such error code. > > Reported-by: Liviu Dudau <liviu.dudau@arm.com> > Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than > one region for given device") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com> > --- > drivers/of/of_reserved_mem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > index 3cf129f..06af99f 100644 > --- a/drivers/of/of_reserved_mem.c > +++ b/drivers/of/of_reserved_mem.c > @@ -334,7 +334,7 @@ int of_reserved_mem_device_init_by_idx(struct device *dev, > > target = of_parse_phandle(np, "memory-region", idx); > if (!target) > - return -EINVAL; > + return -ENODEV; > > rmem = __find_rmem(target); > of_node_put(target); > -- > 1.9.2 > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] of: reserved_mem: restore old behavior when no region is defined 2016-06-08 6:51 ` [PATCH] of: reserved_mem: restore old behavior when no region is defined Marek Szyprowski 2016-06-08 7:35 ` Sylwester Nawrocki 2016-06-08 10:49 ` Liviu Dudau @ 2016-06-08 13:05 ` Rob Herring 2016-06-08 15:35 ` Sumit Semwal 2 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2016-06-08 13:05 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 8, 2016 at 1:51 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Change return value back to -ENODEV when no region is defined for given > device. This restores old behavior of this function, as some drivers rely > on such error code. > > Reported-by: Liviu Dudau <liviu.dudau@arm.com> > Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than > one region for given device") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/of/of_reserved_mem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] of: reserved_mem: restore old behavior when no region is defined 2016-06-08 13:05 ` Rob Herring @ 2016-06-08 15:35 ` Sumit Semwal 0 siblings, 0 replies; 17+ messages in thread From: Sumit Semwal @ 2016-06-08 15:35 UTC (permalink / raw) To: linux-arm-kernel On 8 June 2016 at 18:35, Rob Herring <robh@kernel.org> wrote: > On Wed, Jun 8, 2016 at 1:51 AM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> Change return value back to -ENODEV when no region is defined for given >> device. This restores old behavior of this function, as some drivers rely >> on such error code. >> >> Reported-by: Liviu Dudau <liviu.dudau@arm.com> >> Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than >> one region for given device") >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/of/of_reserved_mem.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > Looks reasonable; FWIW Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org> > Acked-by: Rob Herring <robh@kernel.org> > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Thanks and regards, Sumit Semwal Linaro Mobile Group - Kernel Team Lead Linaro.org ? Open source software for ARM SoCs ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 14:34 ` liviu.dudau at arm.com 2016-06-08 6:51 ` [PATCH] of: reserved_mem: restore old behavior when no region is defined Marek Szyprowski @ 2016-06-08 6:58 ` Marek Szyprowski 2016-06-08 9:05 ` liviu.dudau at arm.com 1 sibling, 1 reply; 17+ messages in thread From: Marek Szyprowski @ 2016-06-08 6:58 UTC (permalink / raw) To: linux-arm-kernel Hi All, On 2016-06-07 16:34, liviu.dudau at arm.com wrote: > On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote: >> Hi Liviu, >> >> On 07/06/16 14:35, liviu.dudau at arm.com wrote: >>> On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: >>>> Having just inadvertently merged -next into my working branch, I find >>>> dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely >>>> affecting my board's ability to boot ;) >>>> >>>> Since I (intentionally) don't have sufficient CMA to create a framebuffer, >>>> drm_gem_cma_create() fails, unconditionally calls the now-NULL >>>> drm->driver->gem_free_object() in its cleanup path, and fiery death >>>> ensues... >>> Thanks for reporting this. What other changes other than reducing the CMA >>> allocation size do you have that I might need in order to reproduce this? >> I've just confirmed a plain checkout of next-20160602, using arm64 defconfig >> + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the >> job: >> >> [ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) >> [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >> [ 3.044970] [drm] No driver support for vblank timestamp query. >> [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size >> 7680000 >> [ 3.084081] Bad mode in Synchronous Abort handler detected, code >> 0x86000004 -- IABT (current EL) >> [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted >> 4.7.0-rc1-next-20160602 #686 >> [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) >> [ 3.106567] Workqueue: deferwq deferred_probe_work_func >> [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: >> ffff8009768e8000 >> [ 3.119198] PC is at 0x0 >> [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 >> ...and so on. >> >> Today's -next, on the other hand, dodges the bullet entirely: >> >> [ 2.903645] [drm] found ARM HDLCD version r0p0 >> [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 >> [ 2.913505] tda998x: probe of 0-0070 failed with error -22 >> [ 2.919141] [drm] found ARM HDLCD version r0p0 >> [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 >> [ 2.928991] tda998x: probe of 0-0071 failed with error -22 > OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when > there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now > it returns -EINVAL. > > Marek, I quite liked the old behaviour to detect if the DT had the optional (from > my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous > when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would > crash before the check anyway. Maybe move the check there? > > Until then I suggest reverting the 59ce4039727ef40 commit. I've just send a fix for this issue. I'm sorry for the regression. I hope the fix fill quickly get into next to solve your problem. The additional check for null dev make sense, because the new function of_reserved_mem_device_init_by_idx can be called with any device and node pointer not embedded with it, so I would like to keep it safe. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-08 6:58 ` HDLCD crashes with 6d910bfa809e Marek Szyprowski @ 2016-06-08 9:05 ` liviu.dudau at arm.com 2016-06-08 10:01 ` Marek Szyprowski 0 siblings, 1 reply; 17+ messages in thread From: liviu.dudau at arm.com @ 2016-06-08 9:05 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote: > Hi All, Hi Marek, > > > On 2016-06-07 16:34, liviu.dudau at arm.com wrote: > >On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote: > >>Hi Liviu, > >> > >>On 07/06/16 14:35, liviu.dudau at arm.com wrote: > >>>On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: > >>>>Having just inadvertently merged -next into my working branch, I find > >>>>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely > >>>>affecting my board's ability to boot ;) > >>>> > >>>>Since I (intentionally) don't have sufficient CMA to create a framebuffer, > >>>>drm_gem_cma_create() fails, unconditionally calls the now-NULL > >>>>drm->driver->gem_free_object() in its cleanup path, and fiery death > >>>>ensues... > >>>Thanks for reporting this. What other changes other than reducing the CMA > >>>allocation size do you have that I might need in order to reproduce this? > >>I've just confirmed a plain checkout of next-20160602, using arm64 defconfig > >>+ DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the > >>job: > >> > >>[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) > >>[ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > >>[ 3.044970] [drm] No driver support for vblank timestamp query. > >>[ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size > >>7680000 > >>[ 3.084081] Bad mode in Synchronous Abort handler detected, code > >>0x86000004 -- IABT (current EL) > >>[ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted > >>4.7.0-rc1-next-20160602 #686 > >>[ 3.100682] Hardware name: ARM Juno development board (r1) (DT) > >>[ 3.106567] Workqueue: deferwq deferred_probe_work_func > >>[ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: > >>ffff8009768e8000 > >>[ 3.119198] PC is at 0x0 > >>[ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 > >>...and so on. > >> > >>Today's -next, on the other hand, dodges the bullet entirely: > >> > >>[ 2.903645] [drm] found ARM HDLCD version r0p0 > >>[ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 > >>[ 2.913505] tda998x: probe of 0-0070 failed with error -22 > >>[ 2.919141] [drm] found ARM HDLCD version r0p0 > >>[ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 > >>[ 2.928991] tda998x: probe of 0-0071 failed with error -22 > >OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when > >there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now > >it returns -EINVAL. > > > >Marek, I quite liked the old behaviour to detect if the DT had the optional (from > >my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous > >when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would > >crash before the check anyway. Maybe move the check there? > > > >Until then I suggest reverting the 59ce4039727ef40 commit. > > I've just send a fix for this issue. I'm sorry for the regression. I hope > the fix fill > quickly get into next to solve your problem. Thanks for the patch, however I have some comments to it. > > The additional check for null dev make sense, because the new function > of_reserved_mem_device_init_by_idx can be called with any device and node > pointer not > embedded with it, so I would like to keep it safe. And I have to admit I find that scary. Why do you accept any node that is *not* related to the device? If you want just the ability to parse multiple "memory-region" phandles (where are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to the the parsing and accept either the single entry style or a node with multiple "memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device and that device would have no idea that I have done this. Can you point me to the latest thread where this patch has been discussed? Best regards, Liviu > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-08 9:05 ` liviu.dudau at arm.com @ 2016-06-08 10:01 ` Marek Szyprowski 2016-06-08 10:42 ` liviu.dudau at arm.com 0 siblings, 1 reply; 17+ messages in thread From: Marek Szyprowski @ 2016-06-08 10:01 UTC (permalink / raw) To: linux-arm-kernel Hi Liviu, On 2016-06-08 11:05, liviu.dudau at arm.com wrote: > On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote: >> On 2016-06-07 16:34, liviu.dudau at arm.com wrote: >>> On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote: >>>> Hi Liviu, >>>> >>>> On 07/06/16 14:35, liviu.dudau at arm.com wrote: >>>>> On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: >>>>>> Having just inadvertently merged -next into my working branch, I find >>>>>> dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely >>>>>> affecting my board's ability to boot ;) >>>>>> >>>>>> Since I (intentionally) don't have sufficient CMA to create a framebuffer, >>>>>> drm_gem_cma_create() fails, unconditionally calls the now-NULL >>>>>> drm->driver->gem_free_object() in its cleanup path, and fiery death >>>>>> ensues... >>>>> Thanks for reporting this. What other changes other than reducing the CMA >>>>> allocation size do you have that I might need in order to reproduce this? >>>> I've just confirmed a plain checkout of next-20160602, using arm64 defconfig >>>> + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the >>>> job: >>>> >>>> [ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) >>>> [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >>>> [ 3.044970] [drm] No driver support for vblank timestamp query. >>>> [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size >>>> 7680000 >>>> [ 3.084081] Bad mode in Synchronous Abort handler detected, code >>>> 0x86000004 -- IABT (current EL) >>>> [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted >>>> 4.7.0-rc1-next-20160602 #686 >>>> [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) >>>> [ 3.106567] Workqueue: deferwq deferred_probe_work_func >>>> [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: >>>> ffff8009768e8000 >>>> [ 3.119198] PC is at 0x0 >>>> [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 >>>> ...and so on. >>>> >>>> Today's -next, on the other hand, dodges the bullet entirely: >>>> >>>> [ 2.903645] [drm] found ARM HDLCD version r0p0 >>>> [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 >>>> [ 2.913505] tda998x: probe of 0-0070 failed with error -22 >>>> [ 2.919141] [drm] found ARM HDLCD version r0p0 >>>> [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 >>>> [ 2.928991] tda998x: probe of 0-0071 failed with error -22 >>> OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when >>> there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now >>> it returns -EINVAL. >>> >>> Marek, I quite liked the old behaviour to detect if the DT had the optional (from >>> my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous >>> when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would >>> crash before the check anyway. Maybe move the check there? >>> >>> Until then I suggest reverting the 59ce4039727ef40 commit. >> I've just send a fix for this issue. I'm sorry for the regression. I hope >> the fix fill >> quickly get into next to solve your problem. > Thanks for the patch, however I have some comments to it. > >> The additional check for null dev make sense, because the new function >> of_reserved_mem_device_init_by_idx can be called with any device and node >> pointer not >> embedded with it, so I would like to keep it safe. > And I have to admit I find that scary. Why do you accept any node that is *not* related to > the device? If you want just the ability to parse multiple "memory-region" phandles (where > are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to > the the parsing and accept either the single entry style or a node with multiple > "memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device > and that device would have no idea that I have done this. The idea is not to steal memory region from another device, but to let driver to use multiple regions assigned to the supported device with dma-mapping api. To use them with dma-mapping subsystem, one needs separate struct device for each reserved region. The idea is to create child devices of the device, which has memory-region property. Then for each such child device, driver can call of_reserved_mem_device_init_by_idx() to enable usage of dma-mapping api based on the selected reserved region. Such approach was already used for long time in the media/platform/s5p-mfc driver and now it has been converted to use generic reserved memory regions. If you feel scary about this approach, maybe a check if the device provided to of_reserved_mem_device_init_by_idx() function is one of the successors of the device hidden behind the provided node pointer (or points to the same device)? > Can you point me to the latest thread where this patch has been discussed? This patch is a part of "Exynos: MFC driver: reserved memory cleanup and IOMMU support" thread and has been around for a while: https://www.mail-archive.com/linux-media at vger.kernel.org/msg97645.html Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-08 10:01 ` Marek Szyprowski @ 2016-06-08 10:42 ` liviu.dudau at arm.com 0 siblings, 0 replies; 17+ messages in thread From: liviu.dudau at arm.com @ 2016-06-08 10:42 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 08, 2016 at 12:01:20PM +0200, Marek Szyprowski wrote: > Hi Liviu, Hi Marek, [HDLCD bug report content removed] > >>>OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when > >>>there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now > >>>it returns -EINVAL. > >>> > >>>Marek, I quite liked the old behaviour to detect if the DT had the optional (from > >>>my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous > >>>when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would > >>>crash before the check anyway. Maybe move the check there? > >>> > >>>Until then I suggest reverting the 59ce4039727ef40 commit. > >>I've just send a fix for this issue. I'm sorry for the regression. I hope > >>the fix fill > >>quickly get into next to solve your problem. > >Thanks for the patch, however I have some comments to it. > > > >>The additional check for null dev make sense, because the new function > >>of_reserved_mem_device_init_by_idx can be called with any device and node > >>pointer not > >>embedded with it, so I would like to keep it safe. > >And I have to admit I find that scary. Why do you accept any node that is *not* related to > >the device? If you want just the ability to parse multiple "memory-region" phandles (where > >are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to > >the the parsing and accept either the single entry style or a node with multiple > >"memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device > >and that device would have no idea that I have done this. > > The idea is not to steal memory region from another device, but to let > driver to use multiple > regions assigned to the supported device with dma-mapping api. To use them > with dma-mapping > subsystem, one needs separate struct device for each reserved region. The > idea is to create > child devices of the device, which has memory-region property. Then for each > such child > device, driver can call of_reserved_mem_device_init_by_idx() to enable usage > of dma-mapping > api based on the selected reserved region. Such approach was already used > for long time in > the media/platform/s5p-mfc driver and now it has been converted to use > generic reserved > memory regions. > > If you feel scary about this approach, maybe a check if the device provided > to > of_reserved_mem_device_init_by_idx() function is one of the successors of > the device hidden > behind the provided node pointer (or points to the same device)? That would not hurt to have. > > >Can you point me to the latest thread where this patch has been discussed? > > This patch is a part of "Exynos: MFC driver: reserved memory cleanup and > IOMMU support" thread > and has been around for a while: > https://www.mail-archive.com/linux-media at vger.kernel.org/msg97645.html Thanks! I'm not subscribed to linux-media and way behind a lot of other MLs, so I have not noticed it. I've had a look at the patchset, found one case where you might leak some memory. I also think you could've had an of_reserved_mem_device_init_list() function that inits all the "memory-region" nodes and returns a list struct reserved_mem* that the driver can then assign to whatever internal variables it has. That way you can validate that all the "memory-region" phandles are used by the same parent device. Best regards, Liviu > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 12:06 HDLCD crashes with 6d910bfa809e Robin Murphy 2016-06-07 13:35 ` liviu.dudau at arm.com @ 2016-06-07 14:40 ` Daniel Vetter 2016-06-07 16:42 ` Robin Murphy 1 sibling, 1 reply; 17+ messages in thread From: Daniel Vetter @ 2016-06-07 14:40 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: > Hi Daniel, Liviu, > > Having just inadvertently merged -next into my working branch, I find > dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely > affecting my board's ability to boot ;) > > Since I (intentionally) don't have sufficient CMA to create a framebuffer, > drm_gem_cma_create() fails, unconditionally calls the now-NULL > drm->driver->gem_free_object() in its cleanup path, and fiery death > ensues... Please make sure you have the drm fixes from 4.7-rc2 included, this issue is fixed there. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 17+ messages in thread
* HDLCD crashes with 6d910bfa809e 2016-06-07 14:40 ` Daniel Vetter @ 2016-06-07 16:42 ` Robin Murphy 0 siblings, 0 replies; 17+ messages in thread From: Robin Murphy @ 2016-06-07 16:42 UTC (permalink / raw) To: linux-arm-kernel On 07/06/16 15:40, Daniel Vetter wrote: > On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote: >> Hi Daniel, Liviu, >> >> Having just inadvertently merged -next into my working branch, I find >> dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely >> affecting my board's ability to boot ;) >> >> Since I (intentionally) don't have sufficient CMA to create a framebuffer, >> drm_gem_cma_create() fails, unconditionally calls the now-NULL >> drm->driver->gem_free_object() in its cleanup path, and fiery death >> ensues... > > Please make sure you have the drm fixes from 4.7-rc2 included, this issue > is fixed there. Ah, right you are, thanks - the fact that that the newer branches I tried crashed or otherwise failed due to unrelated TDA998x problems threw me off. I see Chris' fix there now. Robin. > -Daniel > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-06-08 15:35 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-07 12:06 HDLCD crashes with 6d910bfa809e Robin Murphy 2016-06-07 13:35 ` liviu.dudau at arm.com 2016-06-07 14:11 ` Robin Murphy 2016-06-07 14:14 ` liviu.dudau at arm.com 2016-06-07 14:15 ` liviu.dudau at arm.com 2016-06-07 14:34 ` liviu.dudau at arm.com 2016-06-08 6:51 ` [PATCH] of: reserved_mem: restore old behavior when no region is defined Marek Szyprowski 2016-06-08 7:35 ` Sylwester Nawrocki 2016-06-08 10:49 ` Liviu Dudau 2016-06-08 13:05 ` Rob Herring 2016-06-08 15:35 ` Sumit Semwal 2016-06-08 6:58 ` HDLCD crashes with 6d910bfa809e Marek Szyprowski 2016-06-08 9:05 ` liviu.dudau at arm.com 2016-06-08 10:01 ` Marek Szyprowski 2016-06-08 10:42 ` liviu.dudau at arm.com 2016-06-07 14:40 ` Daniel Vetter 2016-06-07 16:42 ` Robin Murphy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).