Re: [PATCH v2 77/78] drm/bridge: cdns-mhdp8546: Switch to atomic_create_state

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver fails to duplicate subclass-specific state fields (`current_mode`) in `atomic_duplicate_state` and improperly mutates state during `atomic_enable`, leading to data loss and HPD failures on non-modeset commits.
--

--- Patch [77]: [PATCH v2 77/78] drm/bridge: cdns-mhdp8546: Switch to atomic_create_state ---
commit 1fdd589deb947ec0da52f0a1985e0d7446f7455f
Author: Maxime Ripard <[email protected]>

drm/bridge: cdns-mhdp8546: Switch to atomic_create_state
    
The cdns-mhdp8546 bridge atomic_reset implementation is semantically
an atomic_create_state: it allocates and initializes a pristine state
without side effects.

Switch to the atomic_create_state callback, and rename the function
accordingly.

> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index b9574289c247f..504a3186ebb3a 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2051,7 +2051,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.detach = cdns_mhdp_detach,
>  	.atomic_duplicate_state = cdns_mhdp_bridge_atomic_duplicate_state,
>  	.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
> -	.atomic_reset = cdns_mhdp_bridge_atomic_reset,
> +	.atomic_create_state = cdns_mhdp_bridge_atomic_create_state,

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
with how bridge state duplication is handled in
cdns_mhdp_bridge_atomic_duplicate_state().

When a non-modeset commit (like a cursor update) duplicates the state,
cdns_mhdp_bridge_atomic_duplicate_state() allocates a new state but fails to
copy the current_mode pointer from the old state. 

Because it's a non-modeset commit, cdns_mhdp_atomic_enable() (which mutates
the state by setting current_mode) is not invoked. This leaves current_mode
as NULL in the new state, while the old state is destroyed and its
current_mode is freed.

Could this cause hotplug detection and link re-training to fail later when
cdns_mhdp_update_link_status() is triggered, since it encounters the NULL
current_mode and aborts?

>  	.atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
>  	.detect = cdns_mhdp_bridge_detect,
>  	.edid_read = cdns_mhdp_bridge_edid_read,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=77