Re: [PATCH v2 1/7] drm: Introduce drm_set_unique()

[David Herrmann]

Hi

Some minor nitpicks below:

On Tue, May 13, 2014 at 5:30 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> 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@xxxxxxxx>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> 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@xxxxxxxxx>

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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/