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? > + > + 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
