[PATCH] drm: vc4: Fix race during binding

[Stefan Wahren]

> Eric Anholt <eric at anholt.net> hat am 8. Oktober 2017 um 19:09 geschrieben:
> 
> 
> Stefan Wahren <stefan.wahren at i2se.com> writes:
> 
> > Hi Eric,
> >
> >> Eric Anholt <eric at anholt.net> hat am 6. Oktober 2017 um 21:42 geschrieben:
> >> 
> >> 
> >> Stefan Wahren <stefan.wahren at i2se.com> writes:
> >> 
> >> > This fixes the race between vc4_overflow_mem_work and the init of the
> >> > job lock. Otherwise we could trigger a NULL pointer dereference
> >> > during VC4 binding.
> >> >
> >> > Link: https://github.com/anholt/linux/issues/114
> >> > Signed-off-by: Stefan Wahren <stefan.wahren at i2se.com>
> >> > Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
> >> > ---
> >> >  drivers/gpu/drm/vc4/vc4_gem.c | 1 -
> >> >  drivers/gpu/drm/vc4/vc4_irq.c | 1 +
> >> >  2 files changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> >> > index d0c6bfb..47d964f 100644
> >> > --- a/drivers/gpu/drm/vc4/vc4_gem.c
> >> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> >> > @@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev)
> >> >  	INIT_LIST_HEAD(&vc4->render_job_list);
> >> >  	INIT_LIST_HEAD(&vc4->job_done_list);
> >> >  	INIT_LIST_HEAD(&vc4->seqno_cb_list);
> >> > -	spin_lock_init(&vc4->job_lock);
> >> >  
> >> >  	INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
> >> >  	setup_timer(&vc4->hangcheck.timer,
> >> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> >> > index 7d7af3a..d508d13 100644
> >> > --- a/drivers/gpu/drm/vc4/vc4_irq.c
> >> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> >> > @@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev)
> >> >  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> >> >  
> >> >  	init_waitqueue_head(&vc4->job_wait_queue);
> >> > +	spin_lock_init(&vc4->job_lock);
> >> >  	INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
> >> >  
> >> >  	/* Clear any pending interrupts someone might have left around
> >> 
> >> Are you sure this is a fix?  We don't attach the IRQ handler until V3D
> >> bind, and vc4_gem_init happens before component_bind_all(), right?
> >
> > i agree i should have mark it as a RFC. But is it really impossible
> > that vc4_overflow_mem_work is triggered before vc4_gem_init?
> 
> As far as I can see, it gets queued from the IRQ handler, the IRQ
> handler is added during V3D bind, and binding happens after GEM init.
> 
> Hmm.  If we fail out of component binding and try again, we'll end up
> doing the job_wait_queue and overflow_mem_work initialization on the
> same pointer twice.  Will that cause any trouble?  We cancel any pending
> work during uninstall (V3D unbind path), but does drm_irq_uninstall()
> make sure that the irq handler has finished?

I cannot see an issue.