| Message ID | 1391004120-687-6-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi On Wed, Feb 12, 2014 at 2:25 PM, Daniel Vetter <daniel@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@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
On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >>> +/** >>> + * 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. 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? Thanks, Daniel
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@gmail.com> wrote: > >>> +/** > >>> + * 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. > > 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
Hi On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > 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@gmail.com> wrote: >> >>> +/** >> >>> + * 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. >> >> 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. Ok, for _ref() I'm fine dropping it, but for _unref() I really don't understand the concerns. I like to follow the principle of making teardown-functions work with partially initialized objects. A caller shouldn't be required to reverse all it's setup functions if one last step of object-initialization fails. It's much easier if they can just call the destructor which figures itself out which parts are initialized. Obviously, this isn't always possible, but checking for NULL in _unref() or _put() paths simplifies this a lot and avoids non-sense if(obj) unref(obj); For instance for drm_minor objects we only initialize the minors that are enabled by the specific driver. However, it's enough to test for the flags during device-initialization. device-registration, -deregistration and -teardown just call _free/unref on all possible minors. Allowing NULL avoids testing for these flags in every path but the initialization. Anyhow, shared code -> many opinions, so if people agree on dropping it, I will do so. Thanks David
On Wed, Feb 12, 2014 at 06:48:50PM +0100, David Herrmann wrote: > On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > 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@gmail.com> wrote: > >> >>> +/** > >> >>> + * 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. > >> > >> 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. > > Ok, for _ref() I'm fine dropping it, but for _unref() I really don't > understand the concerns. I like to follow the principle of making > teardown-functions work with partially initialized objects. A caller > shouldn't be required to reverse all it's setup functions if one last > step of object-initialization fails. It's much easier if they can just > call the destructor which figures itself out which parts are > initialized. Obviously, this isn't always possible, but checking for > NULL in _unref() or _put() paths simplifies this a lot and avoids > non-sense if(obj) unref(obj); > > For instance for drm_minor objects we only initialize the minors that > are enabled by the specific driver. However, it's enough to test for > the flags during device-initialization. device-registration, > -deregistration and -teardown just call _free/unref on all possible > minors. Allowing NULL avoids testing for these flags in every path but > the initialization. > > Anyhow, shared code -> many opinions, so if people agree on dropping > it, I will do so. I might have missed it, but afaics both drm_minor_free and unregister already have NULL pointer checks at the beginning for other reasons. And the _unref in the drm unload paths also should never see a NULL drm_device. So I think with your current patch series we're already covered and there's no need for any additional NULL checks, hence also none for the drm_dev_unref function. -Daniel
On Wed, Feb 12, 2014 at 06:48:50PM +0100, David Herrmann wrote: > Hi > > On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > 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@gmail.com> wrote: > >> >>> +/** > >> >>> + * 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. > >> > >> 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. > > Ok, for _ref() I'm fine dropping it, but for _unref() I really don't > understand the concerns. I like to follow the principle of making > teardown-functions work with partially initialized objects. A caller > shouldn't be required to reverse all it's setup functions if one last > step of object-initialization fails. It's much easier if they can just > call the destructor which figures itself out which parts are > initialized. Obviously, this isn't always possible, but checking for > NULL in _unref() or _put() paths simplifies this a lot and avoids > non-sense if(obj) unref(obj); > > For instance for drm_minor objects we only initialize the minors that > are enabled by the specific driver. However, it's enough to test for > the flags during device-initialization. device-registration, > -deregistration and -teardown just call _free/unref on all possible > minors. Allowing NULL avoids testing for these flags in every path but > the initialization. I have no objection to that justification for it. greg k-h
On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote: [...] > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c [...] > @@ -486,12 +490,10 @@ EXPORT_SYMBOL(drm_dev_alloc); > * @dev: DRM device to free > * > * Free a DRM device that has previously been allocated via drm_dev_alloc(). > - * You must not use kfree() instead or you will leak memory. > - * > - * This must not be called once the device got registered. Use drm_put_dev() > - * instead, which then calls drm_dev_free(). > + * This may not be called directly, you must use drm_dev_ref() and > + * drm_dev_unref() to gain and drop references to the object. > */ > -void drm_dev_free(struct drm_device *dev) > +static void drm_dev_free(struct drm_device *dev) With the function turning static it can't be called from anywhere else but this file, so perhaps the last sentence of the comment can now go away as well? Thierry
Hi On Fri, Feb 21, 2014 at 8:01 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote: > [...] >> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > [...] >> @@ -486,12 +490,10 @@ EXPORT_SYMBOL(drm_dev_alloc); >> * @dev: DRM device to free >> * >> * Free a DRM device that has previously been allocated via drm_dev_alloc(). >> - * You must not use kfree() instead or you will leak memory. >> - * >> - * This must not be called once the device got registered. Use drm_put_dev() >> - * instead, which then calls drm_dev_free(). >> + * This may not be called directly, you must use drm_dev_ref() and >> + * drm_dev_unref() to gain and drop references to the object. >> */ >> -void drm_dev_free(struct drm_device *dev) >> +static void drm_dev_free(struct drm_device *dev) > > With the function turning static it can't be called from anywhere else > but this file, so perhaps the last sentence of the comment can now go > away as well? Yepp, fixed. Thanks David
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 5736aaa..9ded847 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -351,7 +351,7 @@ err_agp: drm_pci_agp_destroy(dev); pci_disable_device(pdev); err_free: - drm_dev_free(dev); + drm_dev_unref(dev); return ret; } EXPORT_SYMBOL(drm_get_pci_dev); diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index 21fc820..319ff53 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -64,7 +64,7 @@ static int drm_get_platform_dev(struct platform_device *platdev, return 0; err_free: - drm_dev_free(dev); + drm_dev_unref(dev); return ret; } diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 98a33c580..c51333e 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -392,7 +392,7 @@ void drm_put_dev(struct drm_device *dev) } drm_dev_unregister(dev); - drm_dev_free(dev); + drm_dev_unref(dev); } EXPORT_SYMBOL(drm_put_dev); @@ -410,7 +410,7 @@ void drm_unplug_dev(struct drm_device *dev) drm_device_set_unplugged(dev); if (dev->open_count == 0) { - drm_put_dev(dev); + drm_dev_unref(dev); } mutex_unlock(&drm_global_mutex); } @@ -425,6 +425,9 @@ EXPORT_SYMBOL(drm_unplug_dev); * Call drm_dev_register() to advertice the device to user space and register it * with other core subsystems. * + * The initial ref-count of the object is 1. Use drm_dev_ref() and + * drm_dev_unref() to take and drop further ref-counts. + * * RETURNS: * Pointer to new DRM device, or NULL if out of memory. */ @@ -438,6 +441,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, if (!dev) return NULL; + kref_init(&dev->ref); dev->dev = parent; dev->driver = driver; @@ -486,12 +490,10 @@ EXPORT_SYMBOL(drm_dev_alloc); * @dev: DRM device to free * * Free a DRM device that has previously been allocated via drm_dev_alloc(). - * You must not use kfree() instead or you will leak memory. - * - * This must not be called once the device got registered. Use drm_put_dev() - * instead, which then calls drm_dev_free(). + * This may not be called directly, you must use drm_dev_ref() and + * drm_dev_unref() to gain and drop references to the object. */ -void drm_dev_free(struct drm_device *dev) +static void drm_dev_free(struct drm_device *dev) { drm_put_minor(dev->control); drm_put_minor(dev->render); @@ -506,7 +508,44 @@ void drm_dev_free(struct drm_device *dev) kfree(dev->devname); kfree(dev); } -EXPORT_SYMBOL(drm_dev_free); + +static void drm_dev_release(struct kref *ref) +{ + drm_dev_free(container_of(ref, struct drm_device, ref)); +} + +/** + * 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) + kref_get(&dev->ref); +} +EXPORT_SYMBOL(drm_dev_ref); + +/** + * drm_dev_unref - Drop reference of a DRM device + * @dev: device to drop reference of or NULL + * + * This decreases the ref-count of @dev by one. The device is destroyed if the + * ref-count drops to zero. + */ +void drm_dev_unref(struct drm_device *dev) +{ + if (dev) + kref_put(&dev->ref, drm_dev_release); +} +EXPORT_SYMBOL(drm_dev_unref); /** * drm_dev_register - Register DRM device @@ -581,7 +620,7 @@ EXPORT_SYMBOL(drm_dev_register); * * Unregister the DRM device from the system. This does the reverse of * drm_dev_register() but does not deallocate the device. The caller must call - * drm_dev_free() to free all resources. + * drm_dev_unref() to drop their final reference. */ void drm_dev_unregister(struct drm_device *dev) { diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c index 0f8cb1a..c3406aa 100644 --- a/drivers/gpu/drm/drm_usb.c +++ b/drivers/gpu/drm/drm_usb.c @@ -30,7 +30,7 @@ int drm_get_usb_dev(struct usb_interface *interface, return 0; err_free: - drm_dev_free(dev); + drm_dev_unref(dev); return ret; } diff --git a/drivers/gpu/drm/tegra/bus.c b/drivers/gpu/drm/tegra/bus.c index e38e596..71cef5c 100644 --- a/drivers/gpu/drm/tegra/bus.c +++ b/drivers/gpu/drm/tegra/bus.c @@ -63,7 +63,7 @@ int drm_host1x_init(struct drm_driver *driver, struct host1x_device *device) return 0; err_free: - drm_dev_free(drm); + drm_dev_unref(drm); return ret; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c3aaf2d..2c58e94 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -43,6 +43,7 @@ #include <asm/current.h> #endif /* __alpha__ */ #include <linux/kernel.h> +#include <linux/kref.h> #include <linux/miscdevice.h> #include <linux/fs.h> #include <linux/init.h> @@ -1099,6 +1100,7 @@ struct drm_device { /** \name Lifetime Management */ /*@{ */ + struct kref ref; /**< Object ref-count */ struct device *dev; /**< Device structure of bus-device */ struct drm_driver *driver; /**< DRM driver managing the device */ void *dev_private; /**< DRM driver private data */ @@ -1663,7 +1665,8 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map) struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); -void drm_dev_free(struct drm_device *dev); +void drm_dev_ref(struct drm_device *dev); +void drm_dev_unref(struct drm_device *dev); int drm_dev_register(struct drm_device *dev, unsigned long flags); void drm_dev_unregister(struct drm_device *dev); /*@}*/
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> --- drivers/gpu/drm/drm_pci.c | 2 +- drivers/gpu/drm/drm_platform.c | 2 +- drivers/gpu/drm/drm_stub.c | 57 +++++++++++++++++++++++++++++++++++------- drivers/gpu/drm/drm_usb.c | 2 +- drivers/gpu/drm/tegra/bus.c | 2 +- include/drm/drmP.h | 5 +++- 6 files changed, 56 insertions(+), 14 deletions(-)