diff mbox series

[RFT,v2,15/48] drm/panel: ltk050h3146w: Don't call unprepare+disable at shutdown/remove

Message ID 20240503143327.RFT.v2.15.Ibeb2e5692e34b136afe4cf55532f0696ab3f5eed@changeid (mailing list archive)
State New, archived
Headers show
Series drm/panel: Remove most store/double-check of prepared/enabled state | expand

Commit Message

Douglas Anderson May 3, 2024, 9:32 p.m. UTC 
It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

Unfortunately, grepping mainline for this panel's compatible string
shows no hits, so we can't be 100% sure if the DRM modeset driver used
with this panel has been fixed. If it is found that the DRM modeset
driver hasn't been fixed then this patch could be temporarily reverted
until it is.

[1] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org
[2] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org
[3] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org

Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

 .../gpu/drm/panel/panel-leadtek-ltk050h3146w.c  | 17 -----------------
 1 file changed, 17 deletions(-)

Comments

Quentin Schulz May 6, 2024, 2:04 p.m. UTC | #1 
Hi Douglas,

On 5/3/24 11:32 PM, Douglas Anderson wrote:
> [You don't often get email from dianders@chromium.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> It's the responsibility of a correctly written DRM modeset driver to
> call drm_atomic_helper_shutdown() at shutdown time and that should be
> disabling / unpreparing the panel if needed. Panel drivers shouldn't
> be calling these functions themselves.
> 
> A recent effort was made to fix as many DRM modeset drivers as
> possible [1] [2] [3] and most drivers are fixed now.
> 
> Unfortunately, grepping mainline for this panel's compatible string
> shows no hits, so we can't be 100% sure if the DRM modeset driver used
> with this panel has been fixed. If it is found that the DRM modeset
> driver hasn't been fixed then this patch could be temporarily reverted
> until it is.
> 
> [1] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org
> [2] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org
> [3] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org
> 
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

I get:

"""
[   27.239362] ------------[ cut here ]------------
[   27.244549] refcount_t: addition on 0; use-after-free.
[   27.250308] WARNING: CPU: 4 PID: 1 at lib/refcount.c:25 
refcount_warn_saturate+0x120/0x144
[   27.259556] Modules linked in: snd_soc_simple_card crct10dif_ce 
snd_soc_simple_card_utils fuse [last unloaded: panel_leadtek_ltk050h3146w]
[   27.273470] CPU: 4 PID: 1 Comm: systemd-shutdow Not tainted 
6.9.0-rc7-00002-g4a8eaebfcfde-dirty #63
[   27.283584] Hardware name: Theobroma Systems RK3399-Q7 SoM on Haikou 
devkit with Video Demo adapter (DT)
[   27.294180] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[   27.301962] pc : refcount_warn_saturate+0x120/0x144
[   27.307411] lr : refcount_warn_saturate+0x120/0x144
[   27.312860] sp : ffff800081babb10
[   27.316559] x29: ffff800081babb10 x28: ffff000000640000 x27: 
0000000000000000
[   27.324539] x26: ffff8000814847f8 x25: 0000000000000001 x24: 
ffff800081adb028
[   27.332519] x23: ffff000000e13090 x22: ffff800081b3c280 x21: 
ffff0000006c8680
[   27.340489] x20: ffff0000006c8680 x19: ffff00000a6f8000 x18: 
0000000000000006
[   27.348468] x17: 000000040044ffff x16: 00500074b5503510 x15: 
ffff800081bab580
[   27.356447] x14: 0000000000000000 x13: ffff8000819944d0 x12: 
0000000000000a2f
[   27.364426] x11: 0000000000000365 x10: ffff8000819ec4d0 x9 : 
ffff8000819944d0
[   27.372396] x8 : 00000000ffffefff x7 : ffff8000819ec4d0 x6 : 
80000000fffff000
[   27.380375] x5 : 0000000000000366 x4 : 0000000000000000 x3 : 
0000000000000000
[   27.388353] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff000000640000
[   27.396332] Call trace:
[   27.399061]  refcount_warn_saturate+0x120/0x144
[   27.404122]  drm_dev_get+0x78/0x7c
[   27.407924]  drm_atomic_state_init+0x78/0xd0
[   27.412695]  drm_atomic_state_alloc+0x6c/0x9c
[   27.417563]  drm_atomic_helper_disable_all+0x20/0x214
[   27.423208]  drm_atomic_helper_shutdown+0xa4/0x148
[   27.428560]  rockchip_drm_platform_shutdown+0x18/0x28
[   27.434207]  platform_shutdown+0x24/0x34
[   27.438589]  device_shutdown+0x150/0x258
[   27.442973]  kernel_power_off+0x38/0x7c
[   27.447260]  __do_sys_reboot+0x204/0x24c
[   27.451643]  __arm64_sys_reboot+0x24/0x30
[   27.456122]  invoke_syscall+0x48/0x114
[   27.460311]  el0_svc_common.constprop.0+0x40/0xe0
[   27.465567]  do_el0_svc+0x1c/0x28
[   27.469269]  el0_svc+0x34/0xd8
[   27.472681]  el0t_64_sync_handler+0x120/0x12c
[   27.477548]  el0t_64_sync+0x190/0x194
[   27.481639] ---[ end trace 0000000000000000 ]---
[   27.486831] ------------[ cut here ]------------
[   27.491995] refcount_t: underflow; use-after-free.
[   27.497414] WARNING: CPU: 4 PID: 1 at lib/refcount.c:28 
refcount_warn_saturate+0xf4/0x144
[   27.506558] Modules linked in: snd_soc_simple_card crct10dif_ce 
snd_soc_simple_card_utils fuse [last unloaded: panel_leadtek_ltk050h3146w]
[   27.520468] CPU: 4 PID: 1 Comm: systemd-shutdow Tainted: G        W 
        6.9.0-rc7-00002-g4a8eaebfcfde-dirty #63
[   27.532230] Hardware name: Theobroma Systems RK3399-Q7 SoM on Haikou 
devkit with Video Demo adapter (DT)
[   27.542826] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[   27.550607] pc : refcount_warn_saturate+0xf4/0x144
[   27.555958] lr : refcount_warn_saturate+0xf4/0x144
[   27.561309] sp : ffff800081babb10
[   27.565008] x29: ffff800081babb10 x28: ffff000000640000 x27: 
0000000000000000
[   27.572988] x26: ffff8000814847f8 x25: 0000000000000001 x24: 
ffff800081adb028
[   27.580958] x23: ffff000000e13090 x22: ffff00000a6f82e0 x21: 
ffff00000a6f8170
[   27.588928] x20: ffff00000a6f8000 x19: ffff00000a6f8000 x18: 
0000000000000006
[   27.596907] x17: 000000040044ffff x16: 00500074b5503510 x15: 
072007200720072e
[   27.604877] x14: 0000000000000000 x13: 0000000000000046 x12: 
0000000000000000
[   27.612847] x11: 0000000000000000 x10: 0000000000000a20 x9 : 
ffff800081bab980
[   27.620826] x8 : ffff000000640a80 x7 : ffff0000054e1000 x6 : 
00000000ffffffff
[   27.628796] x5 : 00000000410fd080 x4 : 0000000000000002 x3 : 
0000000000000000
[   27.636775] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff000000640000
[   27.644754] Call trace:
[   27.647482]  refcount_warn_saturate+0xf4/0x144
[   27.652445]  drm_dev_put.part.0+0xb0/0xc0
[   27.656925]  drm_dev_put+0x14/0x24
[   27.660722]  __drm_atomic_state_free+0xbc/0xd0
[   27.665687]  drm_atomic_helper_disable_all+0x108/0x214
[   27.671429]  drm_atomic_helper_shutdown+0xa4/0x148
[   27.676781]  rockchip_drm_platform_shutdown+0x18/0x28
[   27.682425]  platform_shutdown+0x24/0x34
[   27.686807]  device_shutdown+0x150/0x258
[   27.691190]  kernel_power_off+0x38/0x7c
[   27.695475]  __do_sys_reboot+0x204/0x24c
[   27.699858]  __arm64_sys_reboot+0x24/0x30
[   27.704337]  invoke_syscall+0x48/0x114
[   27.708525]  el0_svc_common.constprop.0+0x40/0xe0
[   27.713782]  do_el0_svc+0x1c/0x28
[   27.717484]  el0_svc+0x34/0xd8
[   27.720894]  el0t_64_sync_handler+0x120/0x12c
[   27.725762]  el0t_64_sync+0x190/0x194
[   27.729851] ---[ end trace 0000000000000000 ]---
"""

when running "poweroff" after doing a modprobe -r of the driver on 
RK3399 Puma and PX30 Ringneck (the trace comes from RK3399 Puma). It is 
fine if I still have the device probed, no trace when powering off.

BUT, I have the same trace in the 6.9-rc7 already, so I guess:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3399 Puma with 
Haikou Video Demo
Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # PX30 Ringneck 
with Haikou Video Demo

I'll let Heiko test this on RK3588 Tiger with Haikou Video Demo to check 
the whole stack is ready there as well.

Thanks!
Quentin
Douglas Anderson May 6, 2024, 3:17 p.m. UTC | #2 
Hi,

On Mon, May 6, 2024 at 7:04 AM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Douglas,
>
> On 5/3/24 11:32 PM, Douglas Anderson wrote:
> > [You don't often get email from dianders@chromium.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > It's the responsibility of a correctly written DRM modeset driver to
> > call drm_atomic_helper_shutdown() at shutdown time and that should be
> > disabling / unpreparing the panel if needed. Panel drivers shouldn't
> > be calling these functions themselves.
> >
> > A recent effort was made to fix as many DRM modeset drivers as
> > possible [1] [2] [3] and most drivers are fixed now.
> >
> > Unfortunately, grepping mainline for this panel's compatible string
> > shows no hits, so we can't be 100% sure if the DRM modeset driver used
> > with this panel has been fixed. If it is found that the DRM modeset
> > driver hasn't been fixed then this patch could be temporarily reverted
> > until it is.
> >
> > [1] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org
> > [2] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org
> > [3] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org
> >
> > Cc: "Heiko Stübner" <heiko@sntech.de>
> > Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> I get:
>
> """
> [   27.239362] ------------[ cut here ]------------
> [   27.244549] refcount_t: addition on 0; use-after-free.
> [   27.250308] WARNING: CPU: 4 PID: 1 at lib/refcount.c:25
> refcount_warn_saturate+0x120/0x144
> [   27.259556] Modules linked in: snd_soc_simple_card crct10dif_ce
> snd_soc_simple_card_utils fuse [last unloaded: panel_leadtek_ltk050h3146w]
> [   27.273470] CPU: 4 PID: 1 Comm: systemd-shutdow Not tainted
> 6.9.0-rc7-00002-g4a8eaebfcfde-dirty #63
> [   27.283584] Hardware name: Theobroma Systems RK3399-Q7 SoM on Haikou
> devkit with Video Demo adapter (DT)
> [   27.294180] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [   27.301962] pc : refcount_warn_saturate+0x120/0x144
> [   27.307411] lr : refcount_warn_saturate+0x120/0x144
> [   27.312860] sp : ffff800081babb10
> [   27.316559] x29: ffff800081babb10 x28: ffff000000640000 x27:
> 0000000000000000
> [   27.324539] x26: ffff8000814847f8 x25: 0000000000000001 x24:
> ffff800081adb028
> [   27.332519] x23: ffff000000e13090 x22: ffff800081b3c280 x21:
> ffff0000006c8680
> [   27.340489] x20: ffff0000006c8680 x19: ffff00000a6f8000 x18:
> 0000000000000006
> [   27.348468] x17: 000000040044ffff x16: 00500074b5503510 x15:
> ffff800081bab580
> [   27.356447] x14: 0000000000000000 x13: ffff8000819944d0 x12:
> 0000000000000a2f
> [   27.364426] x11: 0000000000000365 x10: ffff8000819ec4d0 x9 :
> ffff8000819944d0
> [   27.372396] x8 : 00000000ffffefff x7 : ffff8000819ec4d0 x6 :
> 80000000fffff000
> [   27.380375] x5 : 0000000000000366 x4 : 0000000000000000 x3 :
> 0000000000000000
> [   27.388353] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
> ffff000000640000
> [   27.396332] Call trace:
> [   27.399061]  refcount_warn_saturate+0x120/0x144
> [   27.404122]  drm_dev_get+0x78/0x7c
> [   27.407924]  drm_atomic_state_init+0x78/0xd0
> [   27.412695]  drm_atomic_state_alloc+0x6c/0x9c
> [   27.417563]  drm_atomic_helper_disable_all+0x20/0x214
> [   27.423208]  drm_atomic_helper_shutdown+0xa4/0x148
> [   27.428560]  rockchip_drm_platform_shutdown+0x18/0x28
> [   27.434207]  platform_shutdown+0x24/0x34
> [   27.438589]  device_shutdown+0x150/0x258
> [   27.442973]  kernel_power_off+0x38/0x7c
> [   27.447260]  __do_sys_reboot+0x204/0x24c
> [   27.451643]  __arm64_sys_reboot+0x24/0x30
> [   27.456122]  invoke_syscall+0x48/0x114
> [   27.460311]  el0_svc_common.constprop.0+0x40/0xe0
> [   27.465567]  do_el0_svc+0x1c/0x28
> [   27.469269]  el0_svc+0x34/0xd8
> [   27.472681]  el0t_64_sync_handler+0x120/0x12c
> [   27.477548]  el0t_64_sync+0x190/0x194
> [   27.481639] ---[ end trace 0000000000000000 ]---
> [   27.486831] ------------[ cut here ]------------
> [   27.491995] refcount_t: underflow; use-after-free.
> [   27.497414] WARNING: CPU: 4 PID: 1 at lib/refcount.c:28
> refcount_warn_saturate+0xf4/0x144
> [   27.506558] Modules linked in: snd_soc_simple_card crct10dif_ce
> snd_soc_simple_card_utils fuse [last unloaded: panel_leadtek_ltk050h3146w]
> [   27.520468] CPU: 4 PID: 1 Comm: systemd-shutdow Tainted: G        W
>         6.9.0-rc7-00002-g4a8eaebfcfde-dirty #63
> [   27.532230] Hardware name: Theobroma Systems RK3399-Q7 SoM on Haikou
> devkit with Video Demo adapter (DT)
> [   27.542826] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [   27.550607] pc : refcount_warn_saturate+0xf4/0x144
> [   27.555958] lr : refcount_warn_saturate+0xf4/0x144
> [   27.561309] sp : ffff800081babb10
> [   27.565008] x29: ffff800081babb10 x28: ffff000000640000 x27:
> 0000000000000000
> [   27.572988] x26: ffff8000814847f8 x25: 0000000000000001 x24:
> ffff800081adb028
> [   27.580958] x23: ffff000000e13090 x22: ffff00000a6f82e0 x21:
> ffff00000a6f8170
> [   27.588928] x20: ffff00000a6f8000 x19: ffff00000a6f8000 x18:
> 0000000000000006
> [   27.596907] x17: 000000040044ffff x16: 00500074b5503510 x15:
> 072007200720072e
> [   27.604877] x14: 0000000000000000 x13: 0000000000000046 x12:
> 0000000000000000
> [   27.612847] x11: 0000000000000000 x10: 0000000000000a20 x9 :
> ffff800081bab980
> [   27.620826] x8 : ffff000000640a80 x7 : ffff0000054e1000 x6 :
> 00000000ffffffff
> [   27.628796] x5 : 00000000410fd080 x4 : 0000000000000002 x3 :
> 0000000000000000
> [   27.636775] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
> ffff000000640000
> [   27.644754] Call trace:
> [   27.647482]  refcount_warn_saturate+0xf4/0x144
> [   27.652445]  drm_dev_put.part.0+0xb0/0xc0
> [   27.656925]  drm_dev_put+0x14/0x24
> [   27.660722]  __drm_atomic_state_free+0xbc/0xd0
> [   27.665687]  drm_atomic_helper_disable_all+0x108/0x214
> [   27.671429]  drm_atomic_helper_shutdown+0xa4/0x148
> [   27.676781]  rockchip_drm_platform_shutdown+0x18/0x28
> [   27.682425]  platform_shutdown+0x24/0x34
> [   27.686807]  device_shutdown+0x150/0x258
> [   27.691190]  kernel_power_off+0x38/0x7c
> [   27.695475]  __do_sys_reboot+0x204/0x24c
> [   27.699858]  __arm64_sys_reboot+0x24/0x30
> [   27.704337]  invoke_syscall+0x48/0x114
> [   27.708525]  el0_svc_common.constprop.0+0x40/0xe0
> [   27.713782]  do_el0_svc+0x1c/0x28
> [   27.717484]  el0_svc+0x34/0xd8
> [   27.720894]  el0t_64_sync_handler+0x120/0x12c
> [   27.725762]  el0t_64_sync+0x190/0x194
> [   27.729851] ---[ end trace 0000000000000000 ]---
> """
>
> when running "poweroff" after doing a modprobe -r of the driver on
> RK3399 Puma and PX30 Ringneck (the trace comes from RK3399 Puma). It is
> fine if I still have the device probed, no trace when powering off.

I'm not too surprised there. It's _really_ hard to successfully
"remove" DRM devices that are used on systems that use the component
model to get everything up and running. My series doesn't attempt to
make this better but it also shouldn't make it worse. It does point to
the fact that probably many of the remove() functions in DRM panels
are never exercised...


> BUT, I have the same trace in the 6.9-rc7 already, so I guess:
>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3399 Puma with
> Haikou Video Demo
> Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # PX30 Ringneck
> with Haikou Video Demo
>
> I'll let Heiko test this on RK3588 Tiger with Haikou Video Demo to check
> the whole stack is ready there as well.

Thank you for reviewing / testing! :-)

-Doug
Heiko Stübner May 6, 2024, 4:21 p.m. UTC | #3 
Am Freitag, 3. Mai 2024, 23:32:56 CEST schrieb Douglas Anderson:
> It's the responsibility of a correctly written DRM modeset driver to
> call drm_atomic_helper_shutdown() at shutdown time and that should be
> disabling / unpreparing the panel if needed. Panel drivers shouldn't
> be calling these functions themselves.
> 
> A recent effort was made to fix as many DRM modeset drivers as
> possible [1] [2] [3] and most drivers are fixed now.
> 
> Unfortunately, grepping mainline for this panel's compatible string
> shows no hits, so we can't be 100% sure if the DRM modeset driver used
> with this panel has been fixed. If it is found that the DRM modeset
> driver hasn't been fixed then this patch could be temporarily reverted
> until it is.
> 
> [1] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org
> [2] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org
> [3] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org
> 
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

On a rk3588-tiger with WIP DSI patches and this display

Tested-by: Heiko Stuebner <heiko@sntech.de>

Before this patch on reboot the system emits
[  181.464230] panel-leadtek-ltk050h3146w fde20000.dsi.0: Skipping disable of already disabled panel
[  181.465056] panel-leadtek-ltk050h3146w fde20000.dsi.0: Skipping unprepare of already unprepared panel

after applying this patch, those lines are gone.

Also the patch itself looks good
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c b/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c
index 5894bf30524a..292aa26a456d 100644
--- a/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c
+++ b/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c
@@ -673,27 +673,11 @@  static int ltk050h3146w_probe(struct mipi_dsi_device *dsi)
 	return 0;
 }
 
-static void ltk050h3146w_shutdown(struct mipi_dsi_device *dsi)
-{
-	struct ltk050h3146w *ctx = mipi_dsi_get_drvdata(dsi);
-	int ret;
-
-	ret = drm_panel_unprepare(&ctx->panel);
-	if (ret < 0)
-		dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
-
-	ret = drm_panel_disable(&ctx->panel);
-	if (ret < 0)
-		dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
-}
-
 static void ltk050h3146w_remove(struct mipi_dsi_device *dsi)
 {
 	struct ltk050h3146w *ctx = mipi_dsi_get_drvdata(dsi);
 	int ret;
 
-	ltk050h3146w_shutdown(dsi);
-
 	ret = mipi_dsi_detach(dsi);
 	if (ret < 0)
 		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
@@ -725,7 +709,6 @@  static struct mipi_dsi_driver ltk050h3146w_driver = {
 	},
 	.probe	= ltk050h3146w_probe,
 	.remove = ltk050h3146w_remove,
-	.shutdown = ltk050h3146w_shutdown,
 };
 module_mipi_dsi_driver(ltk050h3146w_driver);