From: Ville Syrjälä ville.syrjala@linux.intel.com
If the property already has the enum value WARN and bail. Replacing enum values doesn't make sense to me.
Throw out the pointless list_empty() while at it.
Cc: Daniel Vetter daniel@ffwll.ch Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index c37ac41125b5..d77b0c4dc485 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -390,14 +390,9 @@ int drm_property_add_enum(struct drm_property *property, int index, (value > 63)) return -EINVAL;
- if (!list_empty(&property->enum_list)) { - list_for_each_entry(prop_enum, &property->enum_list, head) { - if (prop_enum->value == value) { - strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN); - prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0'; - return 0; - } - } + list_for_each_entry(prop_enum, &property->enum_list, head) { + if (WARN_ON(prop_enum->value == value)) + return -EINVAL; }
prop_enum = kzalloc(sizeof(struct drm_property_enum), GFP_KERNEL);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Trying to add enum values to non-enum/bitmask properties is a programmer mistake. WARN to make sure the developers notice their mistake.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index d77b0c4dc485..e676b1ecc705 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -378,8 +378,8 @@ int drm_property_add_enum(struct drm_property *property, int index, if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) return -EINVAL;
- if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) || - drm_property_type_is(property, DRM_MODE_PROP_BITMASK))) + if (WARN_ON(!drm_property_type_is(property, DRM_MODE_PROP_ENUM) && + !drm_property_type_is(property, DRM_MODE_PROP_BITMASK))) return -EINVAL;
/*
On Tue, Mar 06, 2018 at 06:48:45PM +0200, Ville Syrjala wrote:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
Enum values >63 with a bitmask property is a programmer error. WARN when someone is attempting this.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index e676b1ecc705..019d1abb94ca 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -386,8 +386,8 @@ int drm_property_add_enum(struct drm_property *property, int index, * Bitmask enum properties have the additional constraint of values * from 0 to 63 */ - if (drm_property_type_is(property, DRM_MODE_PROP_BITMASK) && - (value > 63)) + if (WARN_ON(drm_property_type_is(property, DRM_MODE_PROP_BITMASK) && + value > 63)) return -EINVAL;
list_for_each_entry(prop_enum, &property->enum_list, head) {
On Tue, Mar 06, 2018 at 06:48:46PM +0200, Ville Syrjala wrote:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
DRM_MODE_PROP_PENDING is not used anywhere (except printed out by libdrm proptest/modetest).
This seems to be yet another thing blindly copied from xrandr. Quoting from the protocol spec: "If 'pending' is TRUE, changes made to property values with RRChangeOutputProperty will be saved in the pending property value and be automatically copied to the current value on the next RRSetCrtcConfig request involving the named output. If 'pending' is FALSE, changes are copied immediately."
So it was some kind of early idea for atomic property updates.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- include/uapi/drm/drm_mode.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index b5d7d9e0eff5..50bcf4214ff9 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -363,7 +363,7 @@ struct drm_mode_get_connector { __u32 pad; };
-#define DRM_MODE_PROP_PENDING (1<<0) +#define DRM_MODE_PROP_PENDING (1<<0) /* deprecated, do not use */ #define DRM_MODE_PROP_RANGE (1<<1) #define DRM_MODE_PROP_IMMUTABLE (1<<2) #define DRM_MODE_PROP_ENUM (1<<3) /* enumerated type with text strings */
On Tue, Mar 06, 2018 at 06:48:47PM +0200, Ville Syrjala wrote:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
The property flags are part of the uabi and we have 32 bits for them. Pass them around as u32 internally as well, instead of a signed int.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 41 +++++++++++++++++++++-------------------- include/drm/drm_property.h | 24 +++++++++++++----------- 2 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index 019d1abb94ca..027a50e55e96 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -72,8 +72,9 @@ static bool drm_property_type_valid(struct drm_property *property) * Returns: * A pointer to the newly created property on success, NULL on failure. */ -struct drm_property *drm_property_create(struct drm_device *dev, int flags, - const char *name, int num_values) +struct drm_property *drm_property_create(struct drm_device *dev, + u32 flags, const char *name, + int num_values) { struct drm_property *property = NULL; int ret; @@ -136,10 +137,10 @@ EXPORT_SYMBOL(drm_property_create); * Returns: * A pointer to the newly created property on success, NULL on failure. */ -struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags, - const char *name, - const struct drm_prop_enum_list *props, - int num_values) +struct drm_property *drm_property_create_enum(struct drm_device *dev, + u32 flags, const char *name, + const struct drm_prop_enum_list *props, + int num_values) { struct drm_property *property; int i, ret; @@ -185,10 +186,10 @@ EXPORT_SYMBOL(drm_property_create_enum); * A pointer to the newly created property on success, NULL on failure. */ struct drm_property *drm_property_create_bitmask(struct drm_device *dev, - int flags, const char *name, - const struct drm_prop_enum_list *props, - int num_props, - uint64_t supported_bits) + u32 flags, const char *name, + const struct drm_prop_enum_list *props, + int num_props, + uint64_t supported_bits) { struct drm_property *property; int i, ret, index = 0; @@ -222,8 +223,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev, EXPORT_SYMBOL(drm_property_create_bitmask);
static struct drm_property *property_create_range(struct drm_device *dev, - int flags, const char *name, - uint64_t min, uint64_t max) + u32 flags, const char *name, + uint64_t min, uint64_t max) { struct drm_property *property;
@@ -256,9 +257,9 @@ static struct drm_property *property_create_range(struct drm_device *dev, * Returns: * A pointer to the newly created property on success, NULL on failure. */ -struct drm_property *drm_property_create_range(struct drm_device *dev, int flags, - const char *name, - uint64_t min, uint64_t max) +struct drm_property *drm_property_create_range(struct drm_device *dev, + u32 flags, const char *name, + uint64_t min, uint64_t max) { return property_create_range(dev, DRM_MODE_PROP_RANGE | flags, name, min, max); @@ -285,8 +286,8 @@ EXPORT_SYMBOL(drm_property_create_range); * A pointer to the newly created property on success, NULL on failure. */ struct drm_property *drm_property_create_signed_range(struct drm_device *dev, - int flags, const char *name, - int64_t min, int64_t max) + u32 flags, const char *name, + int64_t min, int64_t max) { return property_create_range(dev, DRM_MODE_PROP_SIGNED_RANGE | flags, name, I642U64(min), I642U64(max)); @@ -312,7 +313,7 @@ EXPORT_SYMBOL(drm_property_create_signed_range); * A pointer to the newly created property on success, NULL on failure. */ struct drm_property *drm_property_create_object(struct drm_device *dev, - int flags, const char *name, + u32 flags, const char *name, uint32_t type) { struct drm_property *property; @@ -348,8 +349,8 @@ EXPORT_SYMBOL(drm_property_create_object); * Returns: * A pointer to the newly created property on success, NULL on failure. */ -struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, - const char *name) +struct drm_property *drm_property_create_bool(struct drm_device *dev, + u32 flags, const char *name) { return drm_property_create_range(dev, flags, name, 0, 1); } diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h index 8a522b4bed40..937757a8a568 100644 --- a/include/drm/drm_property.h +++ b/include/drm/drm_property.h @@ -237,27 +237,29 @@ static inline bool drm_property_type_is(struct drm_property *property, return property->flags & type; }
-struct drm_property *drm_property_create(struct drm_device *dev, int flags, - const char *name, int num_values); -struct drm_property *drm_property_create_enum(struct drm_device *dev, int flags, - const char *name, +struct drm_property *drm_property_create(struct drm_device *dev, + u32 flags, const char *name, + int num_values); +struct drm_property *drm_property_create_enum(struct drm_device *dev, + u32 flags, const char *name, const struct drm_prop_enum_list *props, int num_values); struct drm_property *drm_property_create_bitmask(struct drm_device *dev, - int flags, const char *name, + u32 flags, const char *name, const struct drm_prop_enum_list *props, int num_props, uint64_t supported_bits); -struct drm_property *drm_property_create_range(struct drm_device *dev, int flags, - const char *name, +struct drm_property *drm_property_create_range(struct drm_device *dev, + u32 flags, const char *name, uint64_t min, uint64_t max); struct drm_property *drm_property_create_signed_range(struct drm_device *dev, - int flags, const char *name, + u32 flags, const char *name, int64_t min, int64_t max); struct drm_property *drm_property_create_object(struct drm_device *dev, - int flags, const char *name, uint32_t type); -struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, - const char *name); + u32 flags, const char *name, + uint32_t type); +struct drm_property *drm_property_create_bool(struct drm_device *dev, + u32 flags, const char *name); int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name); void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
On Tue, Mar 06, 2018 at 06:48:48PM +0200, Ville Syrjala wrote:
u32 vs uint32_t (in struct drm_property) ftw, but who cares.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
Pimp drm_property_type_valid() to check for more fails with the property flags. Also make the check before adding the property, and bail out if things look bad.
Since we're now chekcing for more than the type let's also change the function name to drm_property_flags_valid().
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index 027a50e55e96..6ac6ee41a6a3 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -50,11 +50,27 @@ * IOCTL and in the get/set property IOCTL. */
-static bool drm_property_type_valid(struct drm_property *property) +static bool drm_property_flags_valid(u32 flags) { - if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE) - return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE); - return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE); + u32 legacy_type = flags & DRM_MODE_PROP_LEGACY_TYPE; + u32 ext_type = flags & DRM_MODE_PROP_EXTENDED_TYPE; + + /* Reject undefined/deprecated flags */ + if (flags & ~(DRM_MODE_PROP_LEGACY_TYPE | + DRM_MODE_PROP_EXTENDED_TYPE | + DRM_MODE_PROP_IMMUTABLE | + DRM_MODE_PROP_ATOMIC)) + return false; + + /* We want either a legacy type or an extended type, but not both */ + if (!legacy_type == !ext_type) + return false; + + /* Only one legacy type at a time please */ + if (legacy_type && !is_power_of_2(legacy_type)) + return false; + + return true; }
/** @@ -79,6 +95,9 @@ struct drm_property *drm_property_create(struct drm_device *dev, struct drm_property *property = NULL; int ret;
+ if (WARN_ON(!drm_property_flags_valid(flags))) + return NULL; + if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) return NULL;
@@ -108,8 +127,6 @@ struct drm_property *drm_property_create(struct drm_device *dev,
list_add_tail(&property->head, &dev->mode_config.property_list);
- WARN_ON(!drm_property_type_valid(property)); - return property; fail: kfree(property->values);
On Tue, Mar 06, 2018 at 06:48:49PM +0200, Ville Syrjala wrote:
I think this catches everything. Well except not-yet-assigned EXTENDED_TYPE defines, but really we can overdo this :-)
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
On Tue, Mar 06, 2018 at 07:22:51PM +0100, Daniel Vetter wrote:
Hmm. Yeah, I guess that kind of a fail is fairly unlikely because the defines won't be there. Would require the driver to basically pass in utter garbage instead of just a bad combination of existing flags.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks. Series pushed to drm-misc-next.
On Tue, Mar 06, 2018 at 06:48:44PM +0200, Ville Syrjala wrote:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Ville Syrjala -
Ville Syrjälä