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