On 18/07/17 19:34, Chris Wilson wrote:
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...
Moved to atomic refcount.
+ 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.
Done.
+ 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.
Done.
+ 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.
Done.
+}
+/**
+ * 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.
Done.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
