Re: [PATCH v2 13/37] drm/bridge: microchip-lvds: Add atomic bridge state handlers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing `.remove` hook for `drm_bridge_remove` causes a use-after-free upon module unload or device unbind.
- [High] Sleeping function usleep_range() is called from an atomic commit path.
- [High] Incorrect masking of the WPKEY password truncates the unlock key, preventing hardware configuration.
--

--- Patch [13]: [PATCH v2 13/37] drm/bridge: microchip-lvds: Add atomic bridge state handlers ---
commit 489328dd1eddcba7ecc56ad3f142d3a6ed0da2f5
Author: Maxime Ripard <mripard@kernel.org>
drm/bridge: microchip-lvds: Add atomic bridge state handlers

The microchip-lvds bridge doesn't implement any enable or disable
callbacks, but is still missing the atomic state handlers. In order to
remove the legacy bridge callback support from the DRM bridge core, add
them.

> diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c b/drivers/gpu/drm/bridge/microchip-lvds.c
> index 5fb8633f43c5c..dd4ffc9f4df3f 100644
> --- a/drivers/gpu/drm/bridge/microchip-lvds.c
> +++ b/drivers/gpu/drm/bridge/microchip-lvds.c
> @@ -167,6 +167,9 @@ static void mchp_lvds_atomic_disable(struct drm_bridge *bridge,
>  }
>  
>  static const struct drm_bridge_funcs mchp_lvds_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 = mchp_lvds_attach,
>  	.atomic_enable = mchp_lvds_atomic_enable,

[Severity: High]
This isn't a bug introduced by this patch, but does the atomic enable path
safely handle sleeping functions?

The subsystem guide states that bridge atomic enable callbacks run in
atomic context. However, mchp_lvds_atomic_enable() calls
lvds_serialiser_on(), which has a hardware polling loop using usleep_range():

drivers/gpu/drm/bridge/microchip-lvds.c:lvds_serialiser_on() {
	...
	usleep_range(1000, 2000);
	...
}

Can this cause kernel warnings or potential deadlocks during the atomic
commit path?

>  	.atomic_disable = mchp_lvds_atomic_disable,
>  };

[Severity: High]
This is a pre-existing issue, but is the WPKEY password being truncated
during initialization?

In lvds_serialiser_on(), the password is masked directly without shifting:

drivers/gpu/drm/bridge/microchip-lvds.c:lvds_serialiser_on() {
	...
	lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD &
				       LVDSC_WPMR_WPKEY_MASK));
	...
}

If LVDSC_WPMR_WPKEY_PSSWD is 0x4C5644 and LVDSC_WPMR_WPKEY_MASK is
GENMASK(31, 8) (0xFFFFFF00), the lower byte 'D' (0x44) gets truncated to
0x00. 

Does this prevent the write protection from being unlocked, causing
subsequent LVDS configuration writes to be ignored by the hardware?

[Severity: Critical]
This is also a pre-existing issue, but does the driver leave the bridge
registered on unload?

The bridge is registered globally in mchp_lvds_probe() using
drm_bridge_add(), but the platform driver lacks a .remove callback:

drivers/gpu/drm/bridge/microchip-lvds.c:mchp_lvds_driver {
	.probe = mchp_lvds_probe,
	.driver = {
		.name = "microchip-lvds",
		.of_match_table = mchp_lvds_dt_ids,
	},
};

Since drm_bridge_remove() is never called to unlink the bridge, and the
mchp_lvds structure is freed automatically by devres, could this lead to a
use-after-free the next time the DRM core traverses the global bridge list?

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