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

[PATCH for drm-misc-fixes 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.

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    | 31 ++++++++++++-------
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  4 ++-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 29 ++++++++++-------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 14 +++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h  |  3 ++
 6 files changed, 58 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH for drm-misc-fixes 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>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 -
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 31 ++++++++++++-------
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  4 ++-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 29 ++++++++++-------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  1 +
 5 files changed, 42 insertions(+), 24 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..b3c7adaaedf7 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,25 @@ 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 ret = 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: /* 0: unplug */
+	case 3: /* 3: done */
+		ret = HIBMC_HPD_OUT;
+		break;
+	case 1: /* plug */
+		ret = HIBMC_HPD_IN;
+		break;
+	default:
+		break;
 	}
 
-	dp->dp_dev->hpd_status = exp_status;
-
-	return true;
+	return ret;
 }
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 0f3662d8737e..bcd4e9d155c8 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,
 };
@@ -55,6 +56,7 @@ struct hibmc_dp {
 	struct drm_dp_aux aux;
 	struct hibmc_dp_cbar_cfg cfg;
 	u32 irq_status;
+	int hpd_status;
 	int phys_status;
 };
 
@@ -66,7 +68,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 2e9403b8bf3c..23716d48149f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -63,12 +63,8 @@ static int hibmc_dp_detect(struct drm_connector *connector,
 	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
 	int ret = connector_status_disconnected;
 
-	if (dp->irq_status) {
-		if (dp_dev->hpd_status != HIBMC_HPD_IN) {
-			ret = connector_status_disconnected;
-			goto exit;
-		}
-	}
+	if (dp->hpd_status != HIBMC_HPD_IN)
+		goto exit;
 
 	if (!hibmc_dp_get_dpcd(dp_dev)) {
 		ret = connector_status_disconnected;
@@ -166,6 +162,9 @@ static void hibmc_dp_encoder_enable(struct drm_encoder *drm_encoder,
 	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
 	struct drm_display_mode *mode = &drm_encoder->crtc->state->mode;
 
+	if (dp->hpd_status != HIBMC_HPD_IN)
+		return;
+
 	if (hibmc_dp_prepare(dp, mode))
 		return;
 
@@ -189,24 +188,31 @@ 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 = 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) {
 		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
+		if (status != HIBMC_HPD_IN) {
+			drm_err(&priv->dev, "HPD status (%d) error", status);
+			goto exit;
+		}
 		hibmc_dp_hpd_cfg(&priv->dp);
-		exp_status = HIBMC_HPD_IN;
 	} else {
 		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
+		if (status != HIBMC_HPD_OUT) {
+			drm_err(&priv->dev, "HPD status (%d) error", status);
+			goto exit;
+		}
 		hibmc_dp_reset_link(&priv->dp);
-		exp_status = HIBMC_HPD_OUT;
 	}
 
-	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;
@@ -223,6 +229,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 4d85c89f3f88..e5cca7b63b78 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -63,6 +63,7 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
 	status = readl(priv->mmio + HIBMC_DP_INTSTAT);
 	if (status) {
 		priv->dp.irq_status = status;
+		priv->dp.hpd_status = hibmc_dp_get_hpd_status(&priv->dp);
 		writel(status, priv->mmio + HIBMC_DP_INTCLR);
 		return IRQ_WAKE_THREAD;
 	}
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Hardcoding the initial HPD status to `HIBMC_HPD_OUT` and unconditionally exiting in `hibmc_dp_detect()` breaks boot-time display detection for monitors connected before the driver loads.
- [High] Gating `hibmc_dp_encoder_enable` on HPD status breaks the DRM connector 'force on' functionality.
- [High] Reading the HPD state immediately in the hard IRQ handler without waiting for it to settle can read transient states, permanently discarding hotplug events.

Pre-existing issues:
- [High] Unconditional dereference of `dp->dp_dev` in the shared interrupt handler causes a kernel panic when the DP block is uninitialized.
- [Medium] Unprotected concurrent access to shared IRQ state variables (`irq_status` and `hpd_status`) can lead to TOCTOU races and dropped interrupts.
--

--- Patch [1]: [PATCH for drm-misc-fixes 1/2] drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status ---
commit 7415e727057e1237039f87fc5bb416de2f8548f4
Author: Lin He <helin52@huawei.com>
drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status

This commit changes how the HPD (Hot Plug Detect) status is retrieved for
the DP connector. It replaces the polling mechanism in the lower-half
interrupt handler with a direct status read in the upper-half handler, and
sets the initial HPD status to disconnected.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
[ ... ]
> @@ -322,20 +326,25 @@ void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
> -bool hibmc_dp_check_hpd_status(struct hibmc_dp *dp, int exp_status)
> +int hibmc_dp_get_hpd_status(struct hibmc_dp *dp)
>  {
> +	int ret = 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));

[Severity: High]
This is a pre-existing issue, but could this dereference dp->dp_dev when it
is NULL?

If hibmc_kms_init() skips hibmc_dp_init(), dp->dp_dev remains NULL. If a
spurious or shared interrupt fires, the hardirq handler calls
hibmc_dp_get_hpd_status() and immediately dereferences dp->dp_dev->base
causing a kernel panic.

[Severity: High]
Could this instantaneous read capture a transient state and permanently drop
hotplug events?

The previous code used readl_poll_timeout() to wait up to 100ms for the
bouncing HPD signal to settle, as required by the DP spec.

If this read occurs during a physical signal bounce upon plug-in, could it
return HIBMC_HPD_UNKNOWN and cause the threaded handler to silently drop the
event instead of waiting for the signal to stabilize?

> +
> +	switch (status) {
> +	case 0: /* 0: unplug */
> +	case 3: /* 3: done */
> +		ret = HIBMC_HPD_OUT;
> +		break;
> +	case 1: /* plug */
> +		ret = HIBMC_HPD_IN;
> +		break;
> +	default:
> +		break;
>  	}
>  
> -	dp->dp_dev->hpd_status = exp_status;
> -
> -	return true;
> +	return ret;
>  }
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -63,12 +63,8 @@ static int hibmc_dp_detect(struct drm_connector *connector,
>  	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>  	int ret = connector_status_disconnected;
>  
> -	if (dp->irq_status) {
> -		if (dp_dev->hpd_status != HIBMC_HPD_IN) {
> -			ret = connector_status_disconnected;
> -			goto exit;
> -		}
> -	}
> +	if (dp->hpd_status != HIBMC_HPD_IN)
> +		goto exit;

[Severity: High]
Does this unconditional exit prevent the detection of displays connected
before the driver loads?

Since the initial state is hardcoded to HIBMC_HPD_OUT and we never poll
the hardware register to get the actual physical state during initialization,
if a monitor is already plugged in at boot, no edge-triggered interrupt
will fire.

Will this cause hibmc_dp_detect() to unconditionally return
connector_status_disconnected for displays connected at boot time?

>  
>  	if (!hibmc_dp_get_dpcd(dp_dev)) {
>  		ret = connector_status_disconnected;
[ ... ]
> @@ -166,6 +162,9 @@ static void hibmc_dp_encoder_enable(struct drm_encoder *drm_encoder,
>  	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
>  	struct drm_display_mode *mode = &drm_encoder->crtc->state->mode;
>  
> +	if (dp->hpd_status != HIBMC_HPD_IN)
> +		return;
> +

[Severity: High]
Could this early return break the DRM connector force on functionality?

If userspace explicitly forces the DP connector on when the physical HPD is
disconnected or broken, the DRM core will commit a modeset expecting the
encoder to be programmed.

By returning early here based on the physical connection state, we bypass
the hardware programming, leaving it out of sync with the DRM atomic state.
Might this lead to VBLANK timeouts or pipeline hangs?

>  	if (hibmc_dp_prepare(dp, mode))
>  		return;
>  
> @@ -189,24 +188,31 @@ 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 = 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) {

[Severity: Medium]
This is a pre-existing issue, but could this lead to a race condition that
drops interrupts?

The hardirq handler writes to priv->dp.irq_status and priv->dp.hpd_status
without locks, while this threaded handler reads them non-atomically.

If a hardirq preempts the threaded handler after it reads priv->dp.hpd_status
into the local status variable but before evaluating priv->dp.irq_status,
it will compare a stale status against a new irq_status. Will this mismatch
trigger the error paths below and improperly discard the event?

>  		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
> +		if (status != HIBMC_HPD_IN) {
> +			drm_err(&priv->dev, "HPD status (%d) error", status);
> +			goto exit;
> +		}
>  		hibmc_dp_hpd_cfg(&priv->dp);
[ ... ]
> @@ -223,6 +229,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);

[Severity: High]
Does hardcoding the initial state to HIBMC_HPD_OUT without checking the
actual hardware register contribute to the boot-time detection issue
mentioned in hibmc_dp_detect() above?

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

[Severity: High]
This is a pre-existing issue, but as mentioned above, if hibmc_dp_init() was
skipped and the DP block is uninitialized, does calling
hibmc_dp_get_hpd_status() here result in a NULL pointer dereference on
dp->dp_dev?

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

[PATCH for drm-misc-fixes 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>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c  | 13 +++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index e5cca7b63b78..6436263a6c03 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -424,6 +424,16 @@ static int hibmc_load(struct drm_device *dev)
 	return ret;
 }
 
+static inline void hibmc_set_enable_flag(struct hibmc_drm_private *priv)
+{
+	writel(HIBMC_ENABLE_STATE, priv->mmio + HIBMC_ENABLE_FLAG);
+}
+
+static inline void hibmc_set_disable_flag(struct hibmc_drm_private *priv)
+{
+	writel(0, priv->mmio + HIBMC_ENABLE_FLAG);
+}
+
 static int hibmc_pci_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
@@ -468,6 +478,8 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
 
 	drm_client_setup(dev, NULL);
 
+	hibmc_set_enable_flag(priv);
+
 	return 0;
 
 err_unload:
@@ -480,6 +492,7 @@ static void hibmc_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
+	hibmc_set_disable_flag(to_hibmc_drm_private(dev));
 	drm_dev_unregister(dev);
 	hibmc_unload(dev);
 }
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.43.0

Re: [PATCH for drm-misc-fixes 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 2 potential issue(s) to consider:
- [High] Directly writing to the `HIBMC_ENABLE_FLAG` register clobbers the `GPIO_DATA_DIRECTION` I2C bits and creates a data race.
- [High] The disable flag is set too early in the teardown sequence, allowing the BMC to re-enable DP hardware while the OS driver is still active.
--

--- Patch [2]: [PATCH for drm-misc-fixes 2/2] drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded ---
commit bfee57e8491146f1282ee8dc01bf699cf1bf50ed
Author: Lin He <helin52@huawei.com>

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

This patch adds 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.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index e5cca7b63b787..6436263a6c039 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c

[ ... ]

> @@ -424,6 +424,16 @@ static int hibmc_load(struct drm_device *dev)
>  	return ret;
>  }
>  
> +static inline void hibmc_set_enable_flag(struct hibmc_drm_private *priv)
> +{
> +	writel(HIBMC_ENABLE_STATE, priv->mmio + HIBMC_ENABLE_FLAG);
> +}
> +
> +static inline void hibmc_set_disable_flag(struct hibmc_drm_private *priv)
> +{
> +	writel(0, priv->mmio + HIBMC_ENABLE_FLAG);
> +}

[Severity: High]
Does this direct writel() overwrite the I2C bus state?

The HIBMC_ENABLE_FLAG offset is 0x802A4, which aliases the
GPIO_DATA_DIRECTION I2C register used by the driver's I2C bit-banging code.

Writing directly instead of using a read-modify-write clears the I2C
mask bits that might be currently active. Direct writing of 0 clears both
the enable bit and the I2C direction bits.

>  static int hibmc_pci_probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {

[ ... ]

> @@ -468,6 +478,8 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>  
>  	drm_client_setup(dev, NULL);
>  
> +	hibmc_set_enable_flag(priv);
> +

[Severity: High]
Can this call to hibmc_set_enable_flag() race with I2C EDID reads?

Calling this immediately after drm_client_setup() means fbdev initialization
can asynchronously probe connectors and trigger I2C bit-banging.

Without locking between the I2C code's read-modify-write cycle and this
direct write, the I2C code might overwrite and clear the enable flag,
reverting the BMC driver status this patch intended to set.

>  	return 0;
>  
>  err_unload:
> @@ -480,6 +492,7 @@ static void hibmc_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> +	hibmc_set_disable_flag(to_hibmc_drm_private(dev));

[Severity: High]
Is it safe to clear the disable flag before userspace is locked out and the
hardware is quiesced?

By clearing the flag before calling drm_dev_unregister() and hibmc_unload(),
the BMC might attempt to take over the DP hardware or ignore interrupts
while the OS is still actively managing it. This could race with the shutdown
of the display hardware in hibmc_unload().

>  	drm_dev_unregister(dev);
>  	hibmc_unload(dev);
>  }

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