On Fri, Jan 13, 2017 at 03:12:07PM +0000, Daniel Stone wrote:
> Hi Dan,
>
> On 13 January 2017 at 12:56, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > drivers/gpu/drm/drm_crtc.c:392 drm_mode_getcrtc()
> > error: we previously assumed 'crtc->primary->state' could be null (see line 384)
> >
> > drivers/gpu/drm/drm_crtc.c
> > 383
> > 384 if (crtc->primary->state && crtc->primary->state->fb)
> > ^^^^^^^^^^^^^^^^^^^^
> > New check for NULL.
> >
> > 385 crtc_resp->fb_id = crtc->primary->state->fb->base.id;
> > 386 else if (!crtc->primary->state && crtc->primary->fb)
> > 387 crtc_resp->fb_id = crtc->primary->fb->base.id;
> > 388 else
> > 389 crtc_resp->fb_id = 0;
> > 390
> > 391 if (crtc->state) {
> > 392 crtc_resp->x = crtc->primary->state->src_x >> 16;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Old unchecked dereference. It's possible that non-NULL "crtc->state"
> > implies a non-NULL "crtc->primary->state", but I didn't spot the
> > relationship immediately.
>
> Thanks for this. I believe this is indeed an invariant; Dan Vetter
> could confirm, if he's managed to find internet in Hobart. Assuming
> this is the case, what's the best way to communicate this to smatch;
> would that be through a BUG_ON or similar?
Yeah, we assume that if a driver uses state stuff, it's used everywhere.
But I'm not sure anymore how much that holds true for a driver
transitioning to atomic, so adding a BUG_ON to tell smatch or extend the
if check to also check for crtc->primary->state (it wont make a diffrence
for atomic drivers) if the BUG_ON doesn't clue in smatch enough.
Can you pls submit the right patch, since I don't have a working smatch
setup here?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
