[PATCH v4 1/1] drm/syncobj: add sideband payload

[Chris Wilson]

Quoting Lionel Landwerlin (2019-08-09 12:30:30)
> +int drm_syncobj_binary_ioctl(struct drm_device *dev, void *data,
> +                            struct drm_file *file_private)
> +{
> +       struct drm_syncobj_binary_array *args = data;
> +       struct drm_syncobj **syncobjs;
> +       u32 __user *access_flags = u64_to_user_ptr(args->access_flags);
> +       u64 __user *values = u64_to_user_ptr(args->values);
> +       u32 i;
> +       int ret;
> +
> +       if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> +               return -EOPNOTSUPP;
> +
> +       if (args->pad != 0)
> +               return -EINVAL;
> +
> +       if (args->count_handles == 0)
> +               return -EINVAL;

You may find it easier to just return success for 0 handles. Slightly less
obnoxious error handling?

> +       ret = drm_syncobj_array_find(file_private,
> +                                    u64_to_user_ptr(args->handles),
> +                                    args->count_handles,
> +                                    &syncobjs);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (i = 0; i < args->count_handles; i++) {
> +               u32 flags;
> +
> +               copy_from_user(&flags, &access_flags[i], sizeof(flags));
> +               ret = ret ? -EFAULT : 0;

Magic?

if (get_user(flags, &access_flags[i[))
	return -EFAULT;

> +               if (ret)
> +                       break;
> +
> +               if (flags & I915_DRM_SYNCOBJ_BINARY_ITEM_VALUE_READ) {
> +                       copy_to_user(&values[i], &syncobjs[i]->binary_payload, sizeof(values[i]));
> +                       ret = ret ? -EFAULT : 0;

More magic.

if (put_user(&syncobjs[i]->binary_payload, &values[i]))
	return -EFAULT;

> +                       if (ret)
> +                               break;
> +               }
> +
> +               if (flags & I915_DRM_SYNCOBJ_BINARY_ITEM_VALUE_INC)
> +                       syncobjs[i]->binary_payload++;

So if an error occurs how does the user know which syncobj were
advanced before the error? (Or explain why it doesn't actually matter)
The clue I guess is with read/inc, but confirmation of design would be
nice.

Not atomic (the u64 write should really be to avoid total corruption)
and nothing prevents userspace from racing. How safe is that in the
overall design?

What would happen if the binary_payload was initialised to -1?

> +       }
> +
>         drm_syncobj_array_free(syncobjs, args->count_handles);
>  
>         return ret;