Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Early return bypasses secondary status clearing, causing an IRQ storm. - [High] Casting opaque `hpd_data` to `struct drm_connector *` causes type confusion and potential kernel panic. - [High] Lockless connector access introduces a Use-After-Free window. - [Medium] Unconditionally writing 0xFF to write-1-to-clear status registers causes lost hardware events. - [Medium] Bitwise ORing negative error codes produces meaningless values. --
--- Patch [1]: [PATCH 1/2] drm/bridge: ti-sn65dsi86: improve HPD interrupt handling --- commit 7011d7627c6af8ae9fd09f6d42480bb03a988154 Author: Yashas D <[email protected]> drm/bridge: ti-sn65dsi86: improve HPD interrupt handling Fix the interrupt handler to clear all three IRQ status registers to fully de-assert the IRQ pin, enable replug event detection, and use per-connector hotplug notification instead of polling all connectors on every DP HPD event. > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 48b83df9aed61..d9bd4ef8f0e21 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c [ ... ] > @@ -1387,23 +1391,35 @@ static irqreturn_t ti_sn_bridge_interrupt(int irq, > void *private) > return IRQ_NONE; > } > > - hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS); > + hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS | > + HPD_REPLUG_STATUS); > > dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status); > if (!status) > return IRQ_NONE; [Severity: Critical] Is this a regression where the early return bypasses the newly added clearing logic? If the interrupt was triggered exclusively by an event in SN_IRQ_STATUS2_REG or SN_IRQ_STATUS3_REG, status will be 0 and the handler returns immediately. Will the hardware IRQ pin remain asserted, leading to an infinite interrupt loop? > > - ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status); > + /* > + * Clear all three IRQ status registers to fully de-assert > + * the IRQ pin > + */ > + ret = regmap_write(pdata->regmap, SN_IRQ_STATUS2_REG, 0xFF); [Severity: Medium] Will this unconditionally acknowledge and clear all pending events in these write-1-to-clear registers? Because the registers aren't read first, might this cause the driver to permanently lose secondary hardware interrupt events or error flags? > + ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS3_REG, 0xFF); > + ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status); [Severity: Medium] Does bitwise ORing the returns of regmap_write() corrupt the error values? Since regmap_write() returns negative error codes (e.g., -EIO), ORing them combines their two's complement bits to produce an unrelated negative number, which could mangle the error code passed to dev_err(). > if (ret) { > dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret); > return IRQ_NONE; > } > > - /* Only send the HPD event if we are bound with a device. */ > + /* Notify only the DP connector, not all connectors on the device. */ > mutex_lock(&pdata->hpd_mutex); > - if (pdata->hpd_enabled && hpd_event) > - drm_kms_helper_hotplug_event(dev); > - mutex_unlock(&pdata->hpd_mutex); > + if (pdata->hpd_enabled && hpd_event && pdata->bridge.hpd_data) { > + struct drm_connector *connector = > + (struct drm_connector *)pdata->bridge.hpd_data; [Severity: High] Does casting the opaque bridge.hpd_data directly to a drm_connector pointer cause type confusion? The hpd_data pointer is an opaque private context owned by the bridge consumer. If a driver passes a different structure or a drm_device pointer, will this cause a kernel panic when dereferenced by drm_connector_helper_hpd_irq_event()? > + mutex_unlock(&pdata->hpd_mutex); > + drm_connector_helper_hpd_irq_event(connector); [Severity: High] Does dropping the hpd_mutex lock here introduce a use-after-free regression? The lifetime of hpd_data is only guaranteed until drm_bridge_hpd_disable() returns. If a concurrent unbind operation completes and frees the connector memory while this interrupt handler is executing drm_connector_helper_hpd_irq_event(), can this result in memory corruption? Would it be safer to retain the lock or take a reference via drm_connector_get() before unlocking? > + } else { > + mutex_unlock(&pdata->hpd_mutex); > + } > > return IRQ_HANDLED; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
