Re: [PATCH 05/11] drm/bridge: panel: add a panel_bridge to every panel

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

New issues:
- [Medium] Incorrect error check `WARN_ON(!panel->bridge)` masks bridge allocation failures, leading to invalid state.

Pre-existing issues:
- [High] Architectural Use-After-Free on panel memory when the panel unbinds.
--

--- Patch [5]: [PATCH 05/11] drm/bridge: panel: add a panel_bridge to every panel ---
commit fe7eeb215d441faca3cc4f0f4f95d9a96a4fb587
Author: Luca Ceresoli <[email protected]>

drm/bridge: panel: add a panel_bridge to every panel

Adding a panel does currently not add a panel_bridge wrapping it. Usually
the panel_bridge creation happens later, when some other driver (e.g. the
previous bridge or the encoder) calls *_of_get_bridge() and the following
element in the pipeline is a panel.

[ ... ]

Switch to the new approach: always create a panel_bridge with a drm_panel,
thus matching the lifetime of the drm_panel and the panel_bridge wrapping
it. This makes lifetime much more straightforward to understand and to
further develop on.

[Severity: High]
This is a pre-existing issue, but does the underlying bridge allocation
architecture have a use-after-free risk when panels unbind?

Looking at drm_panel_bridge_add_typed() in drivers/gpu/drm/bridge/panel.c:

struct drm_bridge *drm_panel_bridge_add_typed(...)
{
        ...
        panel_bridge = devm_drm_bridge_alloc(panel->dev, struct panel_bridge,
                                             bridge,
                                             &panel_bridge_bridge_funcs);
        ...
}

Since devres automatically frees the bridge memory when the panel unbinds
(e.g., via rmmod), this completely ignores the bridge->refcount kref. For
child-parent topologies like DSI panels where fw_devlink cannot enforce
unbind ordering, couldn't the DSI host remain bound and retain a dangling
pointer to the freed bridge in its chain?

[ ... ]

> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f8f6082e637fd..95fc95d1b4b7a 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -453,6 +453,9 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
>  
>  	drm_panel_init(panel, dev, funcs, connector_type);
>  
> +	panel->bridge = devm_drm_panel_bridge_add(dev, panel);
> +	WARN_ON(!panel->bridge);

[Severity: Medium]
Will this check actually catch failures? devm_drm_panel_bridge_add()
returns an ERR_PTR on failure, rather than NULL.

If the allocation fails due to memory pressure or an invalid connector
type, the warning will be silently bypassed since an ERR_PTR evaluates to
true.

The panel driver will then complete allocation successfully but with an
error pointer in panel->bridge, which can cause display controllers to
defer probing indefinitely without any error logs.

Should this use IS_ERR() instead of checking for NULL?

> +
>  	return container;
>  }
>  EXPORT_SYMBOL(__devm_drm_panel_alloc);

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