On Wed, Jun 01, 2016 at 12:01:44PM +0200, Daniel Vetter wrote:
On Mon, May 30, 2016 at 09:38:19AM +0100, Chris Wilson wrote:
In order to allow drivers to pack their privates and drm_device into one struct (e.g. for subclassing), export the initialisation routines for struct drm_device.
v2: Missed return ret. That error path had only one job to do!
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: dri-devel@lists.freedesktop.org
drivers/gpu/drm/drm_drv.c | 63 ++++++++++++++++++++++++++++++++++++----------- include/drm/drmP.h | 3 +++ 2 files changed, 52 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index bff89226a344..f0553ccd4f71 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -549,11 +549,12 @@ static void drm_fs_inode_free(struct inode *inode) }
/**
- drm_dev_alloc - Allocate new DRM device
- @driver: DRM driver to allocate device for
- drm_dev_init - Initialise new DRM device
- @dev: DRM device
- @driver: DRM driver
- @parent: Parent device object
- Allocate and initialize a new DRM device. No device registration is done.
- Initialize a new DRM device. No device registration is done.
- Call drm_dev_register() to advertice the device to user space and register it
- with other core subsystems. This should be done last in the device
- initialization sequence to make sure userspace can't access an inconsistent
@@ -565,18 +566,14 @@ static void drm_fs_inode_free(struct inode *inode)
- Note that for purely virtual devices @parent can be NULL.
Maybe add "Drivers which don't want to allocate their own device structure embedding &drm_device can instead just call drm_dev_alloc()."
- RETURNS:
- Pointer to new DRM device, or NULL if out of memory.
*/
- 0 on success, or error code on failure.
-struct drm_device *drm_dev_alloc(struct drm_driver *driver,
struct device *parent)+int drm_dev_init(struct drm_device *dev,
struct drm_driver *driver,struct device *parent){
struct drm_device *dev; int ret;
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
return NULL;kref_init(&dev->ref); dev->dev = parent; dev->driver = driver;
@@ -638,7 +635,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, goto err_setunique; }
- return dev;
- return 0;
err_setunique: if (drm_core_check_feature(dev, DRIVER_GEM)) @@ -653,8 +650,46 @@ err_minors: drm_fs_inode_free(dev->anon_inode); err_free: mutex_destroy(&dev->master_mutex);
- kfree(dev);
- return NULL;
- return ret;
for "goto err_minors" you need to set ret first, otherwise we'll fail and return 0. Wasn't a problem because of the unconditional return NULL here.
+} +EXPORT_SYMBOL(drm_dev_init);
+/**
- drm_dev_alloc - Allocate new DRM device
- @driver: DRM driver to allocate device for
- @parent: Parent device object
- Allocate and initialize a new DRM device. No device registration is done.
- Call drm_dev_register() to advertice the device to user space and register it
- with other core subsystems. This should be done last in the device
- initialization sequence to make sure userspace can't access an inconsistent
- state.
- The initial ref-count of the object is 1. Use drm_dev_ref() and
- drm_dev_unref() to take and drop further ref-counts.
- Note that for purely virtual devices @parent can be NULL.
Maybe we can also add "Drivers which want to subclass/embed struct &drm_device should instead look at drm_dev_init()."
- RETURNS:
- Pointer to new DRM device, or NULL if out of memory.
- */
+struct drm_device *drm_dev_alloc(struct drm_driver *driver,
struct device *parent)+{
- struct drm_device *dev;
- int ret;
- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
- if (!dev)
return NULL;- ret = drm_dev_init(dev, driver, parent);
- if (ret) {
kfree(dev);return NULL;- }
- return dev;
} EXPORT_SYMBOL(drm_dev_alloc);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c5d29505f937..12dc5f9cad89 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1075,6 +1075,9 @@ extern void drm_sysfs_hotplug_event(struct drm_device *dev);
struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); +int drm_dev_init(struct drm_device *dev,
struct drm_driver *driver,struct device *parent);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);
With the one bug in the error handling fixed, and the kerneldoc augmented:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
r-b retracted, this is a bit incomplete since it doesn't handle drm_dev_release. I think we also should add an optional ->device_free callback and use that in drm_dev_release instead of kfree().
That would finally make the drm_device refcounting also somewhat useful, since right now you have to release driver-private stuff before the device is actually gone, which is buggy. Or just leak, which is also buggy ;-) -Daniel