Re: [PATCH 12/25] drm/tegra: Use new drm_fb_helper functions

From: Archit Taneja <architt@codeaurora.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: "Terje Bergström" <tbergstrom@nvidia.com>,
	"Stephen Warren" <swarren@wwwdotorg.org>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 12/25] drm/tegra: Use new drm_fb_helper functions
Date: Tue, 14 Jul 2015 14:41:58 +0530	[thread overview]
Message-ID: <55A4D25E.1050204@codeaurora.org> (raw)
In-Reply-To: <20150714085009.GD12465@ulmo.nvidia.com>

On 07/14/2015 02:20 PM, Thierry Reding wrote:
> On Mon, Jul 13, 2015 at 12:08:08PM +0530, Archit Taneja wrote:
> [...]
>> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> [...]
>> @@ -224,11 +224,11 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper,
>>   	if (IS_ERR(bo))
>>   		return PTR_ERR(bo);
>>
>> -	info = framebuffer_alloc(0, drm->dev);
>> -	if (!info) {
>> +	info = drm_fb_helper_alloc_fbi(helper);
>> +	if (IS_ERR(info)) {
>>   		dev_err(drm->dev, "failed to allocate framebuffer info\n");
>> -		drm_gem_object_unreference_unlocked(&bo->gem);
>> -		return -ENOMEM;
>> +		err = PTR_ERR(info);
>> +		goto gem_unref;
>>   	}
>>
>>   	fbdev->fb = tegra_fb_alloc(drm, &cmd, &bo, 1);
>> @@ -236,7 +236,6 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper,
>>   		err = PTR_ERR(fbdev->fb);
>>   		dev_err(drm->dev, "failed to allocate DRM framebuffer: %d\n",
>>   			err);
>> -		drm_gem_object_unreference_unlocked(&bo->gem);
>>   		goto release;
>>   	}
>>
>> @@ -248,12 +247,6 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper,
>>   	info->flags = FBINFO_FLAG_DEFAULT;
>>   	info->fbops = &tegra_fb_ops;
>>
>> -	err = fb_alloc_cmap(&info->cmap, 256, 0);
>> -	if (err < 0) {
>> -		dev_err(drm->dev, "failed to allocate color map: %d\n", err);
>> -		goto destroy;
>> -	}
>> -
>>   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
>>   	drm_fb_helper_fill_var(info, helper, fb->width, fb->height);
>>
>> @@ -282,7 +275,9 @@ destroy:
>>   	drm_framebuffer_unregister_private(fb);
>>   	tegra_fb_destroy(fb);
>>   release:
>> -	framebuffer_release(info);
>> +	drm_fb_helper_release_fbi(helper);
>> +gem_unref:
>> +	drm_gem_object_unreference_unlocked(&bo->gem);
>
> You can't do this. tegra_fb_alloc() takes ownership of the buffer, so
> unless you compensate for that (by taking an explicit reference to the
> buffer in tegra_fb_alloc()) you'd be dereferencing twice and likely end
> up with use-after-free errors later on.
>
> I'd prefer to keep the error clean up as it is.

Okay. I missed out on the fact that tegra_fb_destroy in the clean up
code calls drm_gem_object_unreference_unlocked. I'll keep it as is.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel