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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/9/26 14:00, sashiko-bot@kernel.org wrote:
> 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.

Older SoCs can't trigger this path at all, as these have *non* functional AUX HPD
hence the AUX specific hpd interrupt can never be triggered.

> - [High] Unsafe read-modify-write on the interrupt clear register `MTK_DP_AUX_TX_P0_INT_CLR` causes lost interrupts.

No. That's the exact reason why MediaTek has a SETCLR strategy for many registers.
Interrupts can't be lost this way, and whatever I wrote there is indeed safe.

> - [Medium] Missing `legacy_regoff` addition when polling the AUX HPD status register `MTK_DP_AUX_P0_364C`.

It's missing on purpose. Old SoCs can't reach that code path.

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