Re: [PATCH v2 25/37] drm/bridge: waveshare-dsi: 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] Freed `regmap` and `backlight` resources are accessed during `atomic_enable` and `atomic_disable` if the driver is unbound while the bridge is still attached.
- [High] The bridge's atomic_enable and atomic_disable callbacks execute sleeping functions, violating atomic context requirements.
--

--- Patch [25]: [PATCH v2 25/37] drm/bridge: waveshare-dsi: Switch to atomic bridge callbacks ---
commit 5ef73541c5abfd37236a332fd681aeaf873d8239
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: waveshare-dsi: Switch to atomic bridge callbacks
    
The waveshare-dsi 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/waveshare-dsi.c b/drivers/gpu/drm/bridge/waveshare-dsi.c
> index ded57f298d640..9c9825c9b7c59 100644
> --- a/drivers/gpu/drm/bridge/waveshare-dsi.c
> +++ b/drivers/gpu/drm/bridge/waveshare-dsi.c

[ ... ]

> @@ -92,7 +93,8 @@ static int ws_bridge_bridge_attach(struct drm_bridge *bridge,
>  				 &ws->bridge, flags);
>  }
>  
> -static void ws_bridge_bridge_enable(struct drm_bridge *bridge)
> +static void ws_bridge_bridge_enable(struct drm_bridge *bridge,
> +				    struct drm_atomic_commit *commit)
>  {
>  	struct ws_bridge *ws = bridge_to_ws_bridge(bridge);
>  
>  	regmap_write(ws->reg_map, 0xad, 0x01);
>  	backlight_enable(ws->backlight);

[Severity: High]
This isn't a bug introduced by this patch, but does accessing ws->reg_map and
ws->backlight here risk a use-after-free if the driver is unbound?

The bridge object is managed by DRM and stays alive as long as it's attached
to the encoder. However, the regmap and backlight resources are device-managed
(devm) and are freed immediately upon driver unbind (e.g., manual unbind via
sysfs or hardware hot-unplug).

If a modeset or DPMS change occurs after unbind, the DRM core calls the
bridge's atomic_enable callback, which dereferences the freed ws->reg_map and
ws->backlight.

[Severity: High]
This is a pre-existing issue, but does calling sleeping functions from this
atomic callback violate context requirements?

According to the DRM subsystem guidelines, drm_atomic_helper_commit_tail()
(which calls these bridge callbacks) executes in atomic context where sleeping
is forbidden.

However, ws_bridge_bridge_enable() calls regmap_write() (which uses an I2C
transfer and acquires a mutex) and backlight_enable() (which acquires a mutex),
both of which can sleep.

>  }
>  
> -static void ws_bridge_bridge_disable(struct drm_bridge *bridge)
> +static void ws_bridge_bridge_disable(struct drm_bridge *bridge,
> +				     struct drm_atomic_commit *commit)
>  {
>  	struct ws_bridge *ws = bridge_to_ws_bridge(bridge);
>  
>  	backlight_disable(ws->backlight);
>  	regmap_write(ws->reg_map, 0xad, 0x00);

[Severity: High]
This is a pre-existing issue, but similar to the enable path, could these
calls result in a use-after-free or violate atomic context requirements?

If a modeset or DPMS change occurs after the driver is unbound, atomic_disable
will dereference the freed ws->reg_map and ws->backlight. Additionally,
calling backlight_disable() and regmap_write() executes sleeping functions in
atomic context.

>  }

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