Re: [PATCH v3 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Unchecked return value of clk_prepare_enable() in runtime resume.
- [High] Unchecked return value of pm_runtime_get_sync() during atomic enable.
- [High] Interrupt handler accesses hardware registers while device may be suspended.

Pre-existing issues:
- [High] Memory/refcount leak of the next_bridge acquired via of_drm_get_bridge_by_endpoint().
- [High] PHY power_count underflow causing permanent PHY failure after early return in atomic_enable.
- [High] Uninitialized stack memory copied to buffer in dw_dp_aux_read_data().
--

commit 25b9778935f0029deadc6a4ac4d19728b07017ee
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Subject: drm/bridge: synopsys: dw-dp: Add Runtime PM support

The patch adds runtime PM stubs to the Synopsys DesignWare DisplayPort bridge
driver and hooks them up to various hardware access paths.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 7fa38145e35ce..7f4f36c614844 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1465,6 +1465,8 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
>  	if (WARN_ON(msg->size > 16))
>  		return -E2BIG;
>  
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +

[Severity: High]
This is a pre-existing issue, but I noticed a potential information leak in
dw_dp_aux_read_data() which is called during these transfers:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_read_data() {
	...
	u32 value;

	regmap_read(dp->regmap, DW_DP_AUX_DATA0 + i * 4, &value);

	for (j = 0; j < num; j++)
		buffer[i * 4 + j] = value >> (j * 8);
	...
}

If regmap_read() fails, the local variable "value" remains uninitialized.
Could this copy uninitialized kernel stack memory into the buffer, which is
then returned to userspace via the DRM DP AUX chardev?

>  	switch (msg->request & ~DP_AUX_I2C_MOT) {
>  	case DP_AUX_NATIVE_WRITE:
>  	case DP_AUX_I2C_WRITE:

[ ... ]

> @@ -1655,6 +1657,8 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector_state *conn_state;
>  	int ret;
>  
> +	pm_runtime_get_sync(dp->dev);
> +

[Severity: High]
Does this code need to check the return value of pm_runtime_get_sync()?

If the device fails to resume (for example, if the clocks fail to enable),
the driver will continue with hardware initialization routines like
dw_dp_link_enable().  Can this lead to a crash by accessing a suspended
device?

>  	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  	if (!connector) {
>  		dev_err(dp->dev, "failed to get connector\n");
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does returning early here cause a
phy->power_count underflow later?

If we return early because connector is NULL, or if dw_dp_link_enable() fails,
we skip powering on the PHY.  However, because atomic_enable returns void,
the DRM framework will still consider the bridge enabled and later call
dw_dp_bridge_atomic_disable(), which unconditionally executes:

	phy_power_off(dp->phy);

Could this drop the PHY's power_count below zero and permanently prevent it
from powering on during subsequent modesets?

[ ... ]

> @@ -2153,6 +2160,26 @@ void dw_dp_unbind(struct dw_dp *dp)
>  }
>  EXPORT_SYMBOL_GPL(dw_dp_unbind);
>  
> +int dw_dp_runtime_suspend(struct dw_dp *dp)
> +{
> +	clk_disable_unprepare(dp->aux_clk);
> +	clk_disable_unprepare(dp->apb_clk);
> +
> +	return 0;
> +}

[Severity: High]
Is it safe to leave the IRQ enabled while the device is suspended?

If a spurious interrupt or a hot-plug event fires while the device is in
the RPM_SUSPENDED state, dw_dp_irq() will execute and unconditionally call:

	regmap_read(dp->regmap, DW_DP_GENERAL_INTERRUPT, &value);

Since the APB clock is disabled here, might this MMIO read hang the bus
and cause a kernel panic?

> +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend);
> +
> +int dw_dp_runtime_resume(struct dw_dp *dp)
> +{
> +	clk_prepare_enable(dp->apb_clk);
> +	clk_prepare_enable(dp->aux_clk);
> +

[Severity: High]
Should the return values of clk_prepare_enable() be checked?

If the clocks fail to enable, the function proceeds to call dw_dp_init_hw()
which accesses hardware registers via regmap.  Could this lead to a bus fault?

> +	dw_dp_init_hw(dp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_runtime_resume);


There is also one additional pre-existing issue in a part of the file not
modified by this patch:

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak in dw_dp_bind():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind() {
	...
	next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0);
	if (IS_ERR(next_bridge)) {
	...
	ret = drm_bridge_attach(encoder, next_bridge, bridge,
				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
	...
}

Does this code leak the reference count for next_bridge?

of_drm_get_bridge_by_endpoint() returns a bridge with an incremented refcount,
and drm_bridge_attach() takes its own reference.  Should there be a call to
drm_bridge_put() to drop the initial reference so the next bridge module can
be unloaded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-synopsys-dw-dp-improvements-v3-0-dc61e6352508@collabora.com?part=7