Re: [PATCH v4 7/9] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/atomic: sleeping functions called from atomic context in bridge callbacks
- [High] core: static resets array in probe causes devres double-free and data races
- [Medium] pm: PM_RUNTIME_ACQUIRE_IF_ENABLED guard prevents synchronous suspend
- [Medium] pm: refcount underflow in atomic_disable due to early return in atomic_enable
- [Low] build: DRM_RZG3L_LVDS cannot be built under COMPILE_TEST without base DU driver
- [Low] clk: unused dotclk resource allocation wastes memory and references
--

commit 40f44004822fcb3da6bccf8a2c28906d5abaa57d
Author: Biju Das <biju.das.jz@bp.renesas.com>

drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder

Add support for the RZ/G3L LVDS encoder driver. It operates in single-link
mode with 4 lanes (Data) + 1 lane (Clock) and supports pixel clock rates
from 25 to 87 MHz. The LVDS module cannot be used at the same time as
MIPI-DSI.

> diff --git a/drivers/gpu/drm/renesas/rz-du/Kconfig b/drivers/gpu/drm/renesas/rz-du/Kconfig
> index 7f2ef7137ae53..0dbe86bd65d09 100644
> --- a/drivers/gpu/drm/renesas/rz-du/Kconfig
> +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig
> @@ -26,3 +26,16 @@ config DRM_RZG2L_MIPI_DSI
>  	def_tristate DRM_RZG2L_DU
>  	depends on DRM_RZG2L_USE_MIPI_DSI
>  	select DRM_MIPI_DSI
> +
> +config DRM_RZG3L_USE_LVDS
> +	bool "RZ/G3L DU LVDS Encoder Support"
> +	depends on DRM_BRIDGE && OF
> +	depends on DRM_RZG2L_DU || COMPILE_TEST
> +	default DRM_RZG2L_DU
> +	help
> +	  Enable support for the RZ/G3L Display Unit embedded LVDS encoder.
> +
> +config DRM_RZG3L_LVDS
> +	def_tristate DRM_RZG2L_DU
> +	depends on DRM_RZG3L_USE_LVDS
> +	select DRM_KMS_HELPER

[Severity: Low]
Is it intended that this driver can never be built in a pure COMPILE_TEST
configuration when the base driver is disabled?

Without a prompt, def_tristate DRM_RZG2L_DU unconditionally forces the
module to 'n' if DRM_RZG2L_DU is 'n', ignoring the COMPILE_TEST intent.

> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c
> new file mode 100644
> index 0000000000000..1658be8da545f
> --- /dev/null
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c

[ ... ]

> +static void rzg3l_lvds_atomic_enable(struct drm_bridge *bridge,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge);
> +	const struct drm_bridge_state *bridge_state;
> +	u32 fmt;
> +
> +	/* Get the LVDS format from the bridge state. */
> +	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> +	if (WARN_ON(!bridge_state))
> +		return;

[Severity: Medium]
Could this early return cause a PM runtime reference count underflow?

Since rzg3l_lvds_atomic_disable() unconditionally calls
pm_runtime_put_sync(), skipping the pm_runtime_get_sync() here due to an
early return means the subsequent disable will drop a reference that was
never acquired.

> +
> +	switch (bridge_state->output_bus_cfg.format) {
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> +		fmt = RZG3L_LVDS_MODE_JEIDA;
> +		break;
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> +		fmt = RZG3L_LVDS_MODE_VESA;
> +		break;
> +	default:
> +		fmt = RZG3L_LVDS_MODE_VESA;
> +		dev_warn(lvds->dev, "Unsupported bus fmt 0x%04x\n",
> +			 bridge_state->output_bus_cfg.format);
> +		break;
> +	}
> +
> +	if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0))
> +		return;
> +
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_EN_BGR, LVDS_0_PHY_CH_EN_BGR);
> +	fsleep(20);

[Severity: High]
Is it safe to call sleeping functions like pm_runtime_get_sync() and
fsleep() here?

Bridge atomic_enable callbacks run in atomic context during non-blocking
commits, so these sleeping calls might cause kernel warnings or deadlocks.

[ ... ]

> +static void rzg3l_lvds_atomic_disable(struct drm_bridge *bridge,
> +				      struct drm_atomic_commit *state)
> +{
> +	struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge);
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(lvds->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret < 0)
> +		goto pm_put_sync;
> +
> +	regmap_update_bits(lvds->regmap, LVDS_CMN, LVDS_CMN_PHY_RESET, 0);
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_IO_EN0_MSK, 0);
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_EN_LDO, 0);
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_EN_BGR, 0);
> +
> +pm_put_sync:
> +	pm_runtime_put_sync(lvds->dev);
> +}

[Severity: High]
Similar to atomic_enable(), is it safe to call the sleeping function
pm_runtime_put_sync() from the atomic context of atomic_disable()?

[Severity: Medium]
Additionally, does the PM_RUNTIME_ACQUIRE_IF_ENABLED guard prevent
pm_runtime_put_sync() from synchronously suspending the device?

The scoped guard holds a PM reference until the function returns. This
means the usage counter remains > 0 when pm_runtime_put_sync() executes,
causing the device to be suspended asynchronously instead.

[ ... ]

> +static int rzg3l_lvds_probe(struct platform_device *pdev)
> +{
> +	static struct reset_control_bulk_data resets[2];

[Severity: High]
Is there a reason this resets array is declared static?

Because devm_reset_control_bulk_get_exclusive() stores the exact pointer to
this array in the device's devres allocations list, making it globally
shared across all device instances could lead to a double-free on device
unbind, or data races during probe.

> +	struct device *dev = &pdev->dev;
> +	struct rzg3l_lvds *lvds;
> +	void __iomem *base;
> +	int ret;
> +
> +	lvds = devm_drm_bridge_alloc(dev, struct rzg3l_lvds, bridge,
> +				     &rzg3l_lvds_bridge_ops);
> +	if (IS_ERR(lvds))
> +		return PTR_ERR(lvds);
> +
> +	lvds->dev = dev;
> +	lvds->bridge.of_node = pdev->dev.of_node;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	lvds->regmap = devm_regmap_init_mmio(dev, base, &rzg3l_lvds_regmap_config);
> +	if (IS_ERR(lvds->regmap))
> +		return dev_err_probe(dev, PTR_ERR(lvds->regmap),
> +				     "Failed to init regmap\n");
> +
> +	lvds->dotclk = devm_clk_get(dev, "dotclk");
> +	if (IS_ERR(lvds->dotclk))
> +		return PTR_ERR(lvds->dotclk);

[Severity: Low]
Does the driver need to allocate this dotclk?

It is fetched here but never referenced again, which might unnecessarily
consume memory and a clock reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930133257.23210-1-biju.das.jz@bp.renesas.com?part=7