Re: [PATCH v4 10/12] drm/mediatek: mtk_dp: Add support for HotPlug Detection in DP AUX

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unhandled interrupt storm due to unconditional unmasking of AUX HPD interrupts on older SoCs.
- [High] Unsafe read-modify-write on the interrupt clear register `MTK_DP_AUX_TX_P0_INT_CLR` causes lost interrupts.
- [Medium] Missing `legacy_regoff` addition when polling the AUX HPD status register `MTK_DP_AUX_P0_364C`.
--

--- Patch [10]: [PATCH v4 10/12] drm/mediatek: mtk_dp: Add support for HotPlug Detection in DP AUX ---
commit d87425adb9b3f30b1f231894a984a344e048ae99
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dp: Add support for HotPlug Detection in DP AUX

This patch adds support for hotplug detection via DP AUX in MediaTek DP IP,
avoiding the need to power up the entire eDP/DP IP. It introduces a new
flag to select the HPD strategy.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index ea970a9bfe936..772ba311a34a7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1047,7 +1048,30 @@ static u32 mtk_dp_swirq_get_clear(struct mtk_dp *mtk_dp)
>  	return irq_status;
>  }
>  
> -static u32 mtk_dp_hwirq_get_clear(struct mtk_dp *mtk_dp)
> +static u32 mtk_dp_aux_hwirq_get_clear(struct mtk_dp *mtk_dp)
> +{
> +	u32 irq_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_TX_P0_INT_STA);
> +	u32 ret = 0;
> +
> +	if (irq_status) {
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_TX_P0_INT_CLR,
> +				   irq_status, irq_status);

[Severity: High]
Can this read-modify-write operation drop newly arrived interrupts?

mtk_dp_update_bits() uses regmap_update_bits() which performs a
read-modify-write. By using irq_status as the mask, if the clear register
reads back with new interrupt bits set (that arrived after the INT_STA read),
the read-modify-write will preserve those new bits and write them back as 1s,
unintentionally clearing them before they are handled.

Should a full mask (like DP_TX_AUX_INT_MASK) be used here instead to ensure
non-target bits are zeroed out?

> +		mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_TX_P0_INT_CLR,
> +				   0, irq_status);
> +	}
> +

[ ... ]

> @@ -1062,8 +1086,28 @@ static u32 mtk_dp_hwirq_get_clear(struct mtk_dp *mtk_dp)
>  	return irq_status;
>  }
>  
> +static inline u32 mtk_dp_hwirq_get_clear(struct mtk_dp *mtk_dp)
> +{
> +	if (mtk_dp->data->aux_hpd_supported)
> +		return mtk_dp_aux_hwirq_get_clear(mtk_dp);
> +
> +	return mtk_dp_trans_hwirq_get_clear(mtk_dp);
> +}
> +
>  static void mtk_dp_hwirq_enable(struct mtk_dp *mtk_dp, bool enable)
>  {
> +	u32 mask, val;
> +
> +	/* On SoCs with working AUX HPD, the transmitter HPD register is ignored by HW */
> +	if (enable) {
> +		mask = HPD_CONNECT_EVENT | HPD_INTERRUPT_EVENT | HPD_DISCONNECT_EVENT;
> +		val = 0;
> +	} else {
> +		mask = DP_TX_AUX_INT_MASK;
> +		val = DP_TX_AUX_INT_MASK;
> +	}
> +	mtk_dp_update_bits(mtk_dp, MTK_DP_TX_AUX_INT_MASKING, val, mask);
> +

[Severity: High]
Does this code risk causing an unhandled interrupt storm on older SoCs?

Since MTK_DP_TX_AUX_INT_MASKING is updated unconditionally, the AUX HPD
interrupts are unmasked even if aux_hpd_supported is false.

If an older SoC triggers an AUX HPD interrupt, mtk_dp_hwirq_get_clear()
will branch to mtk_dp_trans_hwirq_get_clear() and ignore the AUX interrupt,
causing it to remain asserted and repeatedly trigger.

Should this unmasking be guarded by the aux_hpd_supported flag?

>  	mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3418,
>  			   enable ? 0 :
>  			   IRQ_MASK_DP_TRANS_P0_DISC_IRQ |

[ ... ]

> @@ -2113,10 +2191,15 @@ static int mtk_dp_wait_hpd_asserted(struct drm_dp_aux *mtk_aux, unsigned long wa
>  	u32 val;
>  	int ret;
>  
> -	ret = regmap_read_poll_timeout(mtk_dp->regs,
> -				       MTK_DP_TRANS_P0_3414 + mtk_dp->legacy_regoff,
> -				       val, !!(val & HPD_DB_DP_TRANS_P0_MASK),
> -				       wait_us / 100, wait_us);
> +	if (mtk_dp->data->aux_hpd_supported)
> +		ret = regmap_read_poll_timeout(mtk_dp->regs, MTK_DP_AUX_P0_364C,
> +					       val, !!(val & HPD_STATUS_DP_AUX_TX_P0_MASK),
> +					       wait_us / 100, wait_us);

[Severity: Medium]
Is the register address missing the legacy_regoff addition here?

Unlike mtk_dp_read() which automatically adds mtk_dp->legacy_regoff,
regmap_read_poll_timeout() operates directly on the regmap.

If a platform with aux_hpd_supported also uses a non-zero legacy_regoff, this
will poll the wrong physical register address. The fallback path for older
SoCs correctly applies the offset manually.

> +	else
> +		ret = regmap_read_poll_timeout(mtk_dp->regs,
> +					       MTK_DP_TRANS_P0_3414 + mtk_dp->legacy_regoff,
> +					       val, !!(val & HPD_DB_DP_TRANS_P0_MASK),
> +					       wait_us / 100, wait_us);

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