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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
