Re: [PATCH v2 02/37] drm/bridge: analogix-anx6345: Switch to atomic bridge callbacks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The panel is enabled before it is prepared (powered on) in the bridge enable sequence, violating the DRM panel API contracts and causing potential hardware issues.
- [High] Bridge `atomic_enable` and `atomic_disable` callbacks invoke sleeping functions, violating atomic context constraints.
--

--- Patch [2]: [PATCH v2 02/37] drm/bridge: analogix-anx6345: Switch to atomic bridge callbacks ---
commit c3398824f07e9152c87164f9494fed0231949fb3
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: analogix-anx6345: Switch to atomic bridge callbacks
    
The analogix-anx6345 bridge uses the legacy non-atomic enable and
disable bridge callbacks.
    
In order to remove the legacy bridge callback support from the DRM
bridge core, switch to their atomic counterparts and add the bridge
atomic state handlers.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> index f3fe47b12edca..eba5c6dcb5ada 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
[ ... ]
> @@ -580,7 +581,8 @@ static void anx6345_bridge_disable(struct drm_bridge *bridge)
>  		anx6345_poweroff(anx6345);
>  }
>  
> -static void anx6345_bridge_enable(struct drm_bridge *bridge)
> +static void anx6345_bridge_enable(struct drm_bridge *bridge,
> +				  struct drm_atomic_commit *commit)
>  {
>  	struct anx6345 *anx6345 = bridge_to_anx6345(bridge);
>  	int err;
> @@ -600,11 +602,14 @@ static void anx6345_bridge_enable(struct drm_bridge *bridge)

[Severity: High]
This is a pre-existing issue, but does this function enable the panel before
it is prepared?

In anx6345_bridge_enable(), we have:

	if (anx6345->panel)
		drm_panel_enable(anx6345->panel);

	err = anx6345_start(anx6345);

And anx6345_start() eventually calls anx6345_poweron() which prepares the
panel:

	if (anx6345->panel)
		drm_panel_prepare(anx6345->panel);

On a secondary modeset (after the device has been disabled),
anx6345->powered is false. anx6345_bridge_enable() calls drm_panel_enable()
which turns on the backlight. It then calls anx6345_start() which invokes
anx6345_poweron() and drm_panel_prepare() to power on the LCD logic.

Could this inverted enable -> prepare sequence turn on the backlight while
the panel power is off, potentially causing latch-up and physically damaging
the LCD logic on some displays?

>  }
>  
>  static const struct drm_bridge_funcs anx6345_bridge_funcs = {
> +	.atomic_create_state = drm_atomic_helper_bridge_create_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.attach = anx6345_bridge_attach,
>  	.detach = anx6345_bridge_detach,
>  	.mode_valid = anx6345_bridge_mode_valid,
> -	.disable = anx6345_bridge_disable,
> -	.enable = anx6345_bridge_enable,
> +	.atomic_disable = anx6345_bridge_disable,
> +	.atomic_enable = anx6345_bridge_enable,

[Severity: High]
This is a pre-existing issue, but does mapping these callbacks to atomic
contexts conflict with the sleeping functions they invoke?

The bridge atomic_enable and atomic_disable callbacks execute in the context
of atomic commits (e.g., drm_atomic_helper_commit_tail), where sleeping is
forbidden.

However, anx6345_bridge_enable() and anx6345_bridge_disable() invoke
hardware sequencing functions like anx6345_start(), anx6345_poweron(), and
anx6345_poweroff().

For example, anx6345_start() contains:

	usleep_range(10000, 15000);

And anx6345_poweron() contains:

	gpiod_set_value_cansleep(anx6345->gpiod_reset, 1);
	usleep_range(1000, 2000);

Could calling these sleeping functions from the atomic context cause kernel
panics or BUG warnings (scheduling while atomic)?

>  };
>  
>  static void unregister_i2c_dummy_clients(struct anx6345 *anx6345)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-drm-all-atomic-bridges-v2-0-21d03cbca446@kernel.org?part=2