[PATCH 05/13] drm: provide device-refcount (original) (raw)

Greg KH gregkh at linuxfoundation.org
Wed Feb 12 08:40:55 PST 2014

On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:

On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann at gmail.com> wrote: >>> +/** >>> + * drmdevref - 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 drmdevunref() 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 drmdevref(struct drmdevice *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 getdevice() and putdevice(), 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.

My fear with this kind of magic is that someone accidentally exchanges the pointer clearing to NULL (or assignement when grabbing a ref) with the unref/ref call and then we have a very subtle bug at hand. If we don't accept NULL objects the failure will be much more obvious. The entire kernel kobject stuff is very consistent about this, but I couldn't find a reason for it - all the NULL checks predate git history. Greg can you please shed some lights on best practice here and whether my fears are justified given your experience with shoddy drivers in general?

Yes, the driver core does test for NULL here, as sometimes you are passing in a "parent" pointer, and don't really care if it is NULL or not, so just treating it as if you really do have a reference is usually fine.

But, for a subsystem where you "know" you will not be doing anything as foolish as that, I'd not allow that :)

So I'd recommend taking those checks out of the drm code.

thanks,

greg k-h

More information about the dri-devel mailing list