| Message ID | 20200128104602.1459802-3-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm: Complain if drivers still use the ->load callback | expand |
Quoting Daniel Vetter (2020-01-28 10:46:00) > We want to only take the BKL on crap drivers, but to know whether > 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 > with drm_release_noglobal(). > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > 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); The only reason why I could think it was in drm_stub_open() not drm_open() was for the possibility of some driver using a different callback. Such a driver would not be partaking in the drm_global_mutex so... Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
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; }
We want to only take the BKL on crap drivers, but to know whether 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 with drm_release_noglobal(). Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_drv.c | 14 +++++--------- drivers/gpu/drm/drm_file.c | 6 ++++++ 2 files changed, 11 insertions(+), 9 deletions(-)