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