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

[AngeloGioacchino Del Regno]

On 7/9/26 14:00, sashiko-...@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.delre...@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);