Re: [PATCH v5 19/38] drm/vkms: Introduce configfs for plane color encoding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Rejecting a value of 0 prevents testing planes without color encodings.
- [Low] Passing an `int *` to a function expecting `unsigned int *` causes an incompatible-pointer-types compiler warning.
- [Low] Double assignment typo in variable declaration.
--

commit 14a18de4798714722e237eb41b51bd75e7c7cd56
Author: Louis Chauvet <louis.chauvet@bootlin.com>

drm/vkms: Introduce configfs for plane color encoding
    
This patch introduces a configfs interface to configure the supported and
default color encodings for VKMS planes. It allows userspace to test different
hardware configurations regarding color encoding bitmasks.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 47b3a4aeee8dd..c1d1841b2c5a5 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c

[ ... ]

> @@ -402,14 +410,98 @@ static ssize_t plane_default_rotation_store(struct config_item *item,
>  	return count;
>  }
>  
> +static ssize_t plane_supported_color_encodings_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
> +	unsigned int supported_color_encodings;
> +
> +	scoped_guard(mutex, &plane->dev->lock)
> +		supported_color_encodings = vkms_config_plane_get_supported_color_encodings(plane->config);
> +
> +	return sprintf(page, "%u", supported_color_encodings);
> +}
> +
> +static ssize_t plane_supported_color_encodings_store(struct config_item *item,
> +						     const char *page, size_t count)
> +{
> +	struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
> +	int ret, val = 0;
> +
> +	ret = kstrtouint(page, 10, &val);

[Severity: Low]
Does this pass an int pointer to kstrtouint() instead of an unsigned int
pointer? This might trigger an incompatible-pointer-types compiler warning.

> +	if (ret)
> +		return ret;
> +
> +	/* Should be a supported value */
> +	if (val & ~(VKMS_SUPPORTED_COLOR_ENCODINGS))
> +		return -EINVAL;
> +	/* Should at least provide one color encoding */
> +	if ((val & (VKMS_SUPPORTED_COLOR_ENCODINGS)) == 0)
> +		return -EINVAL;

[Severity: Medium]
Does rejecting 0 here prevent userspace from configuring a VKMS plane without
color encodings?

It looks like the VKMS configuration API and kunit tests support a value of 0 to
emulate an RGB-only plane. Blocking 0 here might unnecessarily limit the hardware
configurations that can be tested.

> +
> +	scoped_guard(mutex, &plane->dev->lock) {
> +		if (plane->dev->enabled)
> +			return -EBUSY;
> +
> +		vkms_config_plane_set_supported_color_encodings(plane->config, val);
> +	}
> +
> +	return count;
> +}
> +
> +static ssize_t plane_default_color_encoding_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_plane *plane = plane = plane_item_to_vkms_configfs_plane(item);

[Severity: Low]
This isn't a bug, but there is an accidental double assignment to the plane
variable here.

> +	unsigned int default_color_encoding;
> +
> +	scoped_guard(mutex, &plane->dev->lock)
> +		default_color_encoding = BIT(vkms_config_plane_get_default_color_encoding(plane->config));
> +
> +	return sprintf(page, "%u", default_color_encoding);
> +}
> +
> +static ssize_t plane_default_color_encoding_store(struct config_item *item,
> +						  const char *page, size_t count)
> +{
> +	struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
> +	int ret, val = 0;
> +
> +	ret = kstrtouint(page, 10, &val);

[Severity: Low]
Similar to plane_supported_color_encodings_store(), does this pass an int
pointer to kstrtouint() instead of an unsigned int pointer?

> +	if (ret)
> +		return ret;
> +
> +	/* Should be a supported value */
> +	if (val & ~VKMS_SUPPORTED_COLOR_ENCODINGS)
> +		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627-vkms-all-config-v5-0-854aa0840926@bootlin.com?part=19