Re: [PATCH v3 10/11] drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC

[sashiko-bot@kernel.org]

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

New issues:
- [High] PHY bias power is incorrectly forced ON instead of OFF during the bridge atomic disable sequence.
- [High] Zero-width reset pulse and failure to clear PHY reset state for the eDP 1.5 IP.
- [Medium] Uninitialized calibration data is unconditionally flushed to PHY registers when MT8196 uses the legacy PHY configuration.

Pre-existing issues:
- [High] Double free / use-after-free of the PHY platform device on probe error path.
- [High] Use-after-free in debounce timer teardown due to failure to disable threaded IRQ.
--

--- Patch [10]: [PATCH v3 10/11] drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC ---
commit f310ca07bbff89c8e8b27b5c8d3a9c8cfc72e839
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC

This commit adds support for MediaTek's Embedded DisplayPort (eDP) v1.5 IP, 
found in the MT8196 SoC. It introduces conditional register writes based on 
platform data booleans to handle SDP encoders, PSR support, and power state 
forcing differences between IP revisions. It also partially prepares for DP 
1.4 IP support on the same SoC.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index fb216811c80c7..868d918a0b161 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[ ... ]
> @@ -1210,24 +1315,87 @@ static void mtk_dp_initialize_aux_settings(struct mtk_dp *mtk_dp)
[ ... ]
> +static void mtk_dp_encoder_reset(struct mtk_dp *mtk_dp)
> +{
>  	/* dp tx encoder reset all sw */
>  	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
>  			   DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0,
>  			   DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
>  
> +	if (mtk_dp->data->hwip_v1p4) {
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, 0,
> +				   DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
> +		mtk_dp_update_bits(mtk_dp, REG_3FF8_DP_ENC_P0_3,
> +				   ENC_PHY_STATE_RESET_ALL_MASK,
> +				   ENC_PHY_STATE_RESET_ALL_MASK);
> +	}
> +
>  	/* Wait for sw reset to complete */
>  	usleep_range(1000, 5000);
> -	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
> -			   0, DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
> +
> +	if (!mtk_dp->data->hwip_v1p4)
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
> +				   0, DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
> +}

[Severity: High]
For the eDP 1.5 IP (hwip_v1p4), does asserting
DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0 to 1 and immediately deasserting it to 0
create a zero-width pulse that the hardware might ignore?

Furthermore, ENC_PHY_STATE_RESET_ALL_MASK is asserted but never explicitly
cleared after the delay. Will this leave the PHY permanently in reset unless
the register is self-clearing?

[ ... ]
> @@ -1359,8 +1550,10 @@ static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp,
>  		}
>  	};
>  
> -	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, DP_PWR_STATE_BANDGAP,
> -			   DP_PWR_STATE_MASK);
> +	/* Set power state requests only if power states are not forced */
> +	if (!mtk_dp->data->force_pwr_states)
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, DP_PWR_STATE_BANDGAP,
> +				   DP_PWR_STATE_MASK);
>  
>  	ret = phy_configure(mtk_dp->phy, &phy_opts);
>  	if (ret)
> @@ -1373,6 +1566,11 @@ static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp,
>  	if (mtk_dp->phy_dev)
>  		mtk_dp_set_calibration_data(mtk_dp);

[Severity: Medium]
If MT8196 is used with a device tree lacking the separate phys node, it will
fall back to the legacy architecture and create mtk_dp->phy_dev.

Since mt8196_edp_data does not provide an efuse_fmt,
mtk_dp_get_calibration_data() is skipped, leaving the cal_data array zeroed.

Later in mtk_dp_phy_configure(), the code unconditionally calls
mtk_dp_set_calibration_data() if mtk_dp->phy_dev exists.

Will this flush uninitialized or zeroed cal_data directly into the PHY
registers and misconfigure the hardware?

[ ... ]
> @@ -2580,6 +2795,9 @@ static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  	/* SDP path reset sw*/
>  	mtk_dp_sdp_path_reset(mtk_dp);
>  
> +	if (mtk_dp->data->force_pwr_states)
> +		mtk_dp_encoder_force_bias_power_on(mtk_dp);
> +

[Severity: High]
Is it intentional to force the PHY bias power ON during the bridge disable
sequence?

The disable path usually turns hardware off to save power (as done in
mtk_dp_power_disable() which calls mtk_dp_encoder_force_bias_power_off()).

Does this cause a power leak or keep the PHY in an incorrect state during
teardown?

>  	/* Ensure the sink is muted */
>  	msleep(20);
>  }
> @@ -2860,7 +3078,8 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  				     "Failed to add phy unregister devm action");
>  
>  	/* PHY calibration data is in mtk_dp only for legacy devicetree */
> -	mtk_dp_get_calibration_data(mtk_dp);
> +	if (mtk_dp->data->efuse_fmt)
> +		mtk_dp_get_calibration_data(mtk_dp);
>  
>  	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");
>  	if (IS_ERR(mtk_dp->phy)) {
>  		platform_device_unregister(mtk_dp->phy_dev);
>  		return dev_err_probe(dev, PTR_ERR(mtk_dp->phy), "Failed to get phy\n");
>  	}

[Severity: High]
This is a pre-existing issue, but if devm_phy_get() fails here, the explicit
error handling path calls platform_device_unregister(mtk_dp->phy_dev) before
returning.

Since mtk_dp_unregister_pdevs was registered as a devm action just prior to
this, returning an error will trigger the devres core to unwind the stack and
call platform_device_unregister() a second time on the same device pointer.

Can this cause a double-free or use-after-free crash?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing issue in
mtk_dp_remove() regarding the debounce timer teardown.

The HPD interrupt is registered during probe using devm_request_threaded_irq(),
meaning its lifetime spans until after mtk_dp_remove() returns.

Inside mtk_dp_remove(), the code calls timer_delete_sync(&mtk_dp->debounce_timer)
but does not disable the IRQ:

static void mtk_dp_remove(struct platform_device *pdev)
{
    ...
    if (mtk_dp->data->bridge_type != DRM_MODE_CONNECTOR_eDP)
        timer_delete_sync(&mtk_dp->debounce_timer);
    ...
}

Because the IRQ is still active, an incoming hardware interrupt can immediately
fire the mtk_dp_hpd_event_thread() handler, which can re-arm the timer using
mod_timer(). Once the mtk_dp object is subsequently freed by devres, the timer
expires and accesses freed memory.

Could this lead to a use-after-free on driver unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709103431.46616-1-angelogioacchino.delregno@collabora.com?part=10