Re: [PATCH v4 15/20] drm/connector: Add new atomic_create_state callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing fallback to `atomic_create_state` in explicit `reset()` callers causes kernel panics (e.g., in `drmm_connector_hdmi_init`).
- [High] TV connector reset helpers are structurally incompatible with `atomic_create_state`, preventing driver migration.
- [High] Missing NULL check for `atomic_destroy_state` causes kernel panic on driver omission.
--

commit 9c7dcc4b2ba2f3c09ed3926c7e0db8c1a524dd31
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/connector: Add new atomic_create_state callback

This commit introduces the atomic_create_state callback for drm_connectors.
It allows drivers to allocate a pristine state without side-effects, separating
initialization from hardware reset operations during suspend and resume.

> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index e2e5a1b8a8202..07686e94aae00 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c

[ ... ]

> @@ -534,6 +534,32 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
>  
> +/**
> + * drm_atomic_helper_connector_create_state - default &drm_connector_funcs.atomic_create_state hook for connectors
> + * @connector: connector object
> + *
> + * Allocates and  initializes pristine @drm_connector_state.
> + *
> + * This is useful for drivers that don't subclass @drm_connector_state.
> + *
> + * RETURNS:
> + * Pointer to new connector state, or ERR_PTR on failure.
> + */
> +struct drm_connector_state *
> +drm_atomic_helper_connector_create_state(struct drm_connector *connector)
> +{
> +	struct drm_connector_state *state;
> +
> +	state = kzalloc_obj(*state);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	__drm_atomic_helper_connector_state_init(state, connector);
> +
> +	return state;
> +}

How should TV drivers migrating to atomic_create_state handle initialization?

The new pattern requires drivers to allocate and return a pristine state
without assigning it to connector->state. However, existing initialization
helpers like drm_atomic_helper_connector_tv_reset() and
drm_atomic_helper_connector_tv_margins_reset() are hardcoded to read and
modify connector->state directly.

If a TV driver attempts to use these helpers on its newly allocated state,
will the helpers dereference a NULL or stale connector->state and crash?
Would it make sense to introduce state-based variants of these helpers
that take a struct drm_connector_state pointer?

> +EXPORT_SYMBOL(drm_atomic_helper_connector_create_state);

[ ... ]

> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 2e2cd18a14b43..9d240817f8b64 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c

[ ... ]

> +static int drm_mode_config_connector_reset_with_create_state(struct drm_connector *connector)
> +{
> +	if (connector->state) {
> +		connector->funcs->atomic_destroy_state(connector, connector->state);

Is there a risk of a NULL pointer dereference here if a driver migrating to
atomic_create_state omits the atomic_destroy_state hook?

While omitting the hook is a driver bug, other DRM core functions like
drm_connector_cleanup() gracefully guard against this by checking
if (connector->funcs->atomic_destroy_state) before calling it. Should this
wrapper include a similar defensive guard to prevent system crashes?

> +		connector->state = NULL;
> +	}
> +
> +	return drm_mode_config_connector_create_state(connector);
> +}

[ ... ]

> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5ad62c207d009..529755c2e8620 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h

[ ... ]

> @@ -1571,6 +1571,22 @@ struct drm_connector_funcs {
>  	 */
>  	void (*destroy)(struct drm_connector *connector);
>  
> +	/**
> +	 * @atomic_create_state:
> +	 *
> +	 * Allocate a pristine, initialized, state for the connector

Does the introduction of atomic_create_state require updates to other core
functions that explicitly call the reset callback to allocate state?

For example, drmm_connector_hdmi_init() checks for connector->funcs->reset
and calls it. If a driver uses atomic_create_state instead, the reset check
fails and the state is never allocated.

Immediately after, drmm_connector_hdmi_init() calls
drm_connector_attach_max_bpc_property(), which dereferences the uninitialized
connector->state. Can this cause a kernel panic on boot for drivers migrating
to atomic_create_state?

> +	 * object and return it. This callback must have no side
> +	 * effects: in particular, the returned state must not be
> +	 * assigned to the object's state pointer and it must not affect
> +	 * the hardware state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-drm-mode-config-init-v4-0-591dfdcc1bf9@kernel.org?part=15