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

[sashiko-bot@kernel.org]

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