Re: [PATCH v4 11/14] drm/bridge: analogix_dp: Use bridge->next_bridge instead of plat_data field

[sashiko-bot@kernel.org]

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 <damon.ding@rock-chips.com>

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/20261019085836.3065008-1-damon.ding@rock-chips.com?part=11