HDLCD crashes with 6d910bfa809e

HDLCD crashes with 6d910bfa809e

[Robin Murphy]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[liviu.dudau]

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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[Robin Murphy]

Hi Liviu,

On 07/06/16 14:35, liviu.dudau@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.

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

Re: HDLCD crashes with 6d910bfa809e

[liviu.dudau]

On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
> Hi Liviu,
> 
> On 07/06/16 14:35, liviu.dudau@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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[liviu.dudau]

On Tue, Jun 07, 2016 at 03:14:04PM +0100, liviu.dudau@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@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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[liviu.dudau]

On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
> Hi Liviu,
> 
> On 07/06/16 14:35, liviu.dudau@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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] of: reserved_mem: restore old behavior when no region is defined

[Marek Szyprowski]

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

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

Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined

[Sylwester Nawrocki]

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

Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined

[Liviu Dudau]

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@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!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined

[Rob Herring]

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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined

[Sumit Semwal]

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@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

Re: HDLCD crashes with 6d910bfa809e

[Marek Szyprowski]

Hi All,


On 2016-06-07 16:34, liviu.dudau@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@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

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

Re: HDLCD crashes with 6d910bfa809e

[liviu.dudau]

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@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@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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[Marek Szyprowski]

Hi Liviu,


On 2016-06-08 11:05, liviu.dudau@arm.com wrote:
> On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote:
>> On 2016-06-07 16:34, liviu.dudau@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@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@vger.kernel.org/msg97645.html

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: HDLCD crashes with 6d910bfa809e

[liviu.dudau]

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@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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[Daniel Vetter]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDLCD crashes with 6d910bfa809e

[Robin Murphy]

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
>

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