Skip to contentSkip to conversation

HDLCD crashes with 6d910bfa809e

PATCH In review Robin Murphy 7 participants 17 messages Jun 7, 2016 – Jun 8, 2016

Subsystems (3)
Media (V4L/DVB) Arm Samsung Exynos Device tree
lore
Share & tools
lore

Robin Murphy

author
#0 lore raw

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.

#1 lore raw
Quoted 1 line · Robin Murphy

Hi Daniel, Liviu,

Hi Robin,

Quoted 9 lines

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

Quoted 4 lines

Regards,
Robin.

Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?
author
#2 lore raw

Hi Liviu,

Quoted 5 lines · liviu.dudau at arm.com
Quoted 4 lines · Robin Murphy

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 ;)

Quoted 7 lines
Quoted 4 lines

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.

#3 lore raw
Quoted 37 lines · Robin Murphy

Hi Liviu,

Quoted 5 lines · liviu.dudau at arm.com
Quoted 4 lines · Robin Murphy

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 ;)

Quoted 7 lines
Quoted 4 lines

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.

Quoted 8 lines

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

Quoted 5 lines

Oh well...

Robin.

Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?
#4 lore raw
Quoted 47 lines · liviu.dudau at arm.com
Quoted 37 lines · Robin Murphy

Hi Liviu,

Quoted 5 lines · liviu.dudau at arm.com
Quoted 4 lines · Robin Murphy

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 ;)

Quoted 7 lines
Quoted 4 lines

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.

Quoted 8 lines

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
Quoted 2 lines

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

Quoted 9 lines

Best regards,
Liviu

Quoted 5 lines

Oh well...

Robin.

Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?
#5 lore raw
In reply to Robin Murphy
Quoted 37 lines · Robin Murphy

Hi Liviu,

Quoted 5 lines · liviu.dudau at arm.com
Quoted 4 lines · Robin Murphy

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 ;)

Quoted 7 lines
Quoted 4 lines

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.

Quoted 8 lines

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

Quoted 5 lines

Oh well...

Robin.

Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?
#6 lore raw

PATCH 1/1 of: reserved_mem: restore old behavior when no region is defined

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 <[email protected]>
Fixes 59ce4039727ef40 ("of: reserved_mem: add support for using more than

one region for given device")

Signed-off-by Marek Szyprowski <[email protected]>
---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/of/of_reserved_mem.c +1 -1
index 3cf129f..06af99f 100644
@@ -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);
Signature · 1 line
1.9.2
Reviewed-by on v1 1/1
#7 lore raw

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

Quoted 8 lines · 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 <[email protected]>
Fixes 59ce4039727ef40 ("of: reserved_mem: add support for using more than

one region for given device")

Signed-off-by Marek Szyprowski <[email protected]>
Reviewed-by Sylwester Nawrocki <[email protected]>

I think this needs to be added to the media tree, where the original patch it fixes was applied.

Signature · 2 lines
Thanks,
Sylwester
Reviewed-by on v1 1/1
#8 lore raw

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

In reply to Marek Szyprowski
Quoted 8 lines · 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 <[email protected]>
Fixes 59ce4039727ef40 ("of: reserved_mem: add support for using more than

one region for given device")

Signed-off-by Marek Szyprowski <[email protected]>
Reviewed-by Liviu Dudau <[email protected]>
Quoted 24 lines
---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/of/of_reserved_mem.c +1 -1
index 3cf129f..06af99f 100644
@@ -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);
Signature · 6 lines
1.9.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
maintainer Acked-by on v1 1/1
#9 lore raw

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

In reply to Marek Szyprowski
Quoted 11 lines · 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 <[email protected]>
Fixes 59ce4039727ef40 ("of: reserved_mem: add support for using more than

one region for given device")

Signed-off-by Marek Szyprowski <[email protected]>
---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by Rob Herring <[email protected]>
Reviewed-by
#10 lore raw

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

Quoted 6 lines · Rob Herring
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 7 lines
Reported-by Liviu Dudau <[email protected]>
Fixes 59ce4039727ef40 ("of: reserved_mem: add support for using more than

one region for given device")

Signed-off-by Marek Szyprowski <[email protected]>
---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Looks reasonable; FWIW

Reviewed-by Sumit Semwal <[email protected]>
Quoted 5 lines
Acked-by Rob Herring <[email protected]>

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 5 lines
Thanks and regards,

Sumit Semwal
Linaro Mobile Group - Kernel Team Lead
Linaro.org │ Open source software for ARM SoCs
#11 lore raw
In reply to [email protected]

Hi All,

Quoted 48 lines · liviu.dudau at arm.com
Quoted 35 lines · Robin Murphy

Hi Liviu,

Quoted 11 lines · liviu.dudau at arm.com
Quoted 8 lines · Robin Murphy

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.

Quoted 8 lines

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.

Quoted 6 lines

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

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
#12 lore raw
Quoted 1 line · Marek Szyprowski

Hi All,

Hi Marek,

Quoted 56 lines · liviu.dudau at arm.com
Quoted 53 lines · Robin Murphy
Quoted 35 lines

Hi Liviu,

Quoted 11 lines · liviu.dudau at arm.com
Quoted 8 lines · Robin Murphy

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.

Quoted 8 lines

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?

Quoted 5 lines
Quoted 1 line

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.

Quoted 5 lines

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

Quoted 6 lines

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?
#13 lore raw

Hi Liviu,

Quoted 65 lines · liviu.dudau at arm.com
Quoted 54 lines · Marek Szyprowski
Quoted 53 lines · liviu.dudau at arm.com
Quoted 35 lines · Robin Murphy

Hi Liviu,

Quoted 11 lines · liviu.dudau at arm.com
Quoted 8 lines · Robin Murphy

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.

Quoted 8 lines

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?

Quoted 4 lines
Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 6 lines

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)?

Quoted 1 line

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

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
#14 lore raw
Quoted 1 line · Marek Szyprowski

Hi Liviu,

Hi Marek,

[HDLCD bug report content removed]

Quoted 42 lines
Quoted 19 lines
Quoted 9 lines
Quoted 9 lines

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?

Quoted 4 lines
Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 7 lines
Quoted 2 lines

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

Quoted 6 lines

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Signature · 8 lines
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?
#15 lore raw
In reply to Robin Murphy
Quoted 10 lines · 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...

Please make sure you have the drm fixes from 4.7-rc2 included, this issue is fixed there.
-Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
author
#16 lore raw
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · 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 ;)

Quoted 7 lines
Quoted 4 lines

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.

Quoted 2 lines

-Daniel