On Mon, Apr 3, 2017 at 4:32 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: > Properties, i.e. the struct drm_property specifying the type and value > range of a property, not the instantiation on a given object, are > invariant over the lifetime of a driver. > > Hence no locking at all is needed, we can just remove it. > > While at it give the function some love and simplify it, to get it > under the 80 char limit: > - Straighten the loops to reduce the nesting. > - use u64_to_user_ptr casting helper > - use put_user for fixed u64 copies. > > Note there's a small behavioural change in that we now copy parts of > the values to userspace if the arrays are a bit too small. Since > userspace will immediately retry anyway, this doesn't matter. > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> This causes a segfault in our ddx: https://bugs.freedesktop.org/show_bug.cgi?id=100673 Alex > --- > drivers/gpu/drm/drm_property.c | 72 +++++++++++++++++------------------------- > 1 file changed, 29 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index b17959c3e099..3feef0659940 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -442,8 +442,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, > struct drm_property *property; > int enum_count = 0; > int value_count = 0; > - int ret = 0, i; > - int copied; > + int i, copied; > struct drm_property_enum *prop_enum; > struct drm_mode_property_enum __user *enum_ptr; > uint64_t __user *values_ptr; > @@ -451,55 +450,43 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EINVAL; > > - drm_modeset_lock_all(dev); > property = drm_property_find(dev, out_resp->prop_id); > - if (!property) { > - ret = -ENOENT; > - goto done; > - } > - > - if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || > - drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { > - list_for_each_entry(prop_enum, &property->enum_list, head) > - enum_count++; > - } > - > - value_count = property->num_values; > + if (!property) > + return -ENOENT; > > strncpy(out_resp->name, property->name, DRM_PROP_NAME_LEN); > out_resp->name[DRM_PROP_NAME_LEN-1] = 0; > out_resp->flags = property->flags; > > - if ((out_resp->count_values >= value_count) && value_count) { > - values_ptr = (uint64_t __user *)(unsigned long)out_resp->values_ptr; > - for (i = 0; i < value_count; i++) { > - if (copy_to_user(values_ptr + i, &property->values[i], sizeof(uint64_t))) { > - ret = -EFAULT; > - goto done; > - } > + value_count = property->num_values; > + values_ptr = u64_to_user_ptr(out_resp->values_ptr); > + > + for (i = 0; i < value_count; i++) { > + if (i < out_resp->count_values && > + put_user(property->values[i], values_ptr + i)) { > + return -EFAULT; > } > } > out_resp->count_values = value_count; > > + copied = 0; > + enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr); > + > if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || > - drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { > - if ((out_resp->count_enum_blobs >= enum_count) && enum_count) { > - copied = 0; > - enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr; > - list_for_each_entry(prop_enum, &property->enum_list, head) { > - > - if (copy_to_user(&enum_ptr[copied].value, &prop_enum->value, sizeof(uint64_t))) { > - ret = -EFAULT; > - goto done; > - } > - > - if (copy_to_user(&enum_ptr[copied].name, > - &prop_enum->name, DRM_PROP_NAME_LEN)) { > - ret = -EFAULT; > - goto done; > - } > - copied++; > - } > + drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { > + list_for_each_entry(prop_enum, &property->enum_list, head) { > + enum_count++; > + if (out_resp->count_enum_blobs <= enum_count) > + continue; > + > + if (copy_to_user(&enum_ptr[copied].value, > + &prop_enum->value, sizeof(uint64_t))) > + return -EFAULT; > + > + if (copy_to_user(&enum_ptr[copied].name, > + &prop_enum->name, DRM_PROP_NAME_LEN)) > + return -EFAULT; > + copied++; > } > out_resp->count_enum_blobs = enum_count; > } > @@ -514,9 +501,8 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, > */ > if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) > out_resp->count_enum_blobs = 0; > -done: > - drm_modeset_unlock_all(dev); > - return ret; > + > + return 0; > } > > static void drm_property_free_blob(struct kref *kref) > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
