Re: [PATCH v2 2/6] drm/sysfb: simpledrm: Improve panel-size validation

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

On Mon, Jun 22, 2026 at 03:19:36PM +0200, Thomas Zimmermann wrote:
> Validate the panel size from the device-tree node against the
> limitations of struct drm_display_mode. The type only stores sizes
> in 16-bit fields. Fail transparently on errors; do not warn.
> 
> v2:
> - only use initialized values in debugging output (Sashiko)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 2a6d731a8f16 ("drm/simpledrm: Allow physical width and height configuration via panel node")
> Cc: Rayyan Ansari <rayyan@ansari.sh>
> Cc: <stable@vger.kernel.org> # v6.4+
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 40 ++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 15dcafa9d524..fa2121f81def 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -193,6 +193,40 @@ simplefb_get_memory_of(struct drm_device *dev, struct device_node *of_node)
>  	return res;
>  }
>  
> +static u16
> +__simplefb_get_panel_size_mm_of(struct drm_device *dev, struct device_node *of_panel_node,
> +				const char *name)
> +{
> +	int ret;
> +	u32 value;
> +
> +	ret = of_property_read_u32(of_panel_node, name, &value);
> +	if (ret) {
> +		drm_dbg(dev, "simplefb: cannot parse panel %s: error %d\n",
> +			name, ret);
> +		return 0; /* not an error, simply ignore */
> +	}
> +	if (value > U16_MAX) {
> +		drm_dbg(dev, "simplefb: panel %s of %u exceeds maximum value\n",
> +			name, value);
> +		return 0; /* not an error, simply ignore */

I wonder if it's perhaps better to move this comment to the function
scope and explain why this can be ignored. I didn't know and had to go
look at drm_sysfb_mode() to see that if these are 0, it'll compute the
physical dimensions based on a default of 96 DPI.

> +	}
> +
> +	return value;
> +}
> +
> +static u16
> +simplefb_get_panel_width_mm_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "width-mm");
> +}
> +
> +static u16
> +simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
> +}
> +
>  /*
>   * Simple Framebuffer device
>   */
> @@ -594,7 +628,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  	struct drm_sysfb_device *sysfb;
>  	struct drm_device *dev;
>  	int width, height, stride;
> -	int width_mm = 0, height_mm = 0;
> +	u16 width_mm = 0, height_mm = 0;
>  	struct device_node *panel_node;
>  	const struct drm_format_info *format;
>  	struct resource *res, *mem = NULL;
> @@ -658,8 +692,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  			return ERR_CAST(mem);
>  		panel_node = of_parse_phandle(of_node, "panel", 0);
>  		if (panel_node) {
> -			simplefb_read_u32_of(dev, panel_node, "width-mm", &width_mm);
> -			simplefb_read_u32_of(dev, panel_node, "height-mm", &height_mm);
> +			width_mm = simplefb_get_panel_width_mm_of(dev, panel_node);
> +			height_mm = simplefb_get_panel_height_mm_of(dev, panel_node);
>  			of_node_put(panel_node);
>  		}
>  	} else {

The drm_sysfb_mode() function that width_mm and height_mm get passed
into accepts them as unsigned int, so maybe that should be changed as
well for more consistency?

In either case, since they all end up in the u16 in the struct, it's
obviously correct to check for the range when parsing, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]