Re: [PATCH 05/24] drm/msm: route the display snapshot through the msm_display interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] TOCTOU race condition checking `dp->power_on` locklessly, which can lead to an unclocked MMIO read and system crash.
--

--- Patch [5]: [PATCH 05/24] drm/msm: route the display snapshot through the msm_display interface ---
commit ccee48fbd111a360e4e514435d662edafb46b06b
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: route the display snapshot through the msm_display interface

This commit refactors the display snapshot code to use a mandatory snapshot()
callback in struct msm_display_funcs. DSI and DP snapshot helpers are
turned into implementations of this callback, and an empty one is provided
for HDMI.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 79dd556e96c27..f4fa86d1b9c77 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -783,8 +783,10 @@ int msm_dp_display_get_test_bpp(struct msm_dp *dp)
>  		msm_dp_display->link->test_video.test_bit_depth);
>  }
>  
> -void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
> +static void msm_dp_snapshot(struct msm_display *display,
> +			    struct msm_disp_state *disp_state)
>  {
> +	struct msm_dp *dp = container_of(display, struct msm_dp, display);
>  	struct msm_dp_display_private *msm_dp_display;

[Severity: High]
This is a pre-existing issue, but could the lockless check of dp->power_on
further down in msm_dp_snapshot() lead to an unclocked MMIO read?

If a concurrent modeset disable or hotplug unplug event disables the DP
controller's clocks and sets dp->power_on to false right after the lockless
if (!dp->power_on) check, the snapshot thread might proceed to read the
hardware registers in msm_disp_snapshot_add_block() without the clocks.

Reading MMIO registers while the DP clocks are disabled causes an AHB bus
hang, leading to a fatal system crash (SError) on Snapdragon hardware.

Should pm_runtime_get_sync() be used around the hardware access here,
similar to how it is handled in msm_dsi_host_snapshot()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=5