[PATCH for drm-misc-fixes v4 0/2] Fix some bugs in the hibmc DP

[PATCH for drm-misc-fixes v4 0/2] Fix some bugs in the hibmc DP

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

Fix some bugs in the hibmc DP driver.

---
ChangeLog:
v3 -> v4:
  - Fix the data race on hpd_status between hardirq and process
    contexts with WRITE_ONCE()/READ_ONCE(). (sashiko-bot)
  - Delete the HPD_IN early return in encoder_enable that breaks KMS
    forced modesets. (sashiko-bot)
v2 -> v3:
  - Delete irq_status and use hpd_status directly for hotplug
    detection. (sashiko-bot)
  - Add debug logs when hpd_status is not HPD_IN in detect and
    encoder_enable.
  - Move hibmc_set_enable_flag() before drm_dev_register() and
    hibmc_set_disable_flag() after drm_dev_unregister() to narrow the
    race window. (sashiko-bot)
  - Add spinlock_t gpio_lock to hibmc_drm_private to protect all
    operations on 0x0802A4. (sashiko-bot)
v1 -> v2:
  - More states in HIBMC_DP_HPD_STATUS are added to the
    'hibmc_dp_get_hpd_status'.
  - The call to `hibmc_set_enable_flag()` has been moved before
    `drm_client_setup()`, specifically before the connector detection
    triggers the I2C bit operation. (sashiko-bot)
  - A read-modify-write operation has been implemented instead of directly
    writing to prevent clearing the I2C mask bits. (sashiko-bot)
---

Lin He (2):
  drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status
  drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver
    has been loaded

 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 -
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 32 +++++++++++-------
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  5 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 29 ++++++++--------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 33 ++++++++++++++++++-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  2 ++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   | 19 +++++++++--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h  |  3 ++
 8 files changed, 92 insertions(+), 32 deletions(-)

-- 
2.33.0

[PATCH for drm-misc-fixes v4 1/2] drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

To more accurately determine whether the current HPD status matches the
interrupt status, the polling mechanism in the lower half of the HPD
interrupt (via hibmc_dp_check_hpd_status) has been replaced with directly
retrieving the interrupt status in the upper half of the interrupt (via
hibmc_dp_get_hpd_status).

* Detection and training are not performed if hpd_status is not HPD_IN.

* Set the initial status of hpd_status to HPD_OUT.

Fixes: 3906e7a3b26d ("drm/hisilicon/hibmc: fix dp probabilistical detect errors after HPD irq")
Signed-off-by: Lin He <helin52@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v3 -> v4:
  - Fix the data race on hpd_status between hardirq and process
    contexts with WRITE_ONCE()/READ_ONCE(). (sashiko-bot)
  - Delete the HPD_IN early return in encoder_enable that breaks KMS
    forced modesets. (sashiko-bot)
v2 -> v3:
  - Delete irq_status and use hpd_status directly for hotplug
    detection. (sashiko-bot)
  - Add debug logs when hpd_status is not HPD_IN in detect and
    encoder_enable.
v1 -> v2:
  - More states in HIBMC_DP_HPD_STATUS are added to the
    'hibmc_dp_get_hpd_status'.
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 -
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 32 ++++++++++++-------
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  5 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 29 +++++++++--------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  3 +-
 5 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index f53dac256ee0..b0e258b9265e 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -43,7 +43,6 @@ struct hibmc_dp_dev {
 	u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 	struct drm_dp_desc desc;
 	bool is_branch;
-	int hpd_status;
 	void __iomem *serdes_base;
 };
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index d5bd3c45649b..a4cd4cd8cc75 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -191,6 +191,10 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
 	/* clock enable */
 	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
+	/* To latch the HPD interrupt, ensuring that DP can support more modes
+	 * within the fbcon framework when connected alone.
+	 */
+	msleep(100);
 
 	return 0;
 }
@@ -322,20 +326,26 @@ void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
 	writel(HIBMC_DP_SYNC_EN_MASK, dp_dev->base + HIBMC_DP_TIMING_SYNC_CTRL);
 }
 
-bool hibmc_dp_check_hpd_status(struct hibmc_dp *dp, int exp_status)
+int hibmc_dp_get_hpd_status(struct hibmc_dp *dp)
 {
+	int hpd_status = HIBMC_HPD_UNKNOWN;
 	u32 status;
-	int ret;
 
-	ret = readl_poll_timeout(dp->dp_dev->base + HIBMC_DP_HPD_STATUS, status,
-				 FIELD_GET(HIBMC_DP_HPD_CUR_STATE, status) == exp_status,
-				 1000, 100000); /* DP spec says 100ms */
-	if (ret) {
-		drm_dbg_dp(dp->drm_dev, "wait hpd status timeout");
-		return false;
+	status = FIELD_GET(HIBMC_DP_HPD_CUR_STATE,
+			   readl(dp->dp_dev->base + HIBMC_DP_HPD_STATUS));
+	switch (status) {
+	case 0: /* idle */
+	case 3: /* unplug */
+	case 4: /* unplug intermediate */
+		hpd_status = HIBMC_HPD_OUT;
+		break;
+	case 1: /* plug */
+	case 2: /* plug intermediate */
+		hpd_status = HIBMC_HPD_IN;
+		break;
+	default:
+		break;
 	}
 
-	dp->dp_dev->hpd_status = exp_status;
-
-	return true;
+	return hpd_status;
 }
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 0f3662d8737e..959bf95f6fe5 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -15,6 +15,7 @@
 struct hibmc_dp_dev;
 
 enum hibmc_hpd_status {
+	HIBMC_HPD_UNKNOWN,
 	HIBMC_HPD_OUT,
 	HIBMC_HPD_IN,
 };
@@ -54,7 +55,7 @@ struct hibmc_dp {
 	void __iomem *mmio;
 	struct drm_dp_aux aux;
 	struct hibmc_dp_cbar_cfg cfg;
-	u32 irq_status;
+	int hpd_status;
 	int phys_status;
 };
 
@@ -66,7 +67,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
 void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
 void hibmc_dp_enable_int(struct hibmc_dp *dp);
 void hibmc_dp_disable_int(struct hibmc_dp *dp);
-bool hibmc_dp_check_hpd_status(struct hibmc_dp *dp, int exp_status);
+int hibmc_dp_get_hpd_status(struct hibmc_dp *dp);
 u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
 u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 7bf6befa0440..65dd042de71d 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -14,8 +14,6 @@
 #include "dp/dp_comm.h"
 #include "dp/dp_config.h"
 
-#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
-
 static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
 {
 	const struct drm_edid *drm_edid;
@@ -60,13 +58,13 @@ static int hibmc_dp_detect(struct drm_connector *connector,
 {
 	struct hibmc_dp *dp = to_hibmc_dp(connector);
 	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
+	int hpd_status = READ_ONCE(dp->hpd_status);
 	int ret = connector_status_disconnected;
 
-	if (dp->irq_status) {
-		if (dp_dev->hpd_status != HIBMC_HPD_IN) {
-			ret = connector_status_disconnected;
-			goto exit;
-		}
+	if (hpd_status != HIBMC_HPD_IN) {
+		drm_dbg_dp(dp->drm_dev, "dp detect skipped, hpd (%d)\n",
+			   hpd_status);
+		goto exit;
 	}
 
 	if (!hibmc_dp_get_dpcd(dp_dev)) {
@@ -188,24 +186,26 @@ irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *)arg;
 	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
-	int idx, exp_status;
+	int status = READ_ONCE(priv->dp.hpd_status);
+	int idx;
 
 	if (!drm_dev_enter(dev, &idx))
 		return -ENODEV;
 
-	if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
+	if (status == HIBMC_HPD_IN) {
 		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
 		hibmc_dp_hpd_cfg(&priv->dp);
-		exp_status = HIBMC_HPD_IN;
-	} else {
+	} else if (status == HIBMC_HPD_OUT) {
 		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
 		hibmc_dp_reset_link(&priv->dp);
-		exp_status = HIBMC_HPD_OUT;
+	} else {
+		drm_err(&priv->dev, "HPD status (%d) error\n", status);
+		goto exit;
 	}
 
-	if (hibmc_dp_check_hpd_status(&priv->dp, exp_status))
-		drm_connector_helper_hpd_irq_event(&priv->dp.connector);
+	drm_connector_helper_hpd_irq_event(&priv->dp.connector);
 
+exit:
 	drm_dev_exit(idx);
 
 	return IRQ_HANDLED;
@@ -222,6 +222,7 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
 
 	dp->mmio = priv->mmio;
 	dp->drm_dev = dev;
+	dp->hpd_status = HIBMC_HPD_OUT;
 
 	ret = hibmc_dp_hw_init(&priv->dp);
 	if (ret) {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 602930964e93..df4501bb076b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -63,7 +63,8 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
 
 	status = readl(priv->mmio + HIBMC_DP_INTSTAT);
 	if (status) {
-		priv->dp.irq_status = status;
+		WRITE_ONCE(priv->dp.hpd_status,
+			   hibmc_dp_get_hpd_status(&priv->dp));
 		writel(status, priv->mmio + HIBMC_DP_INTCLR);
 		return IRQ_WAKE_THREAD;
 	}
-- 
2.33.0

[PATCH for drm-misc-fixes v4 2/2] drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

Add a flag to indicate whether the OS-side driver has been loaded to
prevent the BMC from enabling DP if the driver is not loaded, which could
lead to system failure in handling interrupts and generate error messages
like:

irq xx: nobody cared (try booting with the "irqpoll" option)
...
Call Trace:
<TRQ>
...

Fixes: 0ab6ea261c1f ("drm/hisilicon/hibmc: add dp module in hibmc")
Signed-off-by: Lin He <helin52@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - Move hibmc_set_enable_flag() before drm_dev_register() and
    hibmc_set_disable_flag() after drm_dev_unregister() to narrow the
    race window. (sashiko-bot)
  - Add spinlock_t gpio_lock to hibmc_drm_private to protect all
    operations on 0x0802A4. (sashiko-bot)
v1 -> v2:
  - The call to `hibmc_set_enable_flag()` has been moved before
    `drm_client_setup()`, specifically before the connector detection
    triggers the I2C bit operation. (sashiko-bot)
  - A read-modify-write operation has been implemented instead of directly
    writing to prevent clearing the I2C mask bits. (sashiko-bot)
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 30 +++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  2 ++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   | 19 ++++++++++--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h  |  3 ++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index df4501bb076b..15583a2d1c6f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -169,6 +169,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
 	if (ret)
 		return ret;
 
+	spin_lock_init(&priv->gpio_lock);
+
 	dev->mode_config.min_width = 0;
 	dev->mode_config.min_height = 0;
 	dev->mode_config.max_width = 1920;
@@ -425,6 +427,30 @@ static int hibmc_load(struct drm_device *dev)
 	return ret;
 }
 
+static inline void hibmc_set_enable_flag(struct hibmc_drm_private *priv)
+{
+	unsigned long flags;
+	u32 value;
+
+	spin_lock_irqsave(&priv->gpio_lock, flags);
+	value = readl(priv->mmio + HIBMC_ENABLE_FLAG);
+	value |= HIBMC_ENABLE_STATE;
+	writel(value, priv->mmio + HIBMC_ENABLE_FLAG);
+	spin_unlock_irqrestore(&priv->gpio_lock, flags);
+}
+
+static inline void hibmc_set_disable_flag(struct hibmc_drm_private *priv)
+{
+	unsigned long flags;
+	u32 value;
+
+	spin_lock_irqsave(&priv->gpio_lock, flags);
+	value = readl(priv->mmio + HIBMC_ENABLE_FLAG);
+	value &= ~HIBMC_ENABLE_STATE;
+	writel(value, priv->mmio + HIBMC_ENABLE_FLAG);
+	spin_unlock_irqrestore(&priv->gpio_lock, flags);
+}
+
 static int hibmc_pci_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
@@ -460,6 +486,8 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
 		goto err_return;
 	}
 
+	hibmc_set_enable_flag(priv);
+
 	ret = drm_dev_register(dev, 0);
 	if (ret) {
 		drm_err(dev, "failed to register drv for userspace access: %d\n",
@@ -472,6 +500,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
 	return 0;
 
 err_unload:
+	hibmc_set_disable_flag(to_hibmc_drm_private(dev));
 	hibmc_unload(dev);
 err_return:
 	return ret;
@@ -482,6 +511,7 @@ static void hibmc_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(dev);
+	hibmc_set_disable_flag(to_hibmc_drm_private(dev));
 	hibmc_unload(dev);
 }
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index dce8572bf63e..c2a82000ea7f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -49,6 +49,8 @@ struct hibmc_drm_private {
 	struct drm_crtc crtc;
 	struct hibmc_vdac vdac;
 	struct hibmc_dp dp;
+
+	spinlock_t gpio_lock; /* protects RMW on I2C/ENABLE_FLAG (0x0802A4) */
 };
 
 static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
index 44860011855e..4ceb87efec05 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
@@ -27,7 +27,11 @@ static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
 {
 	struct hibmc_vdac *vdac = data;
 	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
-	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
+	unsigned long flags;
+	u32 tmp_dir;
+
+	spin_lock_irqsave(&priv->gpio_lock, flags);
+	tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
 
 	if (value) {
 		tmp_dir &= ~mask;
@@ -41,20 +45,29 @@ static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
 		tmp_dir |= mask;
 		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
 	}
+	spin_unlock_irqrestore(&priv->gpio_lock, flags);
 }
 
 static int hibmc_get_i2c_signal(void *data, u32 mask)
 {
 	struct hibmc_vdac *vdac = data;
 	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
-	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
+	unsigned long flags;
+	u32 tmp_dir;
+	int ret;
+
+	spin_lock_irqsave(&priv->gpio_lock, flags);
+	tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
 
 	if ((tmp_dir & mask) != mask) {
 		tmp_dir &= ~mask;
 		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
 	}
 
-	return (readl(priv->mmio + GPIO_DATA) & mask) ? 1 : 0;
+	ret = (readl(priv->mmio + GPIO_DATA) & mask) ? 1 : 0;
+	spin_unlock_irqrestore(&priv->gpio_lock, flags);
+
+	return ret;
 }
 
 static void hibmc_ddc_setsda(void *data, int state)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
index 17b30c393b10..b95add5b253c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
@@ -200,5 +200,8 @@
 
 #define HIBMC_CRT_PALETTE                       0x80C00
 
+#define HIBMC_ENABLE_FLAG			0x802A4
+#define HIBMC_ENABLE_STATE			BIT(2)
+
 #define HIBMC_FIELD(field, value) (field(value) & field##_MASK)
 #endif
-- 
2.33.0

Re: [PATCH for drm-misc-fixes v4 1/2] drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/hisilicon/hibmc: lost HPD transitions between hardirq and threaded ISR
- [Medium] drm/hisilicon/hibmc: race between hibmc_dp_hpd_cfg() and AUX transfers
--

commit bbfebdb6730a49d33c7743f617c2581bd1dabe95
Author: Lin He <helin52@huawei.com>

drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status

This commit replaces the HPD status polling in the threaded interrupt handler
with reading HIBMC_DP_HPD_STATUS directly in the hardirq handler via
hibmc_dp_get_hpd_status(). It also initializes hpd_status to HIBMC_HPD_OUT and
skips connector detection when hpd_status is not HIBMC_HPD_IN.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index 2e9403b8bf3c8..10290fd5f503f 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
[ ... ]
> @@ -61,13 +59,13 @@ static int hibmc_dp_detect(struct drm_connector *connector,
>  {
>  	struct hibmc_dp *dp = to_hibmc_dp(connector);
>  	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
> +	int hpd_status = READ_ONCE(dp->hpd_status);
>  	int ret = connector_status_disconnected;
>  
> -	if (dp->irq_status) {
> -		if (dp_dev->hpd_status != HIBMC_HPD_IN) {
> -			ret = connector_status_disconnected;
> -			goto exit;
> -		}
> +	if (hpd_status != HIBMC_HPD_IN) {
> +		drm_dbg_dp(dp->drm_dev, "dp detect skipped, hpd (%d)\n",
> +			   hpd_status);
> +		goto exit;
>  	}
>  
>  	if (!hibmc_dp_get_dpcd(dp_dev)) {
> @@ -189,24 +187,26 @@ irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *)arg;
>  	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> -	int idx, exp_status;
> +	int status = READ_ONCE(priv->dp.hpd_status);
> +	int idx;
>  
>  	if (!drm_dev_enter(dev, &idx))
>  		return -ENODEV;
>  
> -	if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
> +	if (status == HIBMC_HPD_IN) {
>  		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
>  		hibmc_dp_hpd_cfg(&priv->dp);

[Severity: Medium]
Can hibmc_dp_hpd_cfg() race with active AUX transactions when
priv->dp.hpd_status is set to HIBMC_HPD_IN in the top-half handler before
hibmc_dp_hpd_isr() runs?

During hibmc_pci_probe(), hibmc_load() -> hibmc_dp_init() ->
hibmc_dp_hw_init() resets the DPTX block and sleeps 100 ms with msleep(100)
to latch the HPD interrupt. Next, drm_dev_register() calls
hibmc_dp_late_register() -> hibmc_dp_enable_int(), unmasking
HIBMC_DP_INTR_ENABLE. The latched HPD interrupt fires immediately in hardirq
context: hibmc_dp_interrupt() writes HIBMC_HPD_IN to priv->dp.hpd_status and
returns IRQ_WAKE_THREAD to schedule hibmc_dp_hpd_isr().

Right after drm_dev_register() returns, hibmc_pci_probe() calls
drm_client_setup() (or userspace calls GETCONNECTOR). Because
priv->dp.hpd_status is already HIBMC_HPD_IN, hibmc_dp_detect() proceeds into
hibmc_dp_get_dpcd(), hibmc_dp_connector_get_modes(), and
hibmc_dp_encoder_enable() -> hibmc_dp_mode_set() -> hibmc_dp_link_training(),
all of which perform AUX transfers via hibmc_dp_aux_xfer().

Concurrently, the IRQ thread hibmc_dp_hpd_isr() runs hibmc_dp_hpd_cfg()
without holding dev->mode_config.mutex or dp->aux.hw_mutex:

drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c:hibmc_dp_hpd_cfg() {
	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
	...
	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
}

In hibmc_dp_aux_xfer(), hibmc_dp_reg_write_field() sets HIBMC_DP_CFG_AUX_REQ
(BIT(0) of HIBMC_DP_AUX_REQ) to 1 under dp_dev->lock, then drops dp_dev->lock
and polls HIBMC_DP_AUX_REQ via readl_poll_timeout() until hardware clears
BIT(0). If hibmc_dp_hpd_cfg() performs its read-modify-writes on
HIBMC_DP_AUX_REQ while BIT(0) is 1 and writes the value back after hardware
clears BIT(0), it can re-trigger a spurious AUX transfer while
hibmc_dp_aux_xfer() reads the reply status/data, or mutate AUX timing fields
mid-transfer. Additionally, hibmc_dp_hpd_cfg() writes HIBMC_DP_DPTX_RST_CTRL
via raw writel() without holding dp_dev->lock, racing with the
read-modify-write in hibmc_dp_aux_reset().

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 602930964e930..df4501bb076bc 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -63,7 +63,8 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
>  
>  	status = readl(priv->mmio + HIBMC_DP_INTSTAT);
>  	if (status) {
> -		priv->dp.irq_status = status;
> +		WRITE_ONCE(priv->dp.hpd_status,
> +			   hibmc_dp_get_hpd_status(&priv->dp));
>  		writel(status, priv->mmio + HIBMC_DP_INTCLR);
>  		return IRQ_WAKE_THREAD;
>  	}

[Severity: High]
Could overwriting the single priv->dp.hpd_status field in
hibmc_dp_interrupt() lose HPD transitions between the hardirq handler and
hibmc_dp_hpd_isr()?

First, in hibmc_dp_hpd_isr(), hibmc_dp_reset_link() is only called when
status == HIBMC_HPD_OUT is observed by the threaded handler, and
hibmc_dp_reset_link() is the only place that resets
dp_dev->link.status.clock_recovered and channel_equalized to false. If an
HPD_OUT interrupt is followed by an HPD_IN interrupt before
hibmc_dp_hpd_isr() runs or while it is executing
drm_connector_helper_hpd_irq_event() (or if both unplug and plug bits are set
in HIBMC_DP_INTSTAT when hibmc_dp_interrupt() runs), priv->dp.hpd_status is
overwritten with HIBMC_HPD_IN. Then hibmc_dp_hpd_isr() calls
hibmc_dp_hpd_cfg() without calling hibmc_dp_reset_link(), leaving
channel_equalized set to true so subsequent calls to hibmc_dp_mode_set() skip
hibmc_dp_link_training():

drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c:hibmc_dp_mode_set() {
	if (!dp_dev->link.status.channel_equalized) {
		ret = hibmc_dp_link_training(dp_dev);
		if (ret) {
			drm_err(dp->drm_dev, "dp link training failed, ret: %d\n", ret);
			return ret;
		}
	}
}

Second, when hibmc_dp_hpd_isr() handles HIBMC_HPD_IN, it calls
hibmc_dp_hpd_cfg() in threaded context:

drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c:hibmc_dp_hpd_cfg() {
	...
	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
	...
}

Because HPD status is now sampled in hibmc_dp_interrupt() before
hibmc_dp_hpd_cfg() rather than after it, any unplug event that latches before
hibmc_dp_hpd_cfg() writes HIBMC_DP_INT_RST to HIBMC_DP_INTR_ORIGINAL_STATUS
(0x1e0728, the same register as HIBMC_DP_INTCLR) is cleared in hardware
without hibmc_dp_interrupt() updating priv->dp.hpd_status, leaving
priv->dp.hpd_status stuck at HIBMC_HPD_IN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060303.289737-1-shiyongbang@huawei.com?part=1

Re: [PATCH for drm-misc-fixes v4 2/2] drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/hisilicon/hibmc: unconditional enable flag set when DP init fails
--

commit 9960ec3a8f045c4b7198b1788310aca553de0cc3
Author: Lin He <helin52@huawei.com>

drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded

This commit adds a hardware register flag (HIBMC_ENABLE_STATE in
HIBMC_ENABLE_FLAG) to notify the BMC when the OS-side driver is loaded and
unloaded, preventing the BMC from enabling DP and triggering unhandled
interrupts before the driver is ready. It also introduces a spinlock to
serialize read-modify-write operations on the shared register.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index df4501bb076bc..15583a2d1c6fd 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c

[ ... ]

> @@ -460,6 +486,8 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>  		goto err_return;
>  	}
>  
> +	hibmc_set_enable_flag(priv);

[Severity: High]
Should hibmc_set_enable_flag() be called unconditionally here if DP
initialization failed or if fewer than two MSI vectors were allocated?

In drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c, hibmc_kms_init() treats
hibmc_dp_init() failures as non-fatal so that VDAC can still function:

	ret = readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL);
	if (ret) {
		ret = hibmc_dp_init(priv);
		if (ret)
			drm_err(dev, "failed to init dp: %d\n", ret);
	}

Meanwhile, hibmc_msi_init() either allocates 2 MSI vectors and registers
hibmc_dp_interrupt and hibmc_dp_hpd_isr unconditionally, or succeeds with
HIBMC_MIN_VECTORS (1) without registering the DP interrupt handler at all.

If hibmc_set_enable_flag() is called unconditionally in hibmc_pci_probe()
and the BMC subsequently enables DP and triggers a DP interrupt:

1. If hibmc_dp_init() failed before allocating priv->dp.dp_dev (or failed
   in hibmc_dp_serdes_init() before clearing HIBMC_DP_INTR_ENABLE),
   hibmc_dp_interrupt() calls hibmc_dp_get_hpd_status(&priv->dp), which
   dereferences a NULL dp->dp_dev pointer in hard IRQ context.

2. If hibmc_dp_init() failed during encoder or connector initialization,
   priv->dp.connector.dev remains NULL, and the threaded handler
   hibmc_dp_hpd_isr() calls drm_connector_helper_hpd_irq_event(),
   dereferencing NULL.

3. If pci_alloc_irq_vectors() only allocated 1 MSI vector, the DP interrupt
   has no handler registered.

Would it be safer to only set HIBMC_ENABLE_STATE when DP has been
successfully initialized and its MSI vector is registered?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060303.289737-1-shiyongbang@huawei.com?part=2