Re: [PATCH 3/5] drm/rcar-du: dsc: Add rudimentary Renesas R-Car V4H DSC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Resource leak of the `next_bridge` reference on probe failure and driver unbind.
- [Medium] Kconfig logic error prevents the driver from being compiled under `COMPILE_TEST` when `DRM_RCAR_DU` is disabled.
- [Medium] Unbalanced clock disable and reset assertion if initialization fails in `rcar_dsc_atomic_enable()`.
- [Low] Usage of the deprecated `devm_reset_control_get()` API.
--

commit f5db250d6f577d9fcb67af0530df62ba814ec079
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>
drm/rcar-du: dsc: Add rudimentary Renesas R-Car V4H DSC driver

This commit adds a rudimentary VESA DSC encoder driver for the R-Car V4H SoC,
which acts as a pass-through bridge and allows DSI1 to be operational.

> diff --git a/drivers/gpu/drm/renesas/rcar-du/Kconfig b/drivers/gpu/drm/renesas/rcar-du/Kconfig
> index 840305fdeb49a..a51f996d35375 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/renesas/rcar-du/Kconfig

[ ... ]

> @@ -33,6 +33,19 @@ config DRM_RCAR_DW_HDMI
>  	help
>  	  Enable support for R-Car Gen3 or RZ/G2 internal HDMI encoder.
>  
> +config DRM_RCAR_USE_DSC
> +	bool "R-Car DU DSC Encoder Support"
> +	depends on DRM_BRIDGE && OF
> +	depends on DRM_RCAR_DU || COMPILE_TEST
> +	default DRM_RCAR_DU
> +	help
> +	  Enable support for the R-Car Display Unit embedded DSC encoder.
> +
> +config DRM_RCAR_DSC
> +	def_tristate DRM_RCAR_DU

Because DRM_RCAR_DSC is a hidden config with def_tristate DRM_RCAR_DU, will
it always evaluate to 'n' if DRM_RCAR_DU is disabled, even when COMPILE_TEST
is enabled through DRM_RCAR_USE_DSC?

This seems like it would prevent the driver from actually being compiled under
COMPILE_TEST when the main DU driver is not selected.

> +	depends on DRM_RCAR_USE_DSC
> +	select RESET_CONTROLLER

[ ... ]

> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_dsc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_dsc.c
> new file mode 100644
> index 0000000000000..2ab5a4bb099c3
> --- /dev/null
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_dsc.c

[ ... ]

> +static void rcar_dsc_atomic_enable(struct drm_bridge *bridge,
> +				   struct drm_atomic_state *state)
> +{
> +	struct rcar_dsc *dsc = bridge_to_rcar_dsc(bridge);
> +
> +	WARN_ON(clk_prepare_enable(dsc->clk));
> +	WARN_ON(reset_control_deassert(dsc->rst));
> +}
> +
> +static void rcar_dsc_atomic_disable(struct drm_bridge *bridge,
> +				    struct drm_atomic_state *state)
> +{
> +	struct rcar_dsc *dsc = bridge_to_rcar_dsc(bridge);
> +
> +	reset_control_assert(dsc->rst);
> +	clk_disable_unprepare(dsc->clk);

If clk_prepare_enable() fails in rcar_dsc_atomic_enable(), it returns an error
code but execution proceeds since the function returns void.

Will the unconditional call to clk_disable_unprepare() here lead to
unbalanced clock operations and trigger underflow warnings in the clock
framework?

[ ... ]

> +	dsc->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(dsc->clk))
> +		return dev_err_probe(dev, PTR_ERR(dsc->clk),
> +				     "Failed to get CPG clock\n");
> +
> +	dsc->rst = devm_reset_control_get(dev, NULL);

This isn't a bug, but devm_reset_control_get() is documented as a deprecated
transition wrapper in <linux/reset.h>. Should this use
devm_reset_control_get_exclusive() instead?

> +	if (IS_ERR(dsc->rst))
> +		return dev_err_probe(dev, PTR_ERR(dsc->rst),
> +				     "Failed to get CPG reset\n");
> +
> +	remote = of_graph_get_remote_node(dev->of_node, 1, 0);
> +	if (!remote)
> +		return -EINVAL;
> +
> +	dsc->bridge.next_bridge = of_drm_find_and_get_bridge(remote);
> +	of_node_put(remote);
> +	if (!dsc->bridge.next_bridge)
> +		return -EPROBE_DEFER;
> +
> +	dsc->bridge.of_node = dev->of_node;
> +
> +	return devm_drm_bridge_add(dev, &dsc->bridge);
> +}

of_drm_find_and_get_bridge() intentionally increments the module and device
reference counts for the returned bridge.

If devm_drm_bridge_add() fails, does this leak the reference to
next_bridge?

Also, since the driver lacks a .remove callback or a devm cleanup action, is
the reference permanently leaked when the driver is unbound?

Could this use the managed devm_drm_of_get_bridge() API to ensure proper
lifecycle management?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-rcar-du-dsc-v1-0-d65f7a9e9841@ideasonboard.com?part=3