Re: [PATCH 05/13] drm: provide device-refcount

From: Daniel Vetter <daniel@ffwll.ch>
To: David Herrmann <dh.herrmann@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 05/13] drm: provide device-refcount
Date: Wed, 12 Feb 2014 14:25:41 +0100	[thread overview]
Message-ID: <20140212132541.GU17001@phenom.ffwll.local> (raw)
In-Reply-To: <1391004120-687-6-git-send-email-dh.herrmann@gmail.com>

On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote:
> Lets not trick ourselves into thinking "drm_device" objects are not
> ref-counted. That's just utterly stupid. We manage "drm_minor" objects on
> each drm-device and each minor can have an unlimited number of open
> handles. Each of these handles has the drm_minor (and thus the drm_device)
> as private-data in the file-handle. Therefore, we may not destroy
> "drm_device" until all these handles are closed.
> 
> It is *not* possible to reset all these pointers atomically and restrict
> access to them, and this is *not* how this is done! Instead, we use
> ref-counts to make sure the object is valid and not freed.
> 
> Note that we currently use "dev->open_count" for that, which is *exactly*
> the same as a reference-count, just open coded. So this patch doesn't
> change any semantics on DRM devices (well, this patch just introduces the
> ref-count, anyway. Follow-up patches will replace open_count by it).
> 
> Also note that generic VFS revoke support could allow us to drop this
> ref-count again. We could then just synchronously disable any fops->xy()
> calls. However, this is not the case, yet, and no such patches are
> in sight (and I seriously question the idea of dropping the ref-cnt
> again).
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

[snip]

> +/**
> + * drm_dev_ref - Take reference of a DRM device
> + * @dev: device to take reference of or NULL
> + *
> + * This increases the ref-count of @dev by one. You *must* already own a
> + * reference when calling this. Use drm_dev_unref() to drop this reference
> + * again.
> + *
> + * This function never fails. However, this function does not provide *any*
> + * guarantee whether the device is alive or running. It only provides a
> + * reference to the object and the memory associated with it.
> + */
> +void drm_dev_ref(struct drm_device *dev)
> +{
> +	if (dev)

This check here (and below in the unref code) look funny. What's the
reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
pretty serious bug to me. This is in contrast to kfree(NULL) which imo
makes sense - freeing nothing is a legitimate operation imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch