[Intel-gfx] [PATCH 4/5] drm: Push drm_global_mutex locking in drm_open

[Daniel Vetter]

On Wed, Jan 29, 2020 at 05:45:45PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> 
> On Wed, Jan 29, 2020 at 09:24:09AM +0100, Daniel Vetter wrote:
> > We want to only take the BKL on crap drivers, but to know whether
> The BKL was killed long time ago..
> In other words I was confused until I realized that
> - BKL
> - drm_global_mutex BKL
> - drm_global_mutex
> 
> Was all the same. At least my OCD color me confused as is.

The Real BKL was converted into drm_global_mutex for drm when the BKL was
killed. Which is kinda relevant, because the BKL locking horrors (at least
in drm) have been inherited by drm_global_mutex (and the conversion broke
a few things that had to be papered over). Hence the motivation to finally
clean up the locking and figure out what exactly is still protected by
this lock. If you're bored, all this is at least in modern git history
afaics.
-Daniel

> 
> > we have a crap driver we first need to look it up. Split this shuffle
> > out from the main BKL-disabling patch, for more clarity.
> > 
> > Since the minors are refcounted drm_minor_acquire is purely internal
> > and this does not have a driver visible effect.
> > 
> > v2: Push the locking even further into drm_open(), suggested by Chris.
> > This gives us more symmetry with drm_release(), and maybe a futuer
> > avenue where we make drm_globale_mutex locking (partially) opt-in like
> s/drm_globale_mutex/drm_global_mutex/
> 
> > with drm_release_noglobal().
> > 
> > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> Above is IMO fix-while-committing stuff.
> 
> 	Sam
> 
> > ---
> >  drivers/gpu/drm/drm_drv.c  | 14 +++++---------
> >  drivers/gpu/drm/drm_file.c |  6 ++++++
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 8deff75b484c..05bdf0b9d2b3 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -1085,17 +1085,14 @@ static int drm_stub_open(struct inode *inode, struct file *filp)
> >  
> >  	DRM_DEBUG("\n");
> >  
> > -	mutex_lock(&drm_global_mutex);
> >  	minor = drm_minor_acquire(iminor(inode));
> > -	if (IS_ERR(minor)) {
> > -		err = PTR_ERR(minor);
> > -		goto out_unlock;
> > -	}
> > +	if (IS_ERR(minor))
> > +		return PTR_ERR(minor);
> >  
> >  	new_fops = fops_get(minor->dev->driver->fops);
> >  	if (!new_fops) {
> >  		err = -ENODEV;
> > -		goto out_release;
> > +		goto out;
> >  	}
> >  
> >  	replace_fops(filp, new_fops);
> > @@ -1104,10 +1101,9 @@ static int drm_stub_open(struct inode *inode, struct file *filp)
> >  	else
> >  		err = 0;
> >  
> > -out_release:
> > +out:
> >  	drm_minor_release(minor);
> > -out_unlock:
> > -	mutex_unlock(&drm_global_mutex);
> > +
> >  	return err;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 1075b3a8b5b1..d36cb74ebe0c 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -378,6 +378,8 @@ int drm_open(struct inode *inode, struct file *filp)
> >  	if (IS_ERR(minor))
> >  		return PTR_ERR(minor);
> >  
> > +	mutex_unlock(&drm_global_mutex);
> > +
> >  	dev = minor->dev;
> >  	if (!atomic_fetch_inc(&dev->open_count))
> >  		need_setup = 1;
> > @@ -395,10 +397,14 @@ int drm_open(struct inode *inode, struct file *filp)
> >  			goto err_undo;
> >  		}
> >  	}
> > +
> > +	mutex_unlock(&drm_global_mutex);
> > +
> >  	return 0;
> >  
> >  err_undo:
> >  	atomic_dec(&dev->open_count);
> > +	mutex_unlock(&drm_global_mutex);
> >  	drm_minor_release(minor);
> >  	return retcode;
> >  }
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch