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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxx>
> >> 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@xxxxxxxxx>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> ---
> >> 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:
pgp5rYMYkbpsV.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
