On Mon, Oct 19, 2026 at 10:34:06AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
> - [High] drm/bridge: simple-bridge: missing atomic_check() to propagate DE
> bus flags
> - [Medium] drm/bridge: simple-bridge: timings devm_kzalloc() lifetime mismatch
> --
>
> commit 17d03fa8dcd99e5bb3af7da622344c856ca2c887
> Author: Leonardo Costa <[email protected]>
>
> drm/bridge: simple-bridge: Support data-enable-active endpoint property
>
> This commit reads the data-enable-active property from the input endpoint in
> simple_bridge_probe() and allocates a drm_bridge_timings structure to set
> DRM_BUS_FLAG_DE_HIGH or DRM_BUS_FLAG_DE_LOW in input_bus_flags when the bridge
> does not already define its own timings.
>
> > diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> > b/drivers/gpu/drm/bridge/simple-bridge.c
> > index 3fba7002322e9..393228bed6e28 100644
> > --- a/drivers/gpu/drm/bridge/simple-bridge.c
> > +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> > @@ -171,6 +171,9 @@ static int simple_bridge_probe(struct platform_device
> > *pdev)
> > {
> > struct simple_bridge *sbridge;
> > struct device_node *remote;
> > + struct device_node *ep;
> > + bool de_present;
> > + u32 de;
> >
> > sbridge = devm_drm_bridge_alloc(&pdev->dev, struct simple_bridge,
> > bridge, &simple_bridge_bridge_funcs);
> > @@ -208,10 +211,26 @@ static int simple_bridge_probe(struct platform_device
> > *pdev)
> > return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->enable),
> > "Unable to retrieve enable GPIO\n");
> >
> > - /* Register the bridge. */
> > sbridge->bridge.of_node = pdev->dev.of_node;
> > sbridge->bridge.timings = sbridge->info->timings;
> >
> > + ep = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
> > + de_present = !of_property_read_u32(ep, "data-enable-active", &de);
> > + of_node_put(ep);
> > +
> > + if (de_present && !sbridge->bridge.timings) {
> > + struct drm_bridge_timings *timings;
> > +
> > + timings = devm_kzalloc(&pdev->dev, sizeof(*timings),
> > GFP_KERNEL);
> > + if (!timings)
> > + return -ENOMEM;
>
> [Severity: Medium]
> Can allocating timings with devm_kzalloc() in simple_bridge_probe() cause a
> lifetime mismatch with sbridge->bridge?
>
> sbridge is allocated via devm_drm_bridge_alloc(), which manages the
> container's
> lifetime through sbridge->bridge.refcount and only frees it in
> __drm_bridge_free() after all drm_bridge references are dropped:
>
> drivers/gpu/drm/drm_bridge.c:__drm_bridge_free()
> if (bridge->funcs->destroy)
> bridge->funcs->destroy(bridge);
>
> drm_bridge_put(bridge->next_bridge);
>
> kfree(bridge->container);
>
> If pdev is unbound while an upstream bridge or encoder still holds a reference
> to sbridge->bridge, devres_release_all() will free timings immediately,
> leaving
> sbridge->bridge.timings as a dangling pointer that can be dereferenced by
> upstream drivers (for example, in imx8mp_hdmi_pvi_bridge_enable()).
>
> Would embedding struct drm_bridge_timings inside struct simple_bridge (as done
> in lvds-codec.c and ti-tfp410.c), or pointing to static const
> drm_bridge_timings instances for DRM_BUS_FLAG_DE_HIGH and DRM_BUS_FLAG_DE_LOW,
> avoid this lifetime mismatch?
This seemed to make sense to me, but I'm having trouble implementing either of
Sashiko's suggestions in a proper way, as both seemed problematic to me when
actually trying to make the changes.
Adding static const drm_bridge_timings instances for each of the flags goes
against the current pattern of using these to list configurations of actual
pieces of hardware.
On the other hand, embedding struct drm_bridge_timings inside
struct simple_bridge leads to duplicating information in the driver. It would
look something like in the diff below. In this case, when the bridge provides
a timings struct, the embedded drm_bridge_timings needs to receive a copy of it,
despite serving no purpose in this situation, it seems to me to add too much
complexity.
Laurent, do you think this is still better than adding a compatible for the
Apalis iMX6 VGA bridge, as was done in V1? Let me know if you think any of
these concerns are not actual issues, or if you think there is a pattern that
better fits this driver.
Best regards,
Leonardo
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
b/drivers/gpu/drm/bridge/simple-bridge.c
index 848c2df2a276..9787eb7f7b82 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -28,6 +28,7 @@ struct simple_bridge_info {
struct simple_bridge {
struct drm_bridge bridge;
struct drm_connector connector;
+ struct drm_bridge_timings timings;
const struct simple_bridge_info *info;
@@ -183,6 +184,8 @@ static int simple_bridge_probe(struct platform_device *pdev)
{
struct simple_bridge *sbridge;
struct device_node *remote;
+ struct device_node *ep;
+ u32 de;
sbridge = devm_drm_bridge_alloc(&pdev->dev, struct simple_bridge,
bridge, &simple_bridge_bridge_funcs);
@@ -220,10 +223,24 @@ static int simple_bridge_probe(struct platform_device
*pdev)
return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->enable),
"Unable to retrieve enable GPIO\n");
- /* Register the bridge. */
sbridge->bridge.of_node = pdev->dev.of_node;
sbridge->bridge.timings = sbridge->info->timings;
+ if (sbridge->info->timings)
+ sbridge->timings = *sbridge->info->timings;
+
+ ep = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
+
+ if (!of_property_read_u32(ep, "data-enable-active", &de) &&
+ !sbridge->bridge.timings) {
+ sbridge->timings.input_bus_flags |= de ? DRM_BUS_FLAG_DE_HIGH :
+ DRM_BUS_FLAG_DE_LOW;
+ sbridge->bridge.timings = &sbridge->timings;
+ }
+
+ of_node_put(ep);
+
+ /* Register the bridge. */
return devm_drm_bridge_add(&pdev->dev, &sbridge->bridge);
}
> > +
> > + timings->input_bus_flags = de ? DRM_BUS_FLAG_DE_HIGH :
> > + DRM_BUS_FLAG_DE_LOW;
> > + sbridge->bridge.timings = timings;
> > + }
>
> [Severity: High]
> Will setting only sbridge->bridge.timings->input_bus_flags actually propagate
> the DE polarity to upstream atomic encoders and bridges?
>
> simple_bridge_bridge_funcs registers atomic state callbacks without an
> .atomic_check callback:
>
> drivers/gpu/drm/bridge/simple-bridge.c:
> static const struct drm_bridge_funcs simple_bridge_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 = simple_bridge_attach,
> .atomic_enable = simple_bridge_enable,
> .atomic_disable = simple_bridge_disable,
> };
>
> During an atomic modeset, drm_atomic_bridge_chain_check() calls
> drm_atomic_bridge_propagate_bus_flags(), which overwrites
> bridge_state->input_bus_cfg.flags with output_flags from the downstream bridge
> or connector (0 for a VGA connector) and never checks
> bridge->timings->input_bus_flags:
>
> drivers/gpu/drm/drm_bridge.c:drm_atomic_bridge_propagate_bus_flags()
> bridge_state->output_bus_cfg.flags = output_flags;
> ...
> bridge_state->input_bus_cfg.flags = output_flags;
>
> Because simple-bridge has a valid drm_bridge_state, upstream bridges and
> atomic
> encoders such as imx_pd_bridge_atomic_check() in
> drivers/gpu/drm/imx/ipuv3/parallel-display.c read
> next_bridge_state->input_bus_cfg.flags instead of
> next_bridge->timings->input_bus_flags:
>
> drivers/gpu/drm/imx/ipuv3/parallel-display.c:imx_pd_bridge_atomic_check()
> if (next_bridge_state)
> bus_flags = next_bridge_state->input_bus_cfg.flags;
> else
> bus_flags = di->bus_flags;
>
> As a result, bus_flags remains 0 instead of DRM_BUS_FLAG_DE_LOW or
> DRM_BUS_FLAG_DE_HIGH.
>
> Should simple_bridge_bridge_funcs implement an .atomic_check callback that
> copies sbridge->bridge.timings->input_bus_flags into
> bridge_state->input_bus_cfg.flags (similar to tfp410_atomic_check() in
> drivers/gpu/drm/bridge/ti-tfp410.c)?
>
> > +
> > + /* Register the bridge. */
> > return devm_drm_bridge_add(&pdev->dev, &sbridge->bridge);
> > }
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=2
This second finding was a false positive, as per my response in the 3/3
patch.
