Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Splitting `msm_dp_display_atomic_enable()` into `prepare` and `enable` phases removes the early-return mechanism on errors, leading to unclocked hardware accesses and kernel panics. --
commit 8e66b9cc12ba1b2ae6719ca3e9b5d64d23812cf9 Author: Abhinav Kumar <[email protected]> drm/msm/dp: separate dp_display_prepare() into its own API For MST, the link setup should only be done once when multiple sinks are enabled, while stream setup may run multiple times for each sink. Split the link-related preparation out of msm_dp_display_atomic_enable() so it can be called separately before the per-stream enable path. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index cdb7dd4cd7cbd..03320071fc077 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > -void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display, > - struct drm_atomic_commit *state) > +void msm_dp_display_atomic_prepare(struct msm_dp *msm_dp_display, > + struct drm_atomic_commit *state) > { [ ... ] > rc = msm_dp_display_prepare_link(dp); > - if (rc) { > + if (rc) > DRM_ERROR("DP display prepare failed, rc=%d\n", rc); > - return; > - } > +} [Severity: High] Since msm_dp_display_atomic_prepare() now returns void, doesn't this remove the early-return mechanism if msm_dp_display_prepare_link() fails? If power or clock enablement fails here, the error is logged but not propagated to the caller. [ ... ] > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index 6b8923d9dff40..4bf1a5b7c3f97 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c [ ... ] > @@ -55,7 +55,8 @@ static void msm_dp_bridge_atomic_enable(struct drm_bridge > *drm_bridge, > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); > struct msm_dp *dp = dp_bridge->msm_dp_display; > > - msm_dp_display_atomic_enable(dp, state); > + msm_dp_display_atomic_prepare(dp, state); > + msm_dp_display_atomic_enable(dp); > } [Severity: High] Will this unconditionally execute the enable phase even if the prepare phase failed? If msm_dp_display_atomic_prepare() encounters an error (like a pm_runtime failure), calling msm_dp_display_atomic_enable() immediately after might attempt to write to unclocked DP controller registers in msm_dp_ctrl_on_stream(). Can this trigger a synchronous external abort and an immediate kernel panic on Qualcomm platforms? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
