[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
The code in imx_ldb_encoder_mode_set crashes with a NULL pointer dereference trying to access crtc->state->state, which was previously cleared by drm_atomic_helper_swap_state:
Unable to handle kernel NULL pointer dereference at virtual address 00000010 pgd = ae08c000 [00000010] *pgd=3e00e831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 PID: 102 Comm: kmsfb-manage Not tainted 4.7.0-rc5+ #232 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) task: ae058c40 ti: ae04e000 task.ti: ae04e000 PC is at imx_ldb_encoder_mode_set+0x138/0x2f8 LR is at 0xae881818 pc : [<8051a8c8>] lr : [<ae881818>] psr: 600f0013 sp : ae04fc70 ip : ae04fbb0 fp : ae04fcbc r10: ae8ea018 r9 : 00000000 r8 : ae246418 r7 : ae8ea010 r6 : ae8ea308 r5 : 00000000 r4 : 00000000 r3 : 00000000 r2 : 00000000 r1 : 00000110 r0 : 00000000 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 3e08c04a DAC: 00000051 Process kmsfb-manage (pid: 102, stack limit = 0xae04e210) Stack: (0xae04fc70 to 0xae050000) fc60: 043ce660 00000001 0000009e 043ce660 fc80: 00000002 00000000 00000000 af75cf50 00001009 ae23f440 00000001 ae246418 fca0: 8155a210 ae8ea308 8093c364 ae2464e0 ae04fcec ae04fcc0 804ef350 8051a79c fcc0: 00000004 00000004 ae23f440 af3f9000 ae881818 8155a210 af1af200 ae8ea020 fce0: ae04fd1c ae04fcf0 80519124 804ef060 ae04fd34 00000000 00000000 00000000 fd00: ae881818 ae23f440 80d4ec8c 00000000 ae04fd34 ae04fd20 804f00b4 80518fac fd20: ae23f440 00000000 ae04fd54 ae04fd38 804f2190 804f0074 ae23f440 af3f9000 fd40: ae04fdd4 ae881818 ae04fd6c ae04fd58 80516390 804f20f4 ae23f440 00000000 fd60: ae04fd8c ae04fd70 804f26f4 80516348 ae23a000 ae881818 00000001 af3f9000 fd80: ae04fdac ae04fd90 80502c58 804f2678 ae04fe50 ae23f400 00000001 af3f9000 fda0: ae04fe1c ae04fdb0 80507a1c 80502bf8 ae23a000 ae058c40 af1af200 ae23f400 fdc0: ae23a000 af3f9000 ae881818 ae23a00c 80176c7c ae23a000 ae881818 af1af200 fde0: 00000000 00000000 ae23f400 00000001 ae04fe1c 00000051 ae04fe50 8155a210 fe00: 80932060 c06864a2 af3f9000 ae246200 ae04fefc ae04fe20 804f9718 805074e8 fe20: ae04feac ae04fe30 80177360 8017631c 805074dc 00000068 00000068 00000062 fe40: 00000068 000000a2 ae04fe50 7ef29688 7ef29c40 00000000 00000001 00000018 fe60: 00000026 00000000 00000000 00000000 00000001 000115bc 05010500 05a0059f fe80: 03200000 03360321 00000337 0000003c 00000000 00000040 30383231 30303878 fea0: 00000000 00000000 00000000 00000000 00000000 00000000 00004000 aea6a140 fec0: 00000000 80d77b71 00000000 80283110 600f0013 7ef29688 af342bb0 ae250b40 fee0: 80275440 00000003 ae04e000 00000000 ae04ff7c ae04ff00 80274ac8 804f957c ff00: 80283128 80179030 00000000 00000000 80282fd8 ae1e0000 0000003d aea6a1d0 ff20: 00000002 00000003 00004000 007f8c60 c06864a2 7ef29688 ae04e000 00000000 ff40: ae04ff6c ae04ff50 80283260 80282fe4 00017050 ae250b41 00000003 ae250b40 ff60: c06864a2 7ef29688 ae04e000 00000000 ae04ffa4 ae04ff80 80275440 80274a20 ff80: 00017050 00000001 007f8c60 00000036 801088a4 ae04e000 00000000 ae04ffa8 ffa0: 80108700 80275408 00017050 00000001 00000003 c06864a2 7ef29688 000115bc ffc0: 00017050 00000001 007f8c60 00000036 00000003 00000000 00000026 00000018 ffe0: 00016f28 7ef29684 0000b7d9 76e4a1e6 400f0030 00000003 3ff7e861 3ff7ec61 Backtrace: [<8051a790>] (imx_ldb_encoder_mode_set) from [<804ef350>] (drm_atomic_helper_commit_modeset_disables+0x2fc/0x3f0) r10:ae2464e0 r9:8093c364 r8:ae8ea308 r7:8155a210 r6:ae246418 r5:00000001 r4:ae23f440 [<804ef054>] (drm_atomic_helper_commit_modeset_disables) from [<80519124>] (imx_drm_atomic_commit_tail+0x184/0x1e0) r10:ae8ea020 r9:af1af200 r8:8155a210 r7:ae881818 r6:af3f9000 r5:ae23f440 r4:00000004 r3:00000004 [<80518fa0>] (imx_drm_atomic_commit_tail) from [<804f00b4>] (commit_tail+0x4c/0x68) r6:00000000 r5:80d4ec8c r4:ae23f440 [<804f0068>] (commit_tail) from [<804f2190>] (drm_atomic_helper_commit+0xa8/0xd4) r5:00000000 r4:ae23f440 [<804f20e8>] (drm_atomic_helper_commit) from [<80516390>] (drm_atomic_commit+0x54/0x74) r7:ae881818 r6:ae04fdd4 r5:af3f9000 r4:ae23f440 [<8051633c>] (drm_atomic_commit) from [<804f26f4>] (drm_atomic_helper_set_config+0x88/0xac) r5:00000000 r4:ae23f440 [<804f266c>] (drm_atomic_helper_set_config) from [<80502c58>] (drm_mode_set_config_internal+0x6c/0xf4) r7:af3f9000 r6:00000001 r5:ae881818 r4:ae23a000 [<80502bec>] (drm_mode_set_config_internal) from [<80507a1c>] (drm_mode_setcrtc+0x540/0x5b8) r7:af3f9000 r6:00000001 r5:ae23f400 r4:ae04fe50 [<805074dc>] (drm_mode_setcrtc) from [<804f9718>] (drm_ioctl+0x1a8/0x46c) r10:ae246200 r9:af3f9000 r8:c06864a2 r7:80932060 r6:8155a210 r5:ae04fe50 r4:00000051 [<804f9570>] (drm_ioctl) from [<80274ac8>] (do_vfs_ioctl+0xb4/0x9e8) r10:00000000 r9:ae04e000 r8:00000003 r7:80275440 r6:ae250b40 r5:af342bb0 r4:7ef29688 [<80274a14>] (do_vfs_ioctl) from [<80275440>] (SyS_ioctl+0x44/0x6c) r10:00000000 r9:ae04e000 r8:7ef29688 r7:c06864a2 r6:ae250b40 r5:00000003 r4:ae250b41 [<802753fc>] (SyS_ioctl) from [<80108700>] (ret_fast_syscall+0x0/0x1c) r9:ae04e000 r8:801088a4 r7:00000036 r6:007f8c60 r5:00000001 r4:00017050 Code: 1a000018 e596e034 e59e3368 e59331bc (e5930010) ---[ end trace 464e7d3c7f4b9706 ]---
Instead of trying to walk only the connectors in atomic state to which we don't have access, just walk all connectors to find one connected to the current encoder and containing a bus_format description.
Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state") Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/imx/imx-ldb.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index dc2b420..5abed7b 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -298,15 +298,13 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder, }
if (!bus_format) { - struct drm_connector_state *conn_state; + struct drm_crtc_state *crtc_state = encoder->crtc->state; struct drm_connector *connector; - int i;
- for_each_connector_in_state(encoder->crtc->state->state, - connector, conn_state, i) { + drm_for_each_connector(connector, encoder->dev) { struct drm_display_info *di = &connector->display_info;
- if (conn_state->crtc == encoder->crtc && + if (connector->state->crtc == encoder->crtc && di->num_bus_formats) { bus_format = di->bus_formats[0]; break;
On Fri, Jul 22, 2016 at 10:59:51AM +0200, Philipp Zabel wrote:
The code in imx_ldb_encoder_mode_set crashes with a NULL pointer dereference trying to access crtc->state->state, which was previously cleared by drm_atomic_helper_swap_state:
Unable to handle kernel NULL pointer dereference at virtual address 00000010 pgd = ae08c000 [00000010] *pgd=3e00e831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 PID: 102 Comm: kmsfb-manage Not tainted 4.7.0-rc5+ #232 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) task: ae058c40 ti: ae04e000 task.ti: ae04e000 PC is at imx_ldb_encoder_mode_set+0x138/0x2f8 LR is at 0xae881818 pc : [<8051a8c8>] lr : [<ae881818>] psr: 600f0013 sp : ae04fc70 ip : ae04fbb0 fp : ae04fcbc r10: ae8ea018 r9 : 00000000 r8 : ae246418 r7 : ae8ea010 r6 : ae8ea308 r5 : 00000000 r4 : 00000000 r3 : 00000000 r2 : 00000000 r1 : 00000110 r0 : 00000000 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 3e08c04a DAC: 00000051 Process kmsfb-manage (pid: 102, stack limit = 0xae04e210) Stack: (0xae04fc70 to 0xae050000) fc60: 043ce660 00000001 0000009e 043ce660 fc80: 00000002 00000000 00000000 af75cf50 00001009 ae23f440 00000001 ae246418 fca0: 8155a210 ae8ea308 8093c364 ae2464e0 ae04fcec ae04fcc0 804ef350 8051a79c fcc0: 00000004 00000004 ae23f440 af3f9000 ae881818 8155a210 af1af200 ae8ea020 fce0: ae04fd1c ae04fcf0 80519124 804ef060 ae04fd34 00000000 00000000 00000000 fd00: ae881818 ae23f440 80d4ec8c 00000000 ae04fd34 ae04fd20 804f00b4 80518fac fd20: ae23f440 00000000 ae04fd54 ae04fd38 804f2190 804f0074 ae23f440 af3f9000 fd40: ae04fdd4 ae881818 ae04fd6c ae04fd58 80516390 804f20f4 ae23f440 00000000 fd60: ae04fd8c ae04fd70 804f26f4 80516348 ae23a000 ae881818 00000001 af3f9000 fd80: ae04fdac ae04fd90 80502c58 804f2678 ae04fe50 ae23f400 00000001 af3f9000 fda0: ae04fe1c ae04fdb0 80507a1c 80502bf8 ae23a000 ae058c40 af1af200 ae23f400 fdc0: ae23a000 af3f9000 ae881818 ae23a00c 80176c7c ae23a000 ae881818 af1af200 fde0: 00000000 00000000 ae23f400 00000001 ae04fe1c 00000051 ae04fe50 8155a210 fe00: 80932060 c06864a2 af3f9000 ae246200 ae04fefc ae04fe20 804f9718 805074e8 fe20: ae04feac ae04fe30 80177360 8017631c 805074dc 00000068 00000068 00000062 fe40: 00000068 000000a2 ae04fe50 7ef29688 7ef29c40 00000000 00000001 00000018 fe60: 00000026 00000000 00000000 00000000 00000001 000115bc 05010500 05a0059f fe80: 03200000 03360321 00000337 0000003c 00000000 00000040 30383231 30303878 fea0: 00000000 00000000 00000000 00000000 00000000 00000000 00004000 aea6a140 fec0: 00000000 80d77b71 00000000 80283110 600f0013 7ef29688 af342bb0 ae250b40 fee0: 80275440 00000003 ae04e000 00000000 ae04ff7c ae04ff00 80274ac8 804f957c ff00: 80283128 80179030 00000000 00000000 80282fd8 ae1e0000 0000003d aea6a1d0 ff20: 00000002 00000003 00004000 007f8c60 c06864a2 7ef29688 ae04e000 00000000 ff40: ae04ff6c ae04ff50 80283260 80282fe4 00017050 ae250b41 00000003 ae250b40 ff60: c06864a2 7ef29688 ae04e000 00000000 ae04ffa4 ae04ff80 80275440 80274a20 ff80: 00017050 00000001 007f8c60 00000036 801088a4 ae04e000 00000000 ae04ffa8 ffa0: 80108700 80275408 00017050 00000001 00000003 c06864a2 7ef29688 000115bc ffc0: 00017050 00000001 007f8c60 00000036 00000003 00000000 00000026 00000018 ffe0: 00016f28 7ef29684 0000b7d9 76e4a1e6 400f0030 00000003 3ff7e861 3ff7ec61 Backtrace: [<8051a790>] (imx_ldb_encoder_mode_set) from [<804ef350>] (drm_atomic_helper_commit_modeset_disables+0x2fc/0x3f0) r10:ae2464e0 r9:8093c364 r8:ae8ea308 r7:8155a210 r6:ae246418 r5:00000001 r4:ae23f440 [<804ef054>] (drm_atomic_helper_commit_modeset_disables) from [<80519124>] (imx_drm_atomic_commit_tail+0x184/0x1e0) r10:ae8ea020 r9:af1af200 r8:8155a210 r7:ae881818 r6:af3f9000 r5:ae23f440 r4:00000004 r3:00000004 [<80518fa0>] (imx_drm_atomic_commit_tail) from [<804f00b4>] (commit_tail+0x4c/0x68) r6:00000000 r5:80d4ec8c r4:ae23f440 [<804f0068>] (commit_tail) from [<804f2190>] (drm_atomic_helper_commit+0xa8/0xd4) r5:00000000 r4:ae23f440 [<804f20e8>] (drm_atomic_helper_commit) from [<80516390>] (drm_atomic_commit+0x54/0x74) r7:ae881818 r6:ae04fdd4 r5:af3f9000 r4:ae23f440 [<8051633c>] (drm_atomic_commit) from [<804f26f4>] (drm_atomic_helper_set_config+0x88/0xac) r5:00000000 r4:ae23f440 [<804f266c>] (drm_atomic_helper_set_config) from [<80502c58>] (drm_mode_set_config_internal+0x6c/0xf4) r7:af3f9000 r6:00000001 r5:ae881818 r4:ae23a000 [<80502bec>] (drm_mode_set_config_internal) from [<80507a1c>] (drm_mode_setcrtc+0x540/0x5b8) r7:af3f9000 r6:00000001 r5:ae23f400 r4:ae04fe50 [<805074dc>] (drm_mode_setcrtc) from [<804f9718>] (drm_ioctl+0x1a8/0x46c) r10:ae246200 r9:af3f9000 r8:c06864a2 r7:80932060 r6:8155a210 r5:ae04fe50 r4:00000051 [<804f9570>] (drm_ioctl) from [<80274ac8>] (do_vfs_ioctl+0xb4/0x9e8) r10:00000000 r9:ae04e000 r8:00000003 r7:80275440 r6:ae250b40 r5:af342bb0 r4:7ef29688 [<80274a14>] (do_vfs_ioctl) from [<80275440>] (SyS_ioctl+0x44/0x6c) r10:00000000 r9:ae04e000 r8:7ef29688 r7:c06864a2 r6:ae250b40 r5:00000003 r4:ae250b41 [<802753fc>] (SyS_ioctl) from [<80108700>] (ret_fast_syscall+0x0/0x1c) r9:ae04e000 r8:801088a4 r7:00000036 r6:007f8c60 r5:00000001 r4:00017050 Code: 1a000018 e596e034 e59e3368 e59331bc (e5930010) ---[ end trace 464e7d3c7f4b9706 ]---Instead of trying to walk only the connectors in atomic state to which we don't have access, just walk all connectors to find one connected to the current encoder and containing a bus_format description.
Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state") Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
drivers/gpu/drm/imx/imx-ldb.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index dc2b420..5abed7b 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -298,15 +298,13 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder, }
if (!bus_format) {
struct drm_connector_state *conn_state;
struct drm_crtc_state *crtc_state = encoder->crtc->state;
Proper fix would be to roll out atomic_ versions of all teh encoder callbacks where we additionally pass both the crtc state and the connector state. Then there's no need for walking connector lists like that. And in the atomic helpers those two states are always readily available, and passing them down to callbacks is also what we will do in i915. I'd be happy to merge such a patch.
At least in general that's a problem when you can hotplug or hotremove connectors. -Daniel
struct drm_connector *connector;
int i;for_each_connector_in_state(encoder->crtc->state->state,connector, conn_state, i) {
drm_for_each_connector(connector, encoder->dev) { struct drm_display_info *di = &connector->display_info;
if (conn_state->crtc == encoder->crtc &&
if (connector->state->crtc == encoder->crtc && di->num_bus_formats) { bus_format = di->bus_formats[0]; break;-- 2.8.1
Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: [...]
Proper fix would be to roll out atomic_ versions of all teh encoder callbacks where we additionally pass both the crtc state and the connector state. Then there's no need for walking connector lists like that. And in the atomic helpers those two states are always readily available, and passing them down to callbacks is also what we will do in i915. I'd be happy to merge such a patch.
Thanks, that is a good idea. Which encoder callbacks besides mode_set -> atomic_mode_set are you thinking of, though? enable/disable?
regards Philipp
On Fri, Jul 22, 2016 at 12:57:15PM +0200, Philipp Zabel wrote:
Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: [...]
Proper fix would be to roll out atomic_ versions of all teh encoder callbacks where we additionally pass both the crtc state and the connector state. Then there's no need for walking connector lists like that. And in the atomic helpers those two states are always readily available, and passing them down to callbacks is also what we will do in i915. I'd be happy to merge such a patch.
Thanks, that is a good idea. Which encoder callbacks besides mode_set -> atomic_mode_set are you thinking of, though? enable/disable?
Yes. The only other one is mode_fixup, and we already have atomic_check as the fancy replacement for that. Please align the parameter ordering with encoder->atomic_check for consistency.
Also when you do that patch, pls don't forget the vtable kerneldoc comments, and make sure there's no warnings/issues and it renders nicely using
$ make htmldocs
Since 4.8 will feature a new sphinx-based toolchain, pls run that command on top of linux-next (for the latest sphinx fixes from docs-next). -Daniel
Am Freitag, den 22.07.2016, 18:49 +0200 schrieb Daniel Vetter:
On Fri, Jul 22, 2016 at 12:57:15PM +0200, Philipp Zabel wrote:
Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: [...]
Proper fix would be to roll out atomic_ versions of all teh encoder callbacks where we additionally pass both the crtc state and the connector state. Then there's no need for walking connector lists like that. And in the atomic helpers those two states are always readily available, and passing them down to callbacks is also what we will do in i915. I'd be happy to merge such a patch.
Thanks, that is a good idea. Which encoder callbacks besides mode_set -> atomic_mode_set are you thinking of, though? enable/disable?
Yes. The only other one is mode_fixup, and we already have atomic_check as the fancy replacement for that. Please align the parameter ordering with encoder->atomic_check for consistency.
Which crtc state should be passed to encoder atomic_disable? old_crtc_state?
Also when you do that patch, pls don't forget the vtable kerneldoc comments, and make sure there's no warnings/issues and it renders nicely using
$ make htmldocs
Since 4.8 will feature a new sphinx-based toolchain, pls run that command on top of linux-next (for the latest sphinx fixes from docs-next). -Daniel
Ok, I'll have a look.
thanks Philipp
On Mon, Jul 25, 2016 at 12:42:50PM +0200, Philipp Zabel wrote:
Am Freitag, den 22.07.2016, 18:49 +0200 schrieb Daniel Vetter:
On Fri, Jul 22, 2016 at 12:57:15PM +0200, Philipp Zabel wrote:
Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: [...]
Proper fix would be to roll out atomic_ versions of all teh encoder callbacks where we additionally pass both the crtc state and the connector state. Then there's no need for walking connector lists like that. And in the atomic helpers those two states are always readily available, and passing them down to callbacks is also what we will do in i915. I'd be happy to merge such a patch.
Thanks, that is a good idea. Which encoder callbacks besides mode_set -> atomic_mode_set are you thinking of, though? enable/disable?
Yes. The only other one is mode_fixup, and we already have atomic_check as the fancy replacement for that. Please align the parameter ordering with encoder->atomic_check for consistency.
Which crtc state should be passed to encoder atomic_disable? old_crtc_state?
Yes.
Also when you do that patch, pls don't forget the vtable kerneldoc comments, and make sure there's no warnings/issues and it renders nicely using
$ make htmldocs
Since 4.8 will feature a new sphinx-based toolchain, pls run that command on top of linux-next (for the latest sphinx fixes from docs-next). -Daniel
Ok, I'll have a look.
Otoh, if there's not yet a need then maybe it doesn't yet make sense to add the full set of hooks. Might be worth it to skim driver's encoder callbacks for connector loops and similar things. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Philipp Zabel