Re: [PATCH v2 04/13] drm/tilcdc: Move hardware reset to CRTC creation

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Unused variable `priv` causes a compiler warning/error.

Pre-existing issues:
- [High] Missing explicit cancellation of `recover_work` leads to a potential Use-After-Free.
- [High] Missing return value check for `pm_runtime_get_sync()` before accessing hardware registers.
--

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

drm/tilcdc: Move hardware reset to CRTC creation

The tilcdc crtc reset hook performs both atomic state initialization
using drm_atomic_helper_crtc_reset() and hardware-level register
writes to disable the raster and clear IRQ status.

The hardware reset is not related to atomic state initialization, and
the reset hook is being converted to atomic_create_state which only
deals with state allocation. Move the hardware reset to the CRTC
creation path instead.

> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index e07ecb36a27a8..b299d73ce3856 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
[ ... ]
> @@ -685,14 +685,12 @@ static void tilcdc_crtc_disable_vblank(struct drm_crtc *crtc)
>  	spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
>  }
>  
> -static void tilcdc_crtc_reset(struct drm_crtc *crtc)
> +static void tilcdc_crtc_hw_reset(struct drm_crtc *crtc)
>  {
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	int ret;
>  
> -	drm_atomic_helper_crtc_reset(crtc);
> -
>  	/* Turn the raster off if it for some reason is on. */
>  	pm_runtime_get_sync(dev->dev);
>  	if (tilcdc_read(dev, LCDC_RASTER_CTRL_REG) & LCDC_RASTER_ENABLE) {

[Severity: High]
This is a pre-existing issue, but is it safe to proceed with reading
LCDC_RASTER_CTRL_REG if pm_runtime_get_sync() fails?

If pm_runtime_get_sync() returns a negative error code (e.g. -EACCES) due to
a resume failure, the hardware may remain powered off. Accessing MMIO
registers on a powered-off or suspended device can cause a bus fault on ARM
platforms, leading to a synchronous external abort kernel panic.

Should we check the return value of pm_runtime_get_sync() here before
accessing the hardware?

[ ... ]
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 1d6c9a423a410..827f15d14fad9 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -249,9 +249,11 @@ static int tilcdc_pm_suspend(struct device *dev)
>  static int tilcdc_pm_resume(struct device *dev)
>  {
>  	struct drm_device *ddev = dev_get_drvdata(dev);
> +	struct tilcdc_drm_private *priv = ddev_to_tilcdc_priv(ddev);

[Severity: Low]
Does this code leave the priv variable unused?

It appears priv is declared and assigned here but never referenced,
which can cause a compiler warning.

>  
>  	/* Select default pin state */
>  	pinctrl_pm_select_default_state(dev);
> +
>  	return  drm_mode_config_helper_resume(ddev);
>  }

I also noticed a separate regression outside the patch context in the driver
teardown path:

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that the driver
teardown fails to cancel the recover_work?

Looking at tilcdc_crtc_destroy() in tilcdc_crtc.c, the workqueue is flushed:

drivers/gpu/drm/tilcdc/tilcdc_crtc.c:tilcdc_crtc_destroy() {
    ...
	tilcdc_crtc_shutdown(priv->crtc);

	flush_workqueue(priv->wq);

	of_node_put(priv->crtc->port);
}

However, recover_work is queued on system_percpu_wq during sync loss events 
in tilcdc_crtc_irq(), not on priv->wq. Since there is no explicit 
cancel_work_sync(&tilcdc_crtc->recover_work) before the tilcdc_crtc 
structure is ultimately freed, could this lead to a use-after-free if the 
work executes after the CRTC state is destroyed?

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