Re: [PATCH v5 34/38] drm/vkms: Store the enabled/disabled status for connector

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The iterator macros vkms_config_for_each_connector_static and vkms_config_for_each_connector_dynamic introduce a dangling else vulnerability.
- [Low] Kernel-doc comments for the new connector configuration functions contain copy-paste errors, and vkms_config_connector_is_dynamic lacks a const qualifier.
- [Low] The connector's enabled status is printed without a key in `vkms_config_show`, breaking the `key=value` format used for other properties.
--

commit 5e65d90ba630f2831298af2bffa8fd17ac0163ce
Author: Louis Chauvet <louis.chauvet@bootlin.com>

drm/vkms: Store the enabled/disabled status for connector
    
In order to prepare for dynamic connector configuration, we need to store
if a connector is dynamic and if it is enabled.

> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 1a110b8aa114d..98e40c6a2c5b6 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
[ ... ]
> @@ -597,6 +597,8 @@ static int vkms_config_show(struct seq_file *m, void *data)
>  
>  	vkms_config_for_each_connector(vkmsdev->config, connector_cfg) {
>  		seq_puts(m, "connector:\n");
> +		seq_printf(m, "\t%s\n",
> +			   str_enabled_disabled(vkms_config_connector_is_enabled(connector_cfg)));

[Severity: Low]
Does this break the key=value format used for other properties in debugfs?

Surrounding lines output a key, like "\tstatus=%s" or "\ttype=%s", but this
prints the status on a line by itself. Should this be "\tenabled=%s\n" to
maintain consistent formatting for programmatic parsing?

>  		seq_printf(m, "\tstatus=%s\n",
>  			   drm_get_connector_status_name(vkms_config_connector_get_status(connector_cfg)));
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index 7e361b6e45b0e..071887a65380a 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
[ ... ]
> @@ -194,6 +198,24 @@ struct vkms_config_connector {
>  #define vkms_config_for_each_connector(config, connector_cfg) \
>  	list_for_each_entry((connector_cfg), &(config)->connectors, link)
>  
> +/**
> + * vkms_config_for_each_connector_static - Iterate over the static vkms_config connectors
> + * @config: &struct vkms_config pointer
> + * @connector_cfg: &struct vkms_config_connector pointer used as cursor
> + */
> +#define vkms_config_for_each_connector_static(config, connector_cfg) \
> +	vkms_config_for_each_connector((config), (connector_cfg)) \
> +		if (!(connector_cfg)->dynamic)

[Severity: Medium]
Could this macro definition lead to a dangling else vulnerability?

Because the trailing if statement is unbraced, any caller wrapping this
macro in an unbraced if/else block could have their else branch unexpectedly
hijacked by this macro's internal if condition.

> +
> +/**
> + * vkms_config_for_each_connector_dynamic - Iterate over the dynamic vkms_config connectors
> + * @config: &struct vkms_config pointer
> + * @connector_cfg: &struct vkms_config_connector pointer used as cursor
> + */
> +#define vkms_config_for_each_connector_dynamic(config, connector_cfg) \
> +	vkms_config_for_each_connector((config), (connector_cfg)) \
> +		if ((connector_cfg)->dynamic)

[Severity: Medium]
Similarly, this macro also ends in an unbraced if statement. Could this cause
the same latent control flow issue if used in an unbraced conditional block?

[ ... ]
> @@ -472,6 +494,50 @@ vkms_config_connector_set_type(struct vkms_config_connector *connector_cfg,
>  	connector_cfg->type = type;
>  }
>  
> +/**
> + * vkms_config_connector_set_enabled() - If the connector is part of the device
> + * @crtc_cfg: Target connector
> + * @enabled: Add or remove the connector
> + */

[Severity: Low]
This isn't a bug, but there's a typo in the parameter name. Should @crtc_cfg
be @connector_cfg to match the function signature?

> +static inline void
> +vkms_config_connector_set_enabled(struct vkms_config_connector *connector_cfg,
> +				  bool enabled)
> +{
> +	connector_cfg->enabled = enabled;
> +}
[ ... ]
> +/**
> + * vkms_config_connector_set_dynamic() - If the connector is dynamic
> + * @crtc_cfg: Target connector
> + * @enabled: Enable or disable the dynamic status
> + */

[Severity: Low]
Are these parameter names correct? It looks like @crtc_cfg should be
@connector_cfg, and @enabled should be @dynamic.

> +static inline void
> +vkms_config_connector_set_dynamic(struct vkms_config_connector *connector_cfg,
> +				  bool dynamic)
> +{
> +	connector_cfg->dynamic = dynamic;
> +}
> +
> +/**
> + * vkms_config_connector_is_enabled() - If the connector is dynamic
> + * @connector_cfg: The connector
> + */

[Severity: Low]
There's a typo in the function name documented here. Should it be
vkms_config_connector_is_dynamic()?

> +static inline bool
> +vkms_config_connector_is_dynamic(struct vkms_config_connector *connector_cfg)

[Severity: Low]
Is there a reason connector_cfg isn't const here, unlike in the similar
vkms_config_connector_is_enabled() function?

> +{
> +	return connector_cfg->dynamic;
> +}

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