HDLCD crashes with 6d910bfa809e
Subsystems (3)
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.
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-NULLdrm->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! /
---------------
?\_(?)_/?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-NULLdrm->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.
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-NULLdrm->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! /
---------------
?\_(?)_/?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-NULLdrm->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,
LiviuQuoted 5 lines
Oh well...
Robin.
Signature · 8 lines
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?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-NULLdrm->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! /
---------------
?\_(?)_/?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-NULLdrm->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
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-NULLdrm->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
PATCH 1/1 of: reserved_mem: restore old behavior when no region is defined
In reply to [email protected]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.
one region for given device")
--- drivers/of/of_reserved_mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/of/of_reserved_mem.c +1 -1
@@ -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
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-NULLdrm->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 -22OK, the problem is that commit
59ce4039727ef40has 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 usesdev->of_nodefor np so it would crash before the check anyway. Maybe move the check there?Until then I suggest reverting the
59ce4039727ef40commit.
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
Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined
In reply to Marek SzyprowskiQuoted 8 lines · Marek Szyprowski
Change return value back to
-ENODEVwhen 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 thanone region for given device")
Signed-off-by Marek Szyprowski <[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
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-NULLdrm->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 -22OK, the problem is that commit
59ce4039727ef40has 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 usesdev->of_nodefor 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
59ce4039727ef40commit.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_idxcan 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! /
---------------
?\_(?)_/?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-NULLdrm->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 -22OK, the problem is that commit
59ce4039727ef40has 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 usesdev->of_nodefor 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
59ce4039727ef40commit.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_idxcan 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 toof_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
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
59ce4039727ef40has 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 usesdev->of_nodefor 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
59ce4039727ef40commit.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_idxcan 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 callof_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! /
---------------
?\_(?)_/?Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined
In reply to Marek SzyprowskiQuoted 8 lines · Marek Szyprowski
Change return value back to
-ENODEVwhen 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 thanone region for given device")
Signed-off-by Marek Szyprowski <[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@@ -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! /
---------------
¯\_(ツ)_/¯Re: [PATCH] of: reserved_mem: restore old behavior when no region is defined
In reply to Marek SzyprowskiQuoted 11 lines · Marek Szyprowski
Change return value back to
-ENODEVwhen 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 thanone 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(-)
Sumit Semwal
Reviewed-byRe: [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
-ENODEVwhen 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 thanone 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
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