Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Unchecked clk_prepare_enable() return values can lead to synchronous external aborts during MMIO access or unbalanced clock disables. - [High] The patch explicitly turns off `pclk` and `axi_clk` during probe, which can break the inherited display pipeline. - [High] Inherited display clocks are leaked indefinitely if DRM components fail to bind, if the CRTC is disabled without being enabled first, or if userspace never initiates a modeset. Pre-existing issues: - [High] Calling sleeping functions from atomic context in CRTC atomic enable callback. -- commit deec425702ee01b881e4c0a6e40e6ec4f167024f Author: Alexandre MINETTE <[email protected]> drm/msm/mdp4: keep inherited display clocks enabled until modeset This patch detects if the MDP4 display pipeline was already enabled by firmware and keeps the required display clocks prepared to prevent panel glitches. It drops the extra clock references on the first DRM CRTC enable when the driver takes ownership. > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > @@ -297,6 +297,7 @@ static void mdp4_crtc_atomic_enable(struct drm_crtc *crtc, > > mdp4_enable(mdp4_kms); > mdp4_crtc_bus_get(mdp4_kms); > + mdp4_inherited_display_clocks_put(mdp4_kms); [Severity: High] Will the inherited clock references be leaked if the display components never bind or if userspace never initiates a modeset? If msm_drv_probe() successfully adds the master component but the required display components fail to bind, the mdp4_destroy() callback is never executed. If userspace never initiates a modeset or explicitly disables the display first, atomic_enable is never reached. In these cases, it appears the driver holds explicit clk_prepare_enable references that will bypass the clock framework's late initcalls, leaving the hardware fully powered on indefinitely. [Severity: High] This is a pre-existing issue, but does adding this call here worsen a violation of atomic context rules? CRTC atomic enable callbacks run in atomic context during non-blocking commits, where sleeping is strictly forbidden. The mdp4_crtc_atomic_enable() function already contained sleeping calls like mdp4_enable(), and this patch adds mdp4_inherited_display_clocks_put() which acquires a mutex and calls clk_disable_unprepare(). Both of these operations can sleep. Could this lead to kernel might_sleep() warnings or deadlocks during non-blocking commits? > > /* Restore vblank irq handling after power is enabled */ > drm_crtc_vblank_on(crtc); [ ... ] > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c [ ... ] > +static void mdp4_inherited_display_clocks_get(struct mdp4_kms *mdp4_kms) > +{ > + bool inherited_display; > + > + mutex_lock(&mdp4_kms->clock_lock); > + > + clk_prepare_enable(mdp4_kms->clk); > + clk_prepare_enable(mdp4_kms->pclk); > + clk_prepare_enable(mdp4_kms->lut_clk); > + clk_prepare_enable(mdp4_kms->axi_clk); [Severity: High] Is it safe to proceed with hardware MMIO reads if any of these clocks fail to enable? The return values from clk_prepare_enable() are completely ignored here. If enabling the core or bus clock fails, the driver will proceed to perform MMIO reads in mdp4_has_inherited_display_state() on unclocked hardware. On MSM platforms, accessing unclocked registers typically triggers an unhandled synchronous external abort, resulting in a kernel panic. Should these return values be checked and handled? > + > + inherited_display = mdp4_has_inherited_display_state(mdp4_kms); > + > + if (inherited_display) { > + clk_prepare_enable(mdp4_kms->clk); > + clk_prepare_enable(mdp4_kms->lut_clk); > + mdp4_kms->inherited_display_clocks_held = true; > + } > + > + clk_disable_unprepare(mdp4_kms->clk); > + clk_disable_unprepare(mdp4_kms->pclk); > + clk_disable_unprepare(mdp4_kms->lut_clk); > + clk_disable_unprepare(mdp4_kms->axi_clk); [Severity: High] Will unconditionally disabling pclk and axi_clk here break the inherited display pipeline? The Linux Common Clock Framework initializes clocks left on by the bootloader with a software usage count of 0. By temporarily enabling and then disabling them, the usage count goes from 0 to 1 and back to 0. This triggers the framework to physically turn off the hardware clocks immediately. Disabling the pixel clock (pclk) and memory bus clock (axi_clk) while the hardware is actively scanning out a frame will typically starve the pipeline, causing bus hangs, underflows, or the exact blank screen this patch aims to prevent. Should the active state of these clocks be preserved if an inherited display is detected? > + > + mutex_unlock(&mdp4_kms->clock_lock); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260611-mainline-fix-mdp4-blue-screen-sending-v1-v1-0-22925600e...@alex-min.fr?part=2
