Re: [PATCH] drm/cma-helper: fixup compilation

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2286 bytes --]

On Tue, Feb 19, 2013 at 11:16:08AM +0100, Daniel Vetter wrote:
> On Tue, Feb 19, 2013 at 8:43 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Fri, Feb 15, 2013 at 11:24:35AM +0100, Daniel Vetter wrote:
> >> /me grabs a few brown paper bags
> >>
> >> So it looks like I've broken compilation in
> >>
> >> commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069
> >> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Date:   Sun Jan 20 17:32:21 2013 +0100
> >>
> >>     drm: review locking for drm_fb_helper_restore_fbdev_mode
> >>
> >> Fix it up again.
> >>
> >> Reported-by: Wu Fengguang <fengguang.wu@intel.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> ---
> >>  drivers/gpu/drm/drm_fb_cma_helper.c |    2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> index e851658..ef68e34 100644
> >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> @@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
> >>   */
> >>  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma)
> >>  {
> >> +     struct drm_device *dev = fbdev_cma->fb_helper.dev;
> >> +
> >>       drm_modeset_lock_all(dev);
> >>       if (fbdev_cma)
> >>               drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper);
> >
> > The above check indicates that fbdev_cma might be NULL, so you're
> > potentially dereferencing NULL when assigning the dev variable.
> 
> Right, looks like a need more brown-paper bags.
> 
> > Perhaps a better way would be to move the locking into the if block, as
> > in the patch below.
> 
> Otoh I think it's a bit funny that you can pass NULL to restore_mode
> and hotplug_event in the cma fb helper code. Imo it would be better to
> just ditch those checks ... Laurent? I'll update the patch meanwhile.

I agree, it is probably safe to drop the checks and consider it a driver
error (and let it oops accordingly) if NULL is ever passed in. I expect
drivers will usually bail out if drm_fbdev_cma_init() fails. If they
choose to continue maybe they should be responsible for not calling
drm_fbdev_cma_restore_mode() if fbdev is NULL.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel