Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/bridge: analogix_dp: leak of timings in analogix_dp_aux_done_probing() - [Low] drm/bridge: analogix_dp: missing Kconfig selects for display mode bridge --
commit a6bc11591ee5c87dfb3184d18836932fc5960d45 Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Use bridge->next_bridge instead of plat_data field This commit replaces dp->plat_data->next_bridge with dp->bridge.next_bridge in the analogix_dp driver and moves the Exynos display-timings bridge setup from exynos_dp_probe() into analogix_dp_aux_done_probing(). Link: https://lore.kernel.org/all/5609825.iZASKD2KPV@diego/ > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 38857508d9392..853b6a6a7fe4d 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c [ ... ] > @@ -1655,37 +1658,34 @@ struct drm_dp_aux *analogix_dp_get_aux(struct > analogix_dp_device *dp) > } > EXPORT_SYMBOL_GPL(analogix_dp_get_aux); > > -static void analogix_dp_put_bridge(void *data) > -{ > - drm_bridge_put(data); > -} > - > static int analogix_dp_aux_done_probing(struct drm_dp_aux *aux) > { > struct analogix_dp_device *dp = to_dp(aux); > struct analogix_dp_plat_data *plat_data = dp->plat_data; > + struct drm_bridge *bridge = &dp->bridge, *next_bridge; > int port = plat_data->dev_type == EXYNOS_DP ? 0 : 1; > - int ret; > + > + if (plat_data->dev_type == EXYNOS_DP && > of_get_display_timings(dp->dev->of_node)) { > + next_bridge = devm_drm_of_display_mode_bridge(dp->dev, > dp->dev->of_node, > + > DRM_MODE_CONNECTOR_eDP); [Severity: Low] Will moving of_get_display_timings() and devm_drm_of_display_mode_bridge() into analogix_dp_aux_done_probing() cause undefined symbol errors at link time when CONFIG_DRM_EXYNOS_DP is not enabled? Previously these calls lived in drivers/gpu/drm/exynos/exynos_dp.c under CONFIG_DRM_EXYNOS_DP, where DRM_EXYNOS_DP selects DRM_OF_DISPLAY_MODE_BRIDGE and DRM_EXYNOS selects VIDEOMODE_HELPERS. drivers/gpu/drm/bridge/analogix/analogix_dp_core.c is built under CONFIG_DRM_ANALOGIX_DP, which only selects DRM_DISPLAY_DP_AUX_BUS: drivers/gpu/drm/bridge/analogix/Kconfig: config DRM_ANALOGIX_DP tristate depends on DRM depends on OF select DRM_DISPLAY_DP_AUX_BUS Because plat_data->dev_type == EXYNOS_DP is evaluated at runtime, the compiler emits references to both devm_drm_of_display_mode_bridge() (which has no stub in include/drm/bridge/of-display-mode-bridge.h) and of_get_display_timings() (declared whenever CONFIG_OF is enabled, while drivers/video/of_display_timing.o requires CONFIG_VIDEOMODE_HELPERS). Configurations such as CONFIG_ROCKCHIP_ANALOGIX_DP without CONFIG_DRM_EXYNOS_DP, or CONFIG_DRM_ANALOGIX_DP=y with CONFIG_DRM_EXYNOS=m, will fail to link unless DRM_ANALOGIX_DP selects DRM_OF_DISPLAY_MODE_BRIDGE and VIDEOMODE_HELPERS (or stubs are provided). [Severity: Medium] Also in analogix_dp_aux_done_probing(), does calling of_get_display_timings(dp->dev->of_node) directly in the condition leak the allocated struct display_timings? In drivers/video/of_display_timing.c, of_get_display_timings() allocates struct display_timings, disp->timings, and each struct display_timing entry, expecting the caller to free them with display_timings_release(): drivers/video/of_display_timing.c:of_get_display_timings() { ... timings_np = of_get_child_by_name(np, "display-timings"); if (!timings_np) { pr_err("%pOF: could not find display-timings node\n", np); return NULL; } disp = kzalloc_obj(*disp); ... } Because the returned pointer is not saved or passed to display_timings_release(), those allocations are leaked on Exynos DP boards with a display-timings node (and devm_drm_of_display_mode_bridge() will parse the node a second time via of_get_drm_display_mode()). In addition, when no display-timings child node is present on Exynos boards using a panel or bridge endpoint, of_get_display_timings() prints a spurious error message. > + if (IS_ERR(next_bridge)) > + return PTR_ERR(next_bridge); > + bridge->next_bridge = drm_bridge_get(next_bridge); > + > + return component_add(dp->dev, plat_data->ops); > + } [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
