Re: [PATCH v3 3/7] 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 3 potential issue(s) to consider:
- [High] Unbalanced PM runtime usage counter due to unconditionally calling pm_runtime_put() after pm_runtime_resume_and_get() failure.
- [High] Incorrect devm teardown ordering between DRM bridge registration and PM runtime.
- [High] The drm_bridge reference obtained via of_drm_find_and_get_bridge() is never released.
--

commit 02d8011f56016654fa33cd3453d89ee92304eef9
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>

drm/rcar-du: dsc: Add rudimentary Renesas R-Car V4H DSC driver

The Renesas DSC Display Stream Compression is a bridge embedded in the
Renesas R-Car V4H SoC. The bridge performs VESA DSC encoding of up to
8k or 400 Mpixel/s . Add rudimentary driver, which currently acts as a
pass-through bridge and allows DSI1 to be operational on R-Car V4H.

> 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..5d3d083e3ce65
> --- /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(pm_runtime_resume_and_get(dsc->dev));
> +}

If pm_runtime_resume_and_get() fails, it does not increment the usage counter.
Since execution continues even if the call fails:

> +static void rcar_dsc_atomic_disable(struct drm_bridge *bridge,
> +				    struct drm_atomic_state *state)
> +{
> +	struct rcar_dsc *dsc = bridge_to_rcar_dsc(bridge);
> +
> +	pm_runtime_put(dsc->dev);
> +}

Could the unconditional pm_runtime_put() here cause a usage counter underflow
if the initial enable failed?

[ ... ]

> +static int rcar_dsc_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	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;
> +
> +	ret = devm_drm_bridge_add(dev, &dsc->bridge);
> +	if (ret)
> +		return ret;

The function of_drm_find_and_get_bridge() acquires a reference to the bridge.
Is a corresponding drm_bridge_put() needed if devm_drm_bridge_add() fails,
and also when the device is removed?

> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static void rcar_dsc_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +}

Since devm_drm_bridge_add() registers the bridge via devres, the bridge
remains registered and accessible after rcar_dsc_remove() returns.

Does manually calling pm_runtime_disable() here create a window where the
bridge might be accessed while its PM runtime is disabled?

Would using devm_pm_runtime_enable() prior to registering the bridge ensure
proper reverse-order teardown?

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