| Message ID | 1399995050-28435-2-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Some minor nitpicks below: On Tue, May 13, 2014 at 5:30 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > Add a helper function that allows drivers to statically set the unique > name of the device. This will allow platform and USB drivers to get rid > of their DRM bus implementations and directly use drm_dev_alloc() and > drm_dev_register(). > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v2: > - move drm_set_unique() to drivers/gpu/drm/drm_stub.c > - add kernel-doc > > drivers/gpu/drm/drm_ioctl.c | 23 +++++++++++++++++------ > drivers/gpu/drm/drm_stub.c | 24 ++++++++++++++++++++++++ > include/drm/drmP.h | 3 +++ > 3 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 38269d5aa333..a59f6c2d7586 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -131,13 +131,24 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) > if (master->unique != NULL) > drm_unset_busid(dev, master); > > - ret = dev->driver->bus->set_busid(dev, master); > - if (ret) > - goto err; > + if (dev->driver->bus && dev->driver->bus->set_busid) { > + ret = dev->driver->bus->set_busid(dev, master); > + if (ret) { > + drm_unset_busid(dev, master); > + return ret; > + } > + } else { > + WARN(dev->unique == NULL, > + "No drm_bus.set_busid() implementation provided by %ps. " > + "Set the unique name explicitly using drm_set_unique().", > + dev->driver); return -EINVAL? If you don't bail out here, the kstrdup() below will cause a kernel-oops. There's no reason to WARN() if you purposely cause an oops below. > + > + master->unique = kstrdup(dev->unique, GFP_KERNEL); > + if (master->unique) > + master->unique_len = strlen(dev->unique); > + } > + > return 0; > -err: > - drm_unset_busid(dev, master); > - return ret; > } > > /** > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index 1447b0ee3676..64cf97dc4ce4 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -535,6 +535,29 @@ static void drm_fs_inode_free(struct inode *inode) > } > > /** > + * drm_set_unique - Set the unique name of a DRM device > + * @dev: device of which to set the unique name > + * @fmt: format string for unique name > + * > + * Sets the unique name of a DRM device using the specified format string and > + * a variable list of arguments. Drivers can use this at driver probe time if > + * the unique name of the devices they drive is static. > + */ > +int drm_set_unique(struct drm_device *dev, const char *fmt, ...) I tried renaming all the device-management APIs to "drm_dev_*", so how about moving this below drm_dev_unregister() and renaming it to drm_dev_set_unique()? I prefer prefixes which describe the object the function works on. set_unique() works on drm_device, so lets use the known drm_dev_* prefix. Just my opinion, others might disagree.. > +{ > + va_list ap; > + > + kfree(dev->unique); > + > + va_start(ap, fmt); > + dev->unique = kvasprintf(GFP_KERNEL, fmt, ap); > + va_end(ap); > + > + return dev->unique ? 0 : -ENOMEM; > +} > +EXPORT_SYMBOL(drm_set_unique); > + > +/** > * drm_dev_alloc - Allocate new drm device > * @driver: DRM driver to allocate device for > * @parent: Parent device object > @@ -649,6 +672,7 @@ static void drm_dev_release(struct kref *ref) > drm_minor_free(dev, DRM_MINOR_CONTROL); > > mutex_destroy(&dev->master_mutex); > + kfree(dev->unique); > kfree(dev); > } > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 6b4ed9e84e6f..dcec96b1154c 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1235,6 +1235,8 @@ struct drm_device { > struct drm_vma_offset_manager *vma_offset_manager; > /*@} */ > int switch_power_state; > + > + char *unique; Can you add this to the group at the top of the object? This somehow belongs to /** \name Lifetime Management */ as it is part of device initialization and API. > }; > > #define DRM_SWITCH_POWER_ON 0 > @@ -1680,6 +1682,7 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map) > > #include <drm/drm_mem_util.h> > > +int drm_set_unique(struct drm_device *dev, const char *fmt, ...); Again just a minor style-nitpick: Move that below the drm_dev_*() declarations, not above. Usual kernel-style is to have allocators/lifetime-management first, followed by runtime management. Otherwise looks good: Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Thanks David > struct drm_device *drm_dev_alloc(struct drm_driver *driver, > struct device *parent); > void drm_dev_ref(struct drm_device *dev); > -- > 1.9.2 >
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 38269d5aa333..a59f6c2d7586 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -131,13 +131,24 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) if (master->unique != NULL) drm_unset_busid(dev, master); - ret = dev->driver->bus->set_busid(dev, master); - if (ret) - goto err; + if (dev->driver->bus && dev->driver->bus->set_busid) { + ret = dev->driver->bus->set_busid(dev, master); + if (ret) { + drm_unset_busid(dev, master); + return ret; + } + } else { + WARN(dev->unique == NULL, + "No drm_bus.set_busid() implementation provided by %ps. " + "Set the unique name explicitly using drm_set_unique().", + dev->driver); + + master->unique = kstrdup(dev->unique, GFP_KERNEL); + if (master->unique) + master->unique_len = strlen(dev->unique); + } + return 0; -err: - drm_unset_busid(dev, master); - return ret; } /** diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 1447b0ee3676..64cf97dc4ce4 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -535,6 +535,29 @@ static void drm_fs_inode_free(struct inode *inode) } /** + * drm_set_unique - Set the unique name of a DRM device + * @dev: device of which to set the unique name + * @fmt: format string for unique name + * + * Sets the unique name of a DRM device using the specified format string and + * a variable list of arguments. Drivers can use this at driver probe time if + * the unique name of the devices they drive is static. + */ +int drm_set_unique(struct drm_device *dev, const char *fmt, ...) +{ + va_list ap; + + kfree(dev->unique); + + va_start(ap, fmt); + dev->unique = kvasprintf(GFP_KERNEL, fmt, ap); + va_end(ap); + + return dev->unique ? 0 : -ENOMEM; +} +EXPORT_SYMBOL(drm_set_unique); + +/** * drm_dev_alloc - Allocate new drm device * @driver: DRM driver to allocate device for * @parent: Parent device object @@ -649,6 +672,7 @@ static void drm_dev_release(struct kref *ref) drm_minor_free(dev, DRM_MINOR_CONTROL); mutex_destroy(&dev->master_mutex); + kfree(dev->unique); kfree(dev); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 6b4ed9e84e6f..dcec96b1154c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1235,6 +1235,8 @@ struct drm_device { struct drm_vma_offset_manager *vma_offset_manager; /*@} */ int switch_power_state; + + char *unique; }; #define DRM_SWITCH_POWER_ON 0 @@ -1680,6 +1682,7 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map) #include <drm/drm_mem_util.h> +int drm_set_unique(struct drm_device *dev, const char *fmt, ...); struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); void drm_dev_ref(struct drm_device *dev);