Re: [PATCH v2 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:
- [High] The patch introduces a forbidden sleeping function (`msleep`) inside atomic commit paths.
- [High] `mtk_dp_bridge_atomic_disable()` incorrectly forces the encoder bias power ON rather than OFF, likely due to a copy-paste error.
- [High] Conflicting register programming in `mtk_dp_encoder_reset` overwrites PHY enable bits and potentially leaves the PHY in permanent reset.
- [High] Double unregister of the PHY platform device (`mtk_dp->phy_dev`) on the error path.
- [High] `mtk_dp_setup_encoder` constructs an update mask using raw bit values instead of proper field masks, failing to clear bits in multi-bit fields.
--

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

This patch adds support for MediaTek's Embedded DisplayPort (eDP) v1.5 IP
found in the MT8196 SoC and other variants. It introduces conditional
hardware initialization sequences, platform data switches for newer features,
and includes partial support paths for the DisplayPort 1.4 IP 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
[ ... ]
> @@ -648,7 +669,18 @@ static void mtk_dp_setup_encoder(struct mtk_dp *mtk_dp)
>  	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3364,
>  			   FIFO_READ_START_POINT_DP_ENC1_P0_VAL << 12,
>  			   FIFO_READ_START_POINT_DP_ENC1_P0_MASK);
> -	mtk_dp_write(mtk_dp, MTK_DP_ENC1_P0_3368, DP_ENC1_P0_3368_VAL);
> +
> +	mask = VIDEO_SRAM_FIFO_CNT_RESET_SEL_DP_ENC1_P0 | VIDEO_STABLE_CNT_THRD_DP_ENC1_P0;
> +	mask |= BS2BS_MODE_DP_ENC1_P0;
> +
> +	val = mask;
> +
> +	mask |= SDP_DP13_EN_DP_ENC1_P0;
> +
> +	if (mtk_dp->data->sdp_encoder1_enable)
> +		val |= SDP_DP13_EN_DP_ENC1_P0;
> +
> +	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3368, val, mask);

[Severity: High]
Does this code construct the update mask using single-bit values rather than
multi-bit field masks?

The previous code wrote directly to the register, but the update here seems
to use bit values (like BS2BS_MODE_DP_ENC1_P0) as the mask itself. 
Since BS2BS_MODE_DP_ENC1_P0 is just one bit but the field might be larger,
will this leave uncleared bits in the multi-bit fields like
VIDEO_SRAM_FIFO_CNT_RESET_SEL_MASK and BS2BS_MODE_DP_ENC1_P0_MASK?

[ ... ]
> @@ -1116,6 +1148,72 @@ static void mtk_dp_hwirq_enable(struct mtk_dp *mtk_dp, bool enable)
>  			   IRQ_MASK_DP_TRANS_P0_MASK);
>  }
>  
> +static void mtk_dp_encoder_force_bias_power_on(struct mtk_dp *mtk_dp)
> +{
> +	/* Force MCU to enable BIAS */
> +	mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3,
> +			   ENC_PHY_PWR_OVERWRITE_EN, ENC_PHY_PWR_OVERWRITE_EN);
> +	mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3,
> +			   FIELD_PREP_CONST(ENC_PHY_PWR_STATE_OVERWRITE_MASK,
> +					    ENC_PHY_BIAS_PWR_ON),
> +			   ENC_PHY_PWR_STATE_OVERWRITE_MASK);
> +
> +	/* Wait for the MCU to get LDO ready in PHYD */
> +	msleep(50);

[Severity: High]
Can this sleep in an atomic context?

This function is called by mtk_dp_set_lanes() during display enabling, and
by mtk_dp_bridge_atomic_disable(), both of which are executed in atomic
commit paths where sleeping functions are forbidden. Could this msleep(50)
cause kernel warnings or deadlocks during non-blocking commits?

> +
> +	mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3,
> +			   0, ENC_PHY_PWR_OVERWRITE_EN);
> +}
[ ... ]
> +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);

[Severity: High]
Does this write to ENC_PHY_STATE_RESET_ALL_MASK permanently hold the PHY in
reset and overwrite unrelated fields?

Looking at the definition of ENC_PHY_STATE_RESET_ALL_MASK, it is
GENMASK(7, 0). Writing 1s to this entire mask overwrites the enable bits
DP_ENCODER_DVO_ON_W_1 and PHY_STATE_W_1_DP_ENC_P0_3 (bits 5 and 6) that were
just configured. Since the reset is never cleared, will the PHY remain stuck
in this reset state?

> +	}
[ ... ]
> @@ -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 mtk_dp_encoder_force_bias_power_on() the correct function to call during
the disable path?

This turns the encoder bias power ON during bridge disable. Given that
mtk_dp_power_disable() correctly calls mtk_dp_encoder_force_bias_power_off(),
should this be calling the power_off function instead to properly power down
the display path?

>  	/* 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)) {

[Severity: High]
Could the error path for devm_phy_get cause a double free of the PHY device?

The patch registers a devm action earlier in this function:

    ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs,
                                   mtk_dp->phy_dev);

However, the existing error path for devm_phy_get manually unregisters the
device:

    if (IS_ERR(mtk_dp->phy)) {
        platform_device_unregister(mtk_dp->phy_dev);
        return dev_err_probe(...);
    }

Since returning an error will trigger the devres cleanup (and run
mtk_dp_unregister_pdevs), will this result in platform_device_unregister()
being called a second time on the same pointer?

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