Re: [PATCH v4 5/7] drm/tegra: rgb: Parameterize configuration based on bus flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/tegra: unlocked bridge state access in tegra_rgb_encoder_enable()
--

commit 743a0ce150c28976e990ee80b4eaf7239b83fcc3
Author: Svyatoslav Ryhel <clamor95@gmail.com>

drm/tegra: rgb: Parameterize configuration based on bus flags

This commit updates tegra_rgb_encoder_enable() to configure pixel clock
polarity, display enable polarity, and data alignment in the Tegra display
controller registers using bus flags retrieved from the attached bridge state.

> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index b6d09a8d8e5ea..da9e37fe36689 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c

[ ... ]

> @@ -104,14 +105,24 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>  	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
>  	struct tegra_output *output = encoder_to_output(encoder);
>  	struct tegra_rgb *rgb = to_rgb(output);
> -	u32 value;
> +	struct drm_bridge_state *bridge_state;
> +	u32 bus_flags, value;
> +
> +	/* Get but flags from the bridge state. */
> +	bridge_state = drm_bridge_get_current_state(output->bridge);
> +	if (bridge_state)
> +		bus_flags = bridge_state->input_bus_cfg.flags;
> +	else
> +		bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +			    DRM_BUS_FLAG_DE_HIGH |
> +			    DRM_BUS_FLAG_DATA_MSB_TO_LSB;

[Severity: High]
Does calling drm_bridge_get_current_state(output->bridge) from
tegra_rgb_encoder_enable() in drivers/gpu/drm/tegra/rgb.c access the bridge
state without holding output->bridge->base.lock?

Looking at drm_bridge_get_current_state(), it asserts that bridge->base.lock
is held before reading bridge->base.state:

include/drm/drm_bridge.h:drm_bridge_get_current_state() {
	if (!bridge)
		return NULL;

	drm_modeset_lock_assert_held(&bridge->base.lock);

	if (!bridge->base.state)
		return NULL;

	return drm_priv_to_bridge_state(bridge->base.state);
}

There appear to be two paths where output->bridge->base.lock is not held when
tegra_rgb_encoder_enable() runs:

1. Non-blocking atomic modeset commits (DRM_IOCTL_MODE_ATOMIC with
DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_ATOMIC_ALLOW_MODESET):
tegra_drm_mode_config_funcs.atomic_commit uses drm_atomic_helper_commit() in
drivers/gpu/drm/drm_atomic_helper.c, which swaps state via
drm_atomic_helper_swap_state() and schedules state->commit_work onto
system_dfl_wq, after which drm_mode_atomic_ioctl() in
drivers/gpu/drm/drm_atomic_uapi.c immediately drops all acquired modeset
locks:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_commit() {
	...
	drm_atomic_commit_get(state);
	if (nonblock)
		queue_work(system_dfl_wq, &state->commit_work);
	else
		commit_tail(state, false);
	...
}

drivers/gpu/drm/drm_atomic_uapi.c:drm_mode_atomic_ioctl() {
	...
	drm_modeset_drop_locks(&ctx);
	drm_modeset_acquire_fini(&ctx);
	...
}

When the system_dfl_wq worker executes
commit_tail() -> tegra_atomic_commit_tail() ->
drm_atomic_helper_commit_modeset_enables() ->
drm_atomic_helper_commit_encoder_bridge_enable() ->
tegra_rgb_encoder_enable(), no drm_modeset_lock is held by the worker thread.

2. Bridges without .atomic_duplicate_state during synchronous modeset commits:
drm_bridge_attach() always initializes bridge->base.lock via
drm_atomic_private_obj_init(), but drm_atomic_add_encoder_bridges() skips
acquiring bridge->base.lock when the bridge does not implement
.atomic_duplicate_state:

drivers/gpu/drm/drm_atomic.c:drm_atomic_add_encoder_bridges() {
	...
	drm_for_each_bridge_in_chain(encoder, bridge) {
		/* Skip bridges that don't implement the atomic state hooks. */
		if (!bridge->funcs->atomic_duplicate_state)
			continue;

		bridge_state = drm_atomic_get_bridge_state(state, bridge);
	...
}

In both cases, calling drm_bridge_get_current_state(output->bridge) triggers a
lockdep warning on drm_modeset_lock_assert_held(&bridge->base.lock) under
CONFIG_PROVE_LOCKING and reads output->bridge->base.state without holding
bridge->base.lock.

Should tegra_rgb_encoder_helper_funcs implement .atomic_enable() instead of
.enable() and look up the bridge state from the atomic state via
drm_atomic_get_new_bridge_state(state, output->bridge)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022132259.240717-1-clamor95@gmail.com?part=5