Re: [PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Abhinav Kumar <quic_abhinavk@quicinc.com>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>
Cc: Stephen Boyd <swboyd@chromium.org>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error
Date: Thu, 8 Sep 2022 17:47:54 +0300	[thread overview]
Message-ID: <f1d8736c-2e05-885c-aebe-0e89e3139eb5@linaro.org> (raw)
In-Reply-To: <4f4862fd-4cbd-82ac-f162-e38c05c92423@quicinc.com>

On 08/09/2022 03:08, Abhinav Kumar wrote:
> 
> 
> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
>> If worker creation fails, nullify the event_thread->worker, so that
>> msm_drm_uninit() doesn't try accessing invalid memory location. While we
>> are at it, remove duplicate assignment to the ret variable.
>>
>> Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of 
>> kthread_run")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> the change itself LGTM,
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> One minor nit below
>> ---
>>   drivers/gpu/drm/msm/msm_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
>> b/drivers/gpu/drm/msm/msm_drv.c
>> index 44485363f37a..1aab6bf86278 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const 
>> struct drm_driver *drv)
>>           if (IS_ERR(priv->event_thread[i].worker)) {
>>               ret = PTR_ERR(priv->event_thread[i].worker);
>>               DRM_DEV_ERROR(dev, "failed to create crtc_event 
>> kthread\n");
> 
> Can we print ret in this error message?

In a separate change. I'll add it to my todo list.

> 
>> -            ret = PTR_ERR(priv->event_thread[i].worker);
>> +            priv->event_thread[i].worker = NULL;
>>               goto err_msm_uninit;
>>           }

-- 
With best wishes
Dmitry