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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Sleeping functions called in atomic context during bridge enable
- [High] Early returns in `atomic_enable` cause unbalanced PM counter and unpowered hardware access in `atomic_disable`
--

commit 0eae00dcaa6988008a1dca84233d19bc97469ed0
Author: Biju Das <biju.das.jz@bp.renesas.com>

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

Adds support for the RZ/G3L LVDS encoder driver operating in single-link
mode with 4 lanes. It supports pixel clocks from 25 to 87 MHz and shares
peripherals with MIPI-DSI.

> 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..6100888ea7284
> --- /dev/null
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c
[ ... ]
> @@ -0,0 +1,285 @@
> +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: High]
If drm_atomic_get_new_bridge_state() returns NULL and takes this early return,
will rzg3l_lvds_atomic_disable() later execute unconditionally and access
unpowered hardware registers or cause a runtime PM usage counter underflow?
Since atomic_enable returns void, the DRM core might still consider the bridge
enabled.

> +
> +	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;

[Severity: High]
Similarly, if pm_runtime_get_sync() fails, returning early here leaves the
device unpowered. When rzg3l_lvds_atomic_disable() is called during teardown,
could this lead to a bus fault from accessing unpowered MMIO registers and
an unbalanced pm_runtime_put() counter?

[Severity: High]
Also, since atomic_enable callbacks run in an atomic commit context where
sleeping is forbidden, is it safe to use pm_runtime_get_sync() here? This
function blocks and sleeps while waiting for the device to resume, which might
cause a scheduling while atomic regression.

> +
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_EN_BGR, LVDS_0_PHY_CH_EN_BGR);
> +	fsleep(20);
> +
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_EN_LDO, LVDS_0_PHY_CH_EN_LDO);
> +	fsleep(10);

[Severity: High]
Because this is an atomic commit path, delay functions that can sleep like
fsleep() are unsafe. Could these fsleep() calls (which map to usleep_range()
or msleep()) lead to scheduling while atomic bugs? Would udelay() or ndelay()
be more appropriate for these hardware delays?

> +
> +	regmap_write(lvds->regmap, LVDS_CMN, LVDS_CMN_RST_PHY0_SEL);
> +	regmap_update_bits(lvds->regmap, LVDS_0_CTL_OFFSET,
> +			   LVDS_0_CTL_FMT_SEL0_MSK,
> +			   FIELD_PREP(LVDS_0_CTL_FMT_SEL0_MSK, fmt));
> +	regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET,
> +			   LVDS_0_PHY_CH_IO_EN0_MSK, LVDS_0_PHY_CH_IO_EN0);
> +	regmap_write(lvds->regmap, LVDS_CMN,
> +		     LVDS_CMN_RST_PHY0_SEL | LVDS_CMN_PHY_RESET);
> +	fsleep(100);

[Severity: High]
Could this fsleep(100) call also sleep in this atomic context?

> +}

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