Re: [PATCH v2 3/4] drm/vc4: Use the reset controller to recover from a GPU hang

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/vc4: UAF in vc4_job_done_work() due to missing hardware reset
- [Medium] drm/vc4: component re-bind fails due to devm_reset_control in bind
--

commit 8ebe4ef2282246bb3aa856ccf103a302ccfae13f
Author: Maíra Canal <mcanal@igalia.com>

drm/vc4: Use the reset controller to recover from a GPU hang

This patch switches the GPU recovery mechanism from a runtime PM power cycle
to using a dedicated V3D reset line. It also leaves the runtime PM wrapper
functions as simple passthroughs and makes the reset line optional for older
device trees.

> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index e231c906709c1..3212b91676201 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
[ ... ]
> @@ -292,19 +292,22 @@ vc4_save_hang_state(struct drm_device *dev)
>  static void
>  vc4_reset(struct drm_device *dev)
>  {
> -	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct vc4_v3d *v3d = to_vc4_dev(dev)->v3d;
> +	int ret;
>  
> -	DRM_INFO("Resetting GPU.\n");
> +	vc4_irq_disable(dev);
>  
> -	mutex_lock(&vc4->power_lock);
> -	if (vc4->power_refcount) {
> -		/* Power the device off and back on the by dropping the
> -		 * reference on runtime PM.
> -		 */
> -		pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev);
> -		pm_runtime_get_sync(&vc4->v3d->pdev->dev);
> +	if (v3d->reset) {
> +		drm_info(dev, "Resetting GPU.\n");
> +
> +		ret = reset_control_reset(v3d->reset);
> +		if (ret)
> +			drm_err(dev, "Failed to reset the GPU: %d\n", ret);
> +
> +		vc4_v3d_init_hw(dev);
> +	} else {
> +		drm_info_once(dev, "No reset line; GPU state is not reset.\n");
>  	}
> -	mutex_unlock(&vc4->power_lock);
>  
>  	vc4_irq_reset(dev);

[Severity: High]
Does this create a use-after-free on older device trees where v3d->reset is
NULL? 

By removing the runtime PM suspend/resume cycle, the hardware is no longer
reset on older platforms. However, the driver continues to unconditionally
call vc4_irq_reset() which cancels jobs and finishes render jobs:

vc4_irq_reset()
  vc4_cancel_bin_job(dev)
  vc4_irq_finish_render_job(dev)

These routines signal the job fences and schedule vc4_job_done_work, which
frees the GEM memory. If the GPU hardware is still executing because the
reset was skipped, won't it asynchronously access this freed memory when it
eventually makes progress?

[ ... ]
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index a86739873e05b..b40d98c9d1d28 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
[ ... ]
> @@ -447,6 +427,14 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
>  	if (IS_ERR(v3d->clk))
>  		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
>  
> +	v3d->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(v3d->reset))
> +		return dev_err_probe(dev, PTR_ERR(v3d->reset),
> +				     "Failed to get reset control\n");
> +
> +	if (!v3d->reset)
> +		drm_warn(drm, "No V3D reset line in the device tree");
> +
>  	ret = platform_get_irq(pdev, 0);

[Severity: Medium]
Will this cause component re-bind operations to fail?

Since devm allocations are tied to the device's initial probe lifecycle rather
than the component bind/unbind lifecycle, the exclusive reset control is not
released when the component is unbound. 

If the DRM master device is unbound and then rebound, wouldn't a subsequent
bind attempt fail with -EBUSY because the leaked initial allocation still
holds the exclusive lock on the reset line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-vc4-reset-control-v2-0-cb3a25b07822@igalia.com?part=3