[Intel-gfx] [PATCH 2/2] drm/i915: Make use of intel_fb_obj()

[Chris Wilson]

On Tue, Jul 08, 2014 at 11:51:18AM +0200, Daniel Vetter wrote:
> On Tue, Jul 08, 2014 at 07:47:13AM +0100, Chris Wilson wrote:
> > On Mon, Jul 07, 2014 at 06:21:48PM -0700, Matt Roper wrote:
> > > This should hopefully simplify the display code slightly and also
> > > solves at least one mistake in intel_pipe_set_base() where
> > > to_intel_framebuffer(fb)->obj is referenced during local variable
> > > initialization, before 'if (!fb)' gets checked.
> > > 
> > > Potential uses of this macro were identified via the following
> > > Coccinelle patch:
> > > 
> > >         @@
> > >         expression E;
> > >         @@
> > >         * to_intel_framebuffer(E)->obj
> > > 
> > >         @@
> > >         expression E;
> > >         identifier I;
> > >         @@
> > >           I = to_intel_framebuffer(E);
> > >           ...
> > >         * I->obj
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > 
> > The only drawback is that I am suddenly nervous of potential NULL
> > objs...
> 
> I concur with Chris here, I think intel_fb_obj should be a static inline
> and first check for fb == NULL. To catch abuse we could do an
> 
> if (WARN_ON(!fb))
> 	return NULL;
> 
> return to_intel_framebuffer(fb)->obj;
> 
> The most likely abuse is that we call intel_fb_obj before checking fb for
> NULL, so the WARN is better than a BUG_ON. With that I don't think we need
> to change the checks as Chris suggested here.

This came about because of one path where we should have expected fb to
be NULL. Having a WARN followed by a GPF isn't any better than the GPF,
in which case this patch is superfluous and you would rather just fix
the single callsite where the bug occurred.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre