Re: [PATCH v5 21/38] drm/vkms: Introduce configfs for plane color range

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ConfigFS store callbacks enforce base 10 parsing, which rejects documented hexadecimal bitmask inputs.
- [Low] kstrtouint() is called with an 'int *' instead of 'unsigned int *', generating an incompatible pointer type warning.
- [Low] ConfigFS 'show' callbacks omit a trailing newline.
--

commit 75e6ca29907842441c00bd1bce2c7956d8a33c79
Author: Louis Chauvet <louis.chauvet@bootlin.com>

drm/vkms: Introduce configfs for plane color range
    
To allows the userspace to test many hardware configuration, introduce a
new interface to configure the available color ranges per planes. VKMS
supports multiple color ranges, so the userspace can choose any
combination.

> diff --git a/Documentation/ABI/testing/configfs-vkms b/Documentation/ABI/testing/configfs-vkms
> index a9ee05fe0c66a..ef0c3da6b3ca1 100644
> --- a/Documentation/ABI/testing/configfs-vkms
> +++ b/Documentation/ABI/testing/configfs-vkms

[ ... ]

> @@ -132,6 +132,21 @@ Description:
>  	Default color encoding presented to userspace, same
>  	values as supported_color_encoding.
>  
> +What:		/sys/kernel/config/vkms/<device>/planes/<plane>/supported_color_ranges
> +Date:		Nov 2025
> +Contact:	dri-devel@lists.freedesktop.org
> +Description:
> +	Available color ranges for the plane, as a bitmask:
> +	0x1 - DRM_COLOR_YCBCR_LIMITED_RANGE,
> +	0x2 - DRM_COLOR_YCBCR_FULL_RANGE.
> +
> +What:		/sys/kernel/config/vkms/<device>/planes/<plane>/default_color_range
> +Date:		Nov 2025
> +Contact:	dri-devel@lists.freedesktop.org
> +Description:
> +	Default color range presented to userspace, same
> +	values as supported_color_ranges.
> +

[ ... ]

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

[ ... ]

> @@ -410,6 +417,85 @@ static ssize_t plane_default_rotation_store(struct config_item *item,
>  	return count;
>  }
>  
> +static ssize_t plane_supported_color_ranges_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
> +	unsigned int supported_color_ranges;
> +
> +	scoped_guard(mutex, &plane->dev->lock)
> +		supported_color_ranges = vkms_config_plane_get_supported_color_ranges(plane->config);
> +
> +	return sprintf(page, "%u", supported_color_ranges);

[Severity: Low]
Does this configfs show callback omit a trailing newline? Usually, configfs 
attributes include a newline in their format string for better readability 
with standard userspace tools.

> +}
> +
> +static ssize_t plane_supported_color_ranges_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]
Is this passing an int pointer to kstrtouint() instead of an unsigned int 
pointer? Will this mismatch generate a compiler warning about incompatible 
pointer types in plane_supported_color_ranges_store()?

[Severity: Medium]
Will this base 10 parsing reject the hexadecimal values documented in the ABI?
The documentation above suggests inputs like 0x1 and 0x2, but using base 10 
here will cause kstrtouint() to reject strings prefixed with 0x. Should this 
use base 0 instead?

> +	if (ret)
> +		return ret;
> +
> +	/* Should be a supported value */
> +	if (val & ~VKMS_SUPPORTED_COLOR_RANGES)
> +		return -EINVAL;
> +	/* Should at least provide one color range */
> +	if ((val & VKMS_SUPPORTED_COLOR_RANGES) == 0)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &plane->dev->lock) {
> +		if (plane->dev->enabled)
> +			return -EBUSY;
> +
> +		vkms_config_plane_set_supported_color_ranges(plane->config, val);
> +	}
> +
> +	return count;
> +}
> +
> +static ssize_t plane_default_color_range_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
> +	unsigned int default_color_range;
> +
> +	scoped_guard(mutex, &plane->dev->lock)
> +		default_color_range = vkms_config_plane_get_default_color_range(plane->config);
> +
> +	return sprintf(page, "%lu", BIT(default_color_range));

[Severity: Low]
Is it missing a newline character here as well?

> +}
> +
> +static ssize_t plane_default_color_range_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 code have the same pointer type mismatch issue as above in 
plane_default_color_range_store(), passing an int pointer to a function 
expecting an unsigned int pointer?

[Severity: Medium]
Also, does this base 10 parsing conflict with the hexadecimal inputs shown in
the ABI documentation?

> +	if (ret)
> +		return ret;
> +
> +	/* Should be a supported value */

[ ... ]

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