Re: [PATCH 57/75] drm/bridge: synopsys: dw-hdmi: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: dw-hdmi: missing error check for drm_connector_init_with_ddc
- [Medium] drm/bridge: dw-hdmi: max_bpc state is lost during drm_mode_config_reset()
--

commit 00a6167b722bff3c39b80a7bcee2827035fd652c
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/bridge: synopsys: dw-hdmi: Convert to atomic_create_state

The dw-hdmi driver allocates an initial connector state during creation.
This commit converts the allocation from drm_atomic_helper_connector_reset()
to drm_atomic_helper_connector_create_state() and adds proper error handling.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 917cf764c9c5d..5f11707c67b5c 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2605,7 +2605,15 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
>  	 * drm_connector_attach_max_bpc_property() requires the
>  	 * connector to have a state.
>  	 */
> -	drm_atomic_helper_connector_reset(connector);
> +	if (!connector->state) {
> +		struct drm_connector_state *conn_state;
> +
> +		conn_state = drm_atomic_helper_connector_create_state(connector);
> +		if (IS_ERR(conn_state))
> +			return PTR_ERR(conn_state);
> +
> +		connector->state = conn_state;
> +	}

[Severity: High]
Does this code handle failures from drm_connector_init_with_ddc() correctly?

Looking at dw_hdmi_connector_create(), drm_connector_init_with_ddc() is called
just before this block, but its return value is ignored:

    drm_connector_init_with_ddc(hdmi->bridge.dev, connector,
                                &dw_hdmi_connector_funcs,
                                DRM_MODE_CONNECTOR_HDMIA,
                                hdmi->ddc);

If this initialization fails, connector->base.properties remains uninitialized
(NULL).

When execution continues to drm_connector_attach_max_bpc_property(), it will
attempt to attach the property and dereference connector->base.properties,
resulting in a NULL pointer dereference.

>  
>  	drm_connector_attach_max_bpc_property(connector, 8, 16);

[Severity: Medium]
Will this max_bpc initialization be lost during a drm_mode_config_reset()?

The driver assigns .atomic_create_state =
drm_atomic_helper_connector_create_state in dw_hdmi_connector_funcs without
a custom hook to restore max_bpc.

If a host DRM driver calls drm_mode_config_reset(), the core fallback in
drm_mode_config_connector_reset_with_create_state() will destroy this
initially allocated state and replace it with a newly allocated one:

    if (connector->state) {
        connector->funcs->atomic_destroy_state(connector, connector->state);
        connector->state = NULL;
    }

    return drm_mode_config_connector_create_state(connector);

This wipes out the max_bpc property attached here, resetting it to 0. This
could cause the driver to incorrectly fall back to 8-bit color depth later
in dw_hdmi_bridge_mode_set().

>

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