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

[David Herrmann]

Hi

On Wed, Feb 12, 2014 at 2:25 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 at 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.

I added it mainly to simplify cleanup-code paths. You can then just
call unref() and set it to NULL regardless whether you actually hold a
reference or not. For ref() I don't really care but I think the
NULL-test doesn't hurt either.

I copied this behavior from get_device() and put_device(), btw.
Similar to these functions, I think a lot more will go wrong if the
NULL pointer is not intentional. Imo, ref-counting on a NULL object
just means "no object", so it shouldn't do anything.

Thanks
David