[Intel-gfx] [PATCH v6 3/3] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface

[Chris Wilson]

Quoting Lionel Landwerlin (2017-07-18 17:50:42)
>  static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2b824f8875c4..607484737f3d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1917,6 +1917,9 @@ struct i915_oa_config {
>         struct attribute_group sysfs_metric;
>         struct attribute *attrs[2];
>         struct device_attribute sysfs_metric_id;
> +
> +       /* Only updated while holding dev_priv->perf.metrics_lock. */
> +       bool in_use;

Definitely do not get warm fuzzy feeling about this.

Would making this a regular refcount be difficult? It would for example
allow removing whilst it is still in use by a stream (just removing it
from the user table so new streams cannot be created with it). And stop
me from making helpful suggestions about how in_use access is purely
atomic...

> +       oa_config = kzalloc(sizeof(*oa_config), GFP_KERNEL);
> +       if (!oa_config) {
> +               DRM_DEBUG("Failed to allocate memory for the OA config\n");
> +               return -ENOMEM;
> +       }
> +
> +       err = strncpy_from_user(oa_config->uuid, u64_to_user_ptr(args->uuid),
> +                               UUID_STRING_LEN);

So you have a fixed size buffer, why have userspace pass a pointer
rather than an array? Try char uuid[UUID_STRING_LEN] in 
struct drm_i915_perf_oa_config and see which you think is eaiser to use
and, more importantly, harder to get wrong.

> +       if (err < 0) {
> +               DRM_DEBUG("Failed to copy uuid from OA config\n");
> +               goto mux_err;
> +       }
> +
> +       if (!uuid_is_valid(oa_config->uuid)) {
> +               DRM_DEBUG("Invalid uuid format for OA config\n");
> +               err = -EINVAL;
> +               goto mux_err;
> +       }
> +       idr_for_each_entry(&dev_priv->perf.metrics_idr, tmp, id) {

Comment that you don't expect many so this iteration shouldn't be too
costly.

> +               if (!strcmp(tmp->uuid, oa_config->uuid)) {
> +                       DRM_DEBUG("OA config already exists with this uuid\n");
> +                       err = -EADDRINUSE;
> +                       goto sysfs_err;
> +               }
> +       }
> +
> +       err = create_dynamic_oa_sysfs_entry(dev_priv, oa_config);
> +       if (err) {
> +               DRM_DEBUG("Failed to create sysfs entry for OA config\n");
> +               goto sysfs_err;
> +       }
> +
> +       oa_config->id = idr_alloc(&dev_priv->perf.metrics_idr,
> +                                 oa_config, 2,

Comment on reserving 0 for invalid and 1 for default.

> +}
> +/**
> + * Structure to upload perf dynamic configuration into the kernel.
> + */
> +struct drm_i915_perf_oa_config {
> +       /** String formatted like "%08x-%04x-%04x-%04x-%012x" */
> +       __u64 __user uuid;

Not a pointer, so __user is meaningless (and should generate warnings
from sparse). Equally it is nice to know in the name which of these are
encoding user pointers.

> +       __u32 n_mux_regs;
> +       __u32 pad0;
> +       __u64 __user mux_regs;
> +
> +       __u32 n_boolean_regs;
> +       __u32 pad1;
> +       __u64 __user boolean_regs;
> +
> +       __u32 n_flex_regs;
> +       __u32 pad2;
> +       __u64 __user flex_regs;


With a little bit of rearranging you can reduce the padding.
-Chris