Re: [PATCH v3 01/14] drm/msm: fail the snapshot init when its worker cannot be created

[Konrad Dybcio]

On 10/24/26 1:19 AM, Dmitry Baryshkov wrote:
> msm_disp_snapshot_init() only logs a failure of kthread_run_worker() and
> returns success, leaving the error pointer in kms->dump_worker.  Both of
> its users then dereference it: msm_disp_snapshot_state() queues the dump
> work on it on the first display error, and msm_disp_snapshot_destroy()
> only checks the pointer for NULL before passing it to
> kthread_destroy_worker().
> 
> Clear the pointer and return the error instead, so that the KMS init
> fails rather than leaving a snapshot facility which crashes when used.
> 
> Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
> Assisted-by: LLM
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> index d99771684728..d1b10656e41a 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> @@ -98,6 +98,7 @@ int msm_disp_snapshot_init(struct drm_device *drm_dev)
>  {
>  	struct msm_drm_private *priv;
>  	struct msm_kms *kms;
> +	int ret;
>  
>  	if (!drm_dev) {
>  		DRM_ERROR("invalid params\n");
> @@ -110,12 +111,21 @@ int msm_disp_snapshot_init(struct drm_device *drm_dev)
>  	mutex_init(&kms->dump_mutex);
>  
>  	kms->dump_worker = kthread_run_worker(0, "%s", "disp_snapshot");
> -	if (IS_ERR(kms->dump_worker))
> +	if (IS_ERR(kms->dump_worker)) {
> +		ret = PTR_ERR(kms->dump_worker);
>  		DRM_ERROR("failed to create disp state task\n");
> +		goto err_destroy_mutex;
> +	}
>  
>  	kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
>  
>  	return 0;
> +
> +err_destroy_mutex:
> +	kms->dump_worker = NULL;
> +	mutex_destroy(&kms->dump_mutex);

LLM:

The error path calls mutex_destroy(&kms->dump_mutex), but
msm_disp_snapshot_destroy() runs later on the same failure path and
destroys the mutex again. That's harmless in practice (mutex_destroy()
on an unlocked mutex is benign, and the debug checks don't fire), but
it's a redundant double destroy. A simpler version would drop the mutex
handling from the error path and let the destroy function own it

Konrad