is this a problem? before trying to submit a solution I wanted to make sure with the experts
it seems open_count isn't locked and can cause problems
and if it is a problem, and needs to be solved, should locking be done or is it better implemented with the (as I understand standard) kref?
http://lxr.linux.no/linux+v2.6.37/drivers/gpu/drm/drm_fops.c#L121
int drm_open(struct inode *inode, struct file *filp) { ..<no lock>..
retcode = drm_open_helper(inode, filp, dev); if (!retcode) { atomic_inc(&dev->counts[_DRM_STAT_OPENS]); if (!dev->open_count++) retcode = drm_setup(dev); }
.. }
int drm_release(struct inode *inode, struct file *filp) { ..<global lock>.. if (!--dev->open_count) { if (atomic_read(&dev->ioctl_count)) { DRM_ERROR("Device busy: %d\n", atomic_read(&dev->ioctl_count)); retcode = -EBUSY; } else retcode = drm_lastclose(dev); } mutex_unlock(&drm_global_mutex); }
On Sun, 2011-02-06 at 19:24 +0200, adam zeira wrote:
is this a problem? before trying to submit a solution I wanted to make sure with the experts
it seems open_count isn't locked and can cause problems
and if it is a problem, and needs to be solved, should locking be done or is it better implemented with the (as I understand standard) kref?
Ickle?
1a72d65d6291ec248cbc5f05df2487edd714aba6 was your doing and I'm not entirely sure you actually checked all the paths looking back.
Dave.
On Tue, 08 Feb 2011 08:52:52 +1000, Dave Airlie airlied@redhat.com wrote:
On Sun, 2011-02-06 at 19:24 +0200, adam zeira wrote:
is this a problem? before trying to submit a solution I wanted to make sure with the experts
it seems open_count isn't locked and can cause problems
and if it is a problem, and needs to be solved, should locking be done or is it better implemented with the (as I understand standard) kref?
Ickle?
1a72d65d6291ec248cbc5f05df2487edd714aba6 was your doing and I'm not entirely sure you actually checked all the paths looking back.
Would this clarify matters? Perhaps there is some spare annotation that could be used here as well?
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 2ec7d48..5b4ca5b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -125,6 +125,13 @@ int drm_open(struct inode *inode, struct file *filp) struct drm_minor *minor; int retcode = 0;
+ /* Typically we are called (via the driver) from drm_stub_open() + * as their own fops->open and so already hold the global mutex. + * Enforce this. + */ + if (WARN_ON(!mutex_is_locked(&drm_global_mutex))) + return -EINVAL; + minor = idr_find(&drm_minors_idr, minor_id); if (!minor) return -ENODEV;
perfect. the whole structure's much much clearer now thanks to just that comment. atleast that did it for me
maybe move the "Typically we are called (via the driver) from drm_stub_open()" to the function annotation aswell but not strictly needed with the new comment
On Tue, Feb 8, 2011 at 1:18 AM, Chris Wilson chris@chris-wilson.co.ukwrote:
On Tue, 08 Feb 2011 08:52:52 +1000, Dave Airlie airlied@redhat.com wrote:
On Sun, 2011-02-06 at 19:24 +0200, adam zeira wrote:
is this a problem? before trying to submit a solution I wanted to make
sure with the experts
it seems open_count isn't locked and can cause problems
and if it is a problem, and needs to be solved, should locking be done
or is it better implemented with the (as I understand standard) kref?
Ickle?
1a72d65d6291ec248cbc5f05df2487edd714aba6 was your doing and I'm not entirely sure you actually checked all the paths looking back.
Would this clarify matters? Perhaps there is some spare annotation that could be used here as well?
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 2ec7d48..5b4ca5b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -125,6 +125,13 @@ int drm_open(struct inode *inode, struct file *filp) struct drm_minor *minor; int retcode = 0;
/* Typically we are called (via the driver) from drm_stub_open()* as their own fops->open and so already hold the global mutex.* Enforce this.*/if (WARN_ON(!mutex_is_locked(&drm_global_mutex)))return -EINVAL;minor = idr_find(&drm_minors_idr, minor_id); if (!minor) return -ENODEV;-- Chris Wilson, Intel Open Source Technology Centre
dri-devel@lists.freedesktop.org
-
adam zeira -
Chris Wilson -
Dave Airlie