Re: [PATCH] drm: Use global_mutex to serialise open/lastclose

On 08/08/2010 10:37 AM, Chris Wilson wrote:

Dave Airlie: I might be missing something, but what stops the race with something reopening while we are in lastclose now?

The global_mutex which appears to fill this role is only taken inside the drm_stub_open() and not drm_open() itself. As that mutex appears to serve no other role, retask it to serialise open/lastclose.

Hmm.

But, if used this way, drm_global_mutex needs to protect *all* accesses to dev::open_count, what is count_clock used for? Can't it be removed?

/Thomas

Signed-off-by: Chris Wilsonchris@chris-wilson.co.uk

drivers/gpu/drm/drm_fops.c | 35 ++++++++++++++++++----------------- 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 6367961..71bcd1b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -123,27 +123,30 @@ int drm_open(struct inode *inode, struct file *filp) struct drm_device *dev = NULL; int minor_id = iminor(inode); struct drm_minor *minor;

• int retcode = 0;

• int retcode = -ENODEV;

• mutex_lock(&drm_global_mutex); minor = idr_find(&drm_minors_idr, minor_id); if (!minor)

• return -ENODEV;
  

• goto err;
  
  

  if (!(dev = minor->dev))

• return -ENODEV;
  

• goto err;
  
  

  retcode = drm_open_helper(inode, filp, dev);

• if (!retcode) {

• atomic_inc(&dev->counts[_DRM_STAT_OPENS]);
  

• spin_lock(&dev->count_lock);
  

• if (!dev->open_count++) {
  

• 	spin_unlock(&dev->count_lock);
  

• 	retcode = drm_setup(dev);
  

• 	goto out;
  

• }
  

• if (retcode)

• goto err;
  

• atomic_inc(&dev->counts[_DRM_STAT_OPENS]);
• spin_lock(&dev->count_lock);
• if (!dev->open_count++) { spin_unlock(&dev->count_lock);

• }

-out:

• retcode = drm_setup(dev);
  

• } else

• spin_unlock(&dev->count_lock);
  

+err:

• mutex_unlock(&drm_global_mutex);
• if (!retcode) { mutex_lock(&dev->struct_mutex); if (minor->type == DRM_MINOR_LEGACY) {

@@ -178,7 +181,6 @@ int drm_stub_open(struct inode *inode, struct file *filp)

DRM_DEBUG("\n");

• mutex_lock(&drm_global_mutex); minor = idr_find(&drm_minors_idr, minor_id); if (!minor) goto out;

@@ -198,8 +200,8 @@ int drm_stub_open(struct inode *inode, struct file *filp) } fops_put(old_fops);

• ret = 0; out:

• mutex_unlock(&drm_global_mutex); return err; }

@@ -474,8 +476,6 @@ int drm_release(struct inode *inode, struct file *filp) struct drm_device *dev = file_priv->minor->dev; int retcode = 0;

• mutex_lock(&drm_global_mutex);

• DRM_DEBUG("open_count = %d\n", dev->open_count);

  if (dev->driver->preclose)

@@ -569,6 +569,7 @@ int drm_release(struct inode *inode, struct file *filp) * End inline drm_release */

• mutex_lock(&drm_global_mutex); atomic_inc(&dev->counts[_DRM_STAT_CLOSES]); spin_lock(&dev->count_lock); if (!--dev->open_count) {