| Message ID | 1436769501-4105-13-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thierry
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
diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c index 397fb34..a2d66f9 100644 --- a/drivers/gpu/drm/tegra/fb.c +++ b/drivers/gpu/drm/tegra/fb.c @@ -184,9 +184,9 @@ unreference: #ifdef CONFIG_DRM_TEGRA_FBDEV static struct fb_ops tegra_fb_ops = { .owner = THIS_MODULE, - .fb_fillrect = sys_fillrect, - .fb_copyarea = sys_copyarea, - .fb_imageblit = sys_imageblit, + .fb_fillrect = drm_fb_helper_sys_fillrect, + .fb_copyarea = drm_fb_helper_sys_copyarea, + .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_check_var = drm_fb_helper_check_var, .fb_set_par = drm_fb_helper_set_par, .fb_blank = drm_fb_helper_blank, @@ -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); return err; } @@ -347,20 +342,9 @@ fini: static void tegra_fbdev_exit(struct tegra_fbdev *fbdev) { - struct fb_info *info = fbdev->base.fbdev; - - if (info) { - int err; - err = unregister_framebuffer(info); - if (err < 0) - DRM_DEBUG_KMS("failed to unregister framebuffer\n"); - - if (info->cmap.len) - fb_dealloc_cmap(&info->cmap); - - framebuffer_release(info); - } + drm_fb_helper_unregister_fbi(&fbdev->base); + drm_fb_helper_release_fbi(&fbdev->base); if (fbdev->fb) { drm_framebuffer_unregister_private(&fbdev->fb->base);
Use the newly created wrapper drm_fb_helper functions instead of calling core fbdev functions directly. They also simplify the fb_info creation. COMPILE TESTED ONLY. Cc: Thierry Reding <thierry.reding@gmail.com> Cc: "Terje Bergström" <tbergstrom@nvidia.com> Cc: Stephen Warren <swarren@wwwdotorg.org> Signed-off-by: Archit Taneja <architt@codeaurora.org> --- drivers/gpu/drm/tegra/fb.c | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-)