Re: [PATCH v2 74/78] drm/atomic-state-helper: Remove drm_atomic_helper_bridge_reset()

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

New issues:
- [Medium] The commit message claims all drivers have been converted to `atomic_create_state`, but two Cadence bridge drivers were missed.
- [Low] Kernel-doc references to `drm_atomic_helper_bridge_reset()` were left intact despite the function being removed.

Pre-existing issues:
- [High] cdns_dsi_bridge_atomic_reset() fails to initialize the private object pointer in the state structure, which can lead to a NULL pointer dereference.
--

--- Patch [74]: [PATCH v2 74/78] drm/atomic-state-helper: Remove drm_atomic_helper_bridge_reset() ---
commit 5bf80d9ba302438fec05db1d7bd3c9f1680d8cd3
Author: Maxime Ripard <[email protected]>

drm/atomic-state-helper: Remove drm_atomic_helper_bridge_reset()

All drivers have been converted to the atomic_create_state callback
and its drm_atomic_helper_bridge_create_state() helper. Remove the
deprecated drm_atomic_helper_bridge_reset().

> All drivers have been converted to the atomic_create_state callback
> and its drm_atomic_helper_bridge_create_state() helper.

[Severity: Medium]
Are all drivers fully converted? Code inspection shows two Cadence bridge
drivers (cdns-dsi-core.c and cdns-mhdp8546-core.c) still assign .atomic_reset
instead of being converted to the new callback.

> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 268cff1faad66..db42c3823b12a 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
[ ... ]
> @@ -905,22 +905,6 @@ void __drm_atomic_helper_bridge_state_init(struct drm_bridge_state *state,
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_bridge_state_init);
>  
> -/**
> - * drm_atomic_helper_bridge_reset() - Allocate and initialize a bridge state
> - *				      to its default
> - * @bridge: the bridge this state refers to
> - *
> - * Allocates the bridge state and initializes it to default values. This helper
> - * is meant to be used as a bridge &drm_bridge_funcs.atomic_reset hook for
> - * bridges that don't subclass the bridge state.
> - */
> -struct drm_bridge_state *
> -drm_atomic_helper_bridge_reset(struct drm_bridge *bridge)
> -{
> -	return drm_atomic_helper_bridge_create_state(bridge);
> -}
> -EXPORT_SYMBOL(drm_atomic_helper_bridge_reset);
> -

[Severity: Low]
Are there still kernel-doc references to drm_atomic_helper_bridge_reset()
that need to be updated?

For example, struct drm_bridge_funcs in include/drm/drm_bridge.h and
drm_atomic_helper_bridge_destroy_state() in
drivers/gpu/drm/drm_atomic_state_helper.c still refer to this removed function.


While reviewing this refactoring, I noticed a pre-existing issue in one of the
Cadence bridge drivers that hasn't been converted yet:

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c:cdns_dsi_bridge_atomic_reset() {
	struct cdns_dsi_bridge_state *dsi_state;

	dsi_state = kzalloc_obj(*dsi_state);
	if (!dsi_state)
		return ERR_PTR(-ENOMEM);

	memset(dsi_state, 0, sizeof(*dsi_state));
	dsi_state->base.bridge = bridge;

	return &dsi_state->base;
}

[Severity: High]
This isn't a bug introduced by this patch, but could this fail to initialize
the private object pointer (state->base.base.obj) and leave it uninitialized
(null)?

Since it does not call __drm_atomic_helper_bridge_state_init() like the core
helpers do, could this lead to a null pointer dereference when attempting to
print the private object state later (for example, on a mode commit failure
or debugfs read)?

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