[Intel-gfx] [PATCH] drm/probe-helpers: Drop locking from poll_enable

[Daniel Vetter]

On Tue, Jan 10, 2017 at 05:10:50PM +0000, Chris Wilson wrote:
> On Tue, Jan 10, 2017 at 05:34:08PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 10, 2017 at 03:17:44PM +0000, Chris Wilson wrote:
> > > On Tue, Jan 10, 2017 at 03:29:10PM +0100, Daniel Vetter wrote:
> > > > -void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
> > > > +void drm_kms_helper_poll_enable(struct drm_device *dev)
> > > >  {
> > > >  	bool poll = false;
> > > >  	struct drm_connector *connector;
> > > >  	struct drm_connector_list_iter conn_iter;
> > > >  	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
> > > >  
> > > > -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > > > -
> > > >  	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > > >  		return;
> > > 
> > > Hmm, output_poll_execute() itself is not checking this correctly,
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 84b75709af90..cb3febc6e719 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -405,7 +405,7 @@ static void output_poll_execute(struct work_struct *work)
> > >         changed = dev->mode_config.delayed_event;
> > >         dev->mode_config.delayed_event = false;
> > >  
> > > -       if (!drm_kms_helper_poll)
> > > +       if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> > >                 goto out;
> > 
> > The cancel_work_sync in poll_disable should make this impossible, but it
> > might be a good thing to check this with a WARN_ON? Care to type that
> > patch as a follow up please?
> 
> Imagine a drm_kms_helper_poll_disable() run concurrently with
> drm_kms_helper_poll_enable(). The enable() gets past the is-enabled? check
> and begins iterating the list, meanwhile the disable() cancels the work
> and turns off the helper. The enable() is unaware of this and so
> reschedules the work, and as output_poll_execute() doesn't check for
> dev->mode_config.poll_enabled it stays active.

I thought I also added that this case is a driver bug to the kernel-docs,
but I failed. Driver needs to ensure this is all ordered reasonably well
(which it is, if you only call it from suspend/resume callbacks). I'll
respin with that fixed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch