[Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation

[Chris Wilson]

On Fri, Feb 05, 2016 at 01:27:10AM +0100, Lukas Wunner wrote:
> Hi,
> 
> On Thu, Feb 04, 2016 at 09:21:17AM +0000, Li, Weinan Z wrote:
> > We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> > failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> > initialization fails") this 2 patches can???t cover this case. It???s access ifbdev->fb before the initialization
> > finished, but not initialization failed. If don???t have any other patches or code update relative, it may still have in 4.5.
> 
> Okay, I see.
> 
> > 
> > add info NULL check should be better, it is also initialized in the async queue
> > >       info = ifbdev->helper.fbdev;
> > > +     if (info == NULL)
> > > +            return false;
> > >       if (!info->screen_base)
> 
> So if there's indeed a race between fbdev initialization and the call to
> intel_fbdev_restore_mode() (on lastclose), there's more that can go awry:
> - intel_fbdev_restore_mode() dereferences ifbdev->fb->obj
> - it calls __drm_atomic_helper_set_config() which WARNs if ifbdev->fb is NULL
> - it calls drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev
> 
> Instead of checking all that it's probably simpler to call
> async_synchronize_full() at the beginning of intel_fbdev_restore_mode(),
> as Li Weinan suggested. I'll submit the corresponding one-liner patch
> tomorrow if noone else does.
> 
> Chris' patch also modified intel_fbdev_set_suspend() as well as
> intel_fbdev_output_poll_changed(), not sure if these are racy as well.
> At least the stack traces posted by Li Weinan and Gustav Fägerlind
> only indicate that lastclose is racy.

We called set-suspend internally, but we don't do any checks before
hand. I was concerned we may be able to get into a situation where
.fb_probe fails and leaves a dangling dev_priv->ifbdev for which we
would then trip over the NULL info->screen_base. So I was looking for a
suitable guard.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre