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 <[email protected]> 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/[email protected]?part=2
