Re: [PATCH] drm/client: Don't print a NULL firmware config mode

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 22.10.26 um 13:43 schrieb Hugo Sales:
> When some tiles of a tiled display are missing,
> drm_client_firmware_config() replaces the connector's mode with
> drm_connector_fallback_non_tiled_mode(). That returns NULL when
> connector->modes has no mode other than the tile size, but the debug
> message after it still reads modes[i]->name. Accessing a member through
> a NULL pointer is undefined behaviour. Today printk happens to get an
> address below PAGE_SIZE and prints "(efault)"
>
> Print which step found no mode instead, "no non tiled mode found" in
> this case, from the mode_type the function already tracks. A connector
> whose mode is NULL is skipped by drm_client_modeset_probe(), so it is
> left off, as before
>
> Sashiko found this while reviewing a series that makes the fallback
> return NULL more often, by also skipping modes larger than the client
>
> Fixes: cf1d0180f571 ("drm/fbdev: Fallback to non tiled mode if all tiles not present")
> Link: https://lore.kernel.org/all/sashiko-outbox-173425@kernel.org/
> Signed-off-by: Hugo Sales <hugo@hsal.es>
> ---
> Sashiko found this while reviewing my fbdev probe series:
> https://lore.kernel.org/all/20261015212545.1989962-1-hugo@hsal.es/
> That series makes the fallback return NULL more often
>
> I ran checkpatch and built drm_client_modeset.o with W=1
>
>   drivers/gpu/drm/drm_client_modeset.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 0080a8e9556a..a8cbe4c0e166 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -754,10 +754,15 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   		}
>   		crtcs[i] = crtc;
>   
> -		drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
> -			    connector->base.id, connector->name,
> -			    crtc->base.id, crtc->name,
> -			    mode_type, modes[i]->name);
> +		if (modes[i])
> +			drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
> +				    connector->base.id, connector->name,
> +				    crtc->base.id, crtc->name,
> +				    mode_type, modes[i]->name);
> +		else
> +			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] on [CRTC:%d:%s] no %s mode found\n",
> +				    connector->base.id, connector->name,
> +				    crtc->base.id, crtc->name, mode_type);

That seems excessive. Can you use   modes[i] ? modes[i]->name : 
"(none)"  for the final argument?

Best regards
Thomas

>   
>   		fallback = false;
>   		conn_configured |= BIT(i);
>
> base-commit: cf5d8e390e2ac2cfa24a50645efdbb9f0eebe65f

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)