Re: [PATCH v2 08/13] drm/loongson: Move hardware reset to CRTC creation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `lsdc_crtc1_soft_reset` calls `msleep(20)`, which can sleep, but it is reachable from the CRTC atomic enable callback (`lsdc_crtc_atomic_enable`), which runs in atomic context.
--

commit 4cb085e0fbbbf2cb0c2550f2c69cc0175ba5ecf8
Author: Maxime Ripard <mripard@kernel.org>

drm/loongson: Move hardware reset to CRTC creation

This patch moves the hardware-level CRTC reset from the atomic state
initialization hook to the CRTC creation path. It adds a new hw_reset
callback to handle the register writes independently.

> diff --git a/drivers/gpu/drm/loongson/lsdc_crtc.c b/drivers/gpu/drm/loongson/lsdc_crtc.c
> index 16b8f36a40710..7cca574e3b6af 100644
> --- a/drivers/gpu/drm/loongson/lsdc_crtc.c
> +++ b/drivers/gpu/drm/loongson/lsdc_crtc.c

[ ... ]

> @@ -937,6 +932,15 @@ static bool lsdc_crtc_get_scanout_position(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +static void lsdc_crtc_hw_reset(struct drm_crtc *crtc)
> +{
> +	struct lsdc_crtc *lcrtc = to_lsdc_crtc(crtc);
> +
> +	/* Reset the CRTC hardware, this is required for S3 support */
> +	if (lcrtc->hw_ops->reset)
> +		lcrtc->hw_ops->reset(lcrtc);
> +}
> +

While reviewing the hardware reset paths in this file, I noticed a pre-existing
issue nearby:

drivers/gpu/drm/loongson/lsdc_crtc.c:
> static void lsdc_crtc1_soft_reset(struct lsdc_crtc *lcrtc)
> {
[ ... ]
> 	/* Wait about a vblank time */
> 	msleep(20);
> }

[Severity: High]
This isn't a bug introduced by this patch, but can this msleep() be called
during atomic context?

The DRM core calls the CRTC .atomic_enable callback, which executes in atomic
context:

lsdc_crtc_atomic_enable()
  lcrtc->hw_ops->enable(lcrtc) // resolves to lsdc_crtc1_enable()

If a hardware stall is detected, it attempts error recovery:

lsdc_crtc1_enable() {
    ...
    if (val & CRTC_ANCHORED) {
        drm_warn(&ldev->base, "%s stall\n", lcrtc->base.name);
        return lsdc_crtc1_soft_reset(lcrtc);
    }
}

This calls lsdc_crtc1_soft_reset(), which erroneously calls msleep(20).

Sleeping in atomic context violates core kernel locking rules. This triggers
might_sleep() warnings and can lead to deadlocks or panics. (Note that
lsdc_crtc0_soft_reset() correctly uses mdelay(20) instead).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-drm-no-more-crtc-reset-v2-0-4500d49a337f@kernel.org?part=8