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@kernel.org]

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