Re: [PATCH 5/8] drm: trylock modest locking for fbdev panics

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 30, 2014 at 08:56:07AM -0700, Matt Roper wrote:
> On Tue, Jul 29, 2014 at 11:32:20PM +0200, Daniel Vetter wrote:
> > In the fbdev code we want to do trylocks only to avoid deadlocks and
> > other ugly issues. Thus far we've only grabbed the overall modeset
> > lock, but that already failed to exclude a pile of potential
> > concurrent operations. With proper atomic support this will be worse.
> > 
> > So add a trylock mode to the modeset locking code which attempts all
> > locks only with trylocks, if possible. We need to track this in the
> > locking functions themselves and can't restrict this to drivers since
> > driver-private w/w mutexes must be treated the same way.
> > 
> > There's still the issue that other driver private locks aren't handled
> > here at all, but well can't have everything. With this we will at
> > least not regress, even once atomic allows lots of concurrent kms
> > activity.
> > 
> > Aside: We should move the acquire context to stack-based allocation in
> > the callers to get rid of that awful WARN_ON(kmalloc_failed) control
> > flow which just blows up when memory is short. But that's material for
> > separate patches.
> > 
> > v2:
> > - Fix logic inversion fumble in the fb helper.
> > - Add proper kerneldoc.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c    | 10 +++----
> >  drivers/gpu/drm/drm_modeset_lock.c | 56 ++++++++++++++++++++++++++++++--------
> >  include/drm/drm_modeset_lock.h     |  6 ++++
> >  3 files changed, 55 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 3144db9dc0f1..841e039ba028 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void)
> >  		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  			continue;
> >  
> > -		/* NOTE: we use lockless flag below to avoid grabbing other
> > -		 * modeset locks.  So just trylock the underlying mutex
> > -		 * directly:
> > +		/*
> > +		 * NOTE: Use trylock mode to avoid deadlocks and sleeping in
> > +		 * panic context.
> >  		 */
> > -		if (!mutex_trylock(&dev->mode_config.mutex)) {
> > +		if (__drm_modeset_lock_all(dev, true) != 0) {
> >  			error = true;
> >  			continue;
> >  		}
> 
> Can we succeed locking config->mutex and connection_mutex inside
> __drm_modeset_lock_all(), but then fail to lock one of the CRTC mutexes
> in drm_modeset_lock_all_crtcs()?  If so, __drm_modeset_lock_all() will
> return -EBUSY, but not drop the locks it acquired, and then it seems
> like we can return from the function here without ever dropping locks.

Well it's the panic code, so after this the machine is officially dead
anyway. The only thing left to do is get the dmesg out with as little risk
as possible.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch