Re: [PATCH 56/75] drm/atomic-state-helper: Rename connector_tv_reset to state_init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: Incomplete state decoupling causes NULL deref in tv_margins_state_init
--

commit 44e808ff1921e4945725ba2420217e2015fc2db5
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic-state-helper: Rename connector_tv_reset to state_init

This commit renames drm_atomic_helper_connector_tv_reset to
drm_atomic_helper_connector_tv_state_init. It adds a connector state
parameter to decouple the initialization from the reset hook, preparing it
for contexts like atomic_create_state where the connector state isn't
assigned yet.

> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 6238066fd5d60..1424c4873251c 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
[ ... ]
> @@ -501,16 +501,17 @@ void drm_atomic_helper_connector_tv_margins_state_init(struct drm_connector_stat
>  EXPORT_SYMBOL(drm_atomic_helper_connector_tv_margins_state_init);
>  
>  /**
> - * drm_atomic_helper_connector_tv_reset - Resets Analog TV connector properties
> + * drm_atomic_helper_connector_tv_state_init - Initializes Analog TV connector properties
> + * @state: the DRM State object
>   * @connector: DRM connector
>   *
> - * Resets the analog TV properties attached to a connector
> + * Initializes the analog TV properties attached to a connector
>   */
> -void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector)
> +void drm_atomic_helper_connector_tv_state_init(struct drm_connector_state *state,
> +					       struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> -	struct drm_connector_state *state = connector->state;
>  	struct drm_property *prop;
>  	uint64_t val;
>  
[ ... ]
> @@ -574,7 +575,7 @@ void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector)
>  	drm_atomic_helper_connector_tv_margins_state_init(connector->state,
>  							  connector);

[Severity: High]
Since the new state parameter is added to decouple this function from
the reset hook, should this call pass the newly provided state instead of
connector->state?

When a driver like vc4_vec calls this via its atomic_create_state hook,
connector->state is not yet assigned and is NULL.

Passing connector->state here forwards a NULL pointer to
drm_atomic_helper_connector_tv_margins_state_init(), which then writes
directly to state->tv.margins.left, leading to a NULL pointer dereference
during device probe:

void drm_atomic_helper_connector_tv_margins_state_init(struct drm_connector_state *state,
						       struct drm_connector *connector)
{
	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;

	state->tv.margins.left = cmdline->tv_margins.left;

>  }
> -EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset);
> +EXPORT_SYMBOL(drm_atomic_helper_connector_tv_state_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026823@kernel.org?part=56