Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [High] Missing W1C handling on DP_GENERAL_INTERRUPT causes unintentionally cleared interrupts. - [High] Late dynamic bridge attachment breaks `drm_bridge_connector_init` for eDP panels, resulting in 0 modes. - [High] Custom EDID reading bypasses standard DRM core helpers and breaks I2C MOT semantics. - [High] Integer truncation of the pixel clock causes inaccurate DisplayPort Transfer Unit (TU) and HBLANK calculations. - [High] Link training aborts prematurely because successful DPCD writes are evaluated as errors. - [High] Asynchronous panel probe causes Use-After-Free due to improper unbind/error cleanup order. - [Medium] Returning IRQ_NONE after actively clearing pending hardware interrupts violates the IRQ API. - [Medium] Sink capabilities are read concurrently without synchronization. - [Medium] Hardware shutdown sequence modifies registers without acquiring `dp->mode_lock`. --
--- Patch [12]: [PATCH 12/17] drm/spacemit: add Innosilicon DP/eDP controller bridge driver --- commit 7a2976d5d6f50aaede0b18a3f5e99c3cb67bd5c1 Author: Cody Kang <[email protected]> drm/spacemit: add Innosilicon DP/eDP controller bridge driver Add the DP/eDP controller that sits downstream of the Saturn DPU. Two identical instances share one compatible; the eDP-vs-DP role is board wiring, so it is taken from the devicetree: an eDP panel always sits under an aux-bus child node, an external DP connector never does. The link is driven through the generic PHY framework, so the controller never touches a PLL register. The controller's HPD interrupt is gated by the DP pixel clock, which can be off exactly when a plug has to be caught, so the connector is also polled and the interrupt path re-reads the live level when it does fire. > diff --git a/drivers/gpu/drm/spacemit/spacemit_inno_dp.c > b/drivers/gpu/drm/spacemit/spacemit_inno_dp.c > index 0000000000000..e88731d9b18bf > --- /dev/null > +++ b/drivers/gpu/drm/spacemit/spacemit_inno_dp.c [ ... ] > @@ -290,6 +290,9 @@ static void spacemit_dp_aux_hw_reset(struct > spacemit_dp_dev *dp) > usleep_range(2000, 2500); > regmap_write_bits(dp->regs, DP_SOFT_RESET, DP_AUX_RESET, > FIELD_PREP(DP_AUX_RESET, 0x0)); > usleep_range(2000, 2500); > regmap_write_bits(dp->regs, DP_GENERAL_INTERRUPT, > DP_AUX_REPLY_EVENT_INT_STA, > FIELD_PREP(DP_AUX_REPLY_EVENT_INT_STA, 1)); > } [Severity: High] Does this unintentionally clear other pending interrupts? DP_GENERAL_INTERRUPT appears to be a write-1-to-clear register. Calling regmap_write_bits() performs a read-modify-write operation, which reads all currently asserted interrupt bits and writes them back as 1s. This drops any unrelated pending interrupts (such as HDCP, overflow events, and hotplugs) that arrived concurrently. [ ... ] > @@ -401,9 +401,12 @@ static ssize_t spacemit_dp_aux_transfer(struct > drm_dp_aux *aux, > regmap_read(dp->regs, DP_AUX_STS_REG, > &status); status = FIELD_GET(DP_AUX_STATUS, status); > > /* Write 1 to clear. */ > regmap_write_bits(dp->regs, DP_GENERAL_INTERRUPT, > DP_AUX_REPLY_EVENT_INT_STA, > FIELD_PREP(DP_AUX_REPLY_EVENT_INT_STA, 1)); [Severity: High] Can this read-modify-write on a write-1-to-clear register silently drop other pending hardware interrupts that arrive during the AUX transfer? [ ... ] > @@ -806,6 +806,8 @@ static int spacemit_dp_aux_i2c_write(struct > spacemit_dp_dev *dp, u32 address, > .buffer = (u8 *)buf, > .size = len, > }; > int ret; > > ret = spacemit_dp_aux_transfer(&dp->aux, &msg); > if (ret < 0) > return ret; [Severity: High] Does this custom I2C over AUX implementation bypass synchronization? By directly invoking spacemit_dp_aux_transfer() here rather than using standard DRM core helpers, the code bypasses aux->hw_mutex, allowing concurrent AUX transactions on the bus. [ ... ] > @@ -831,6 +831,10 @@ static int spacemit_dp_link_train_clock_recovery(struct > spacemit_dp_dev *dp, > > if (drm_dp_clock_recovery_ok(link_status, lanes)) > return 0; > > ret = spacemit_dp_link_apply_adjust(dp, link_status, lanes, > training_set); > if (ret) { > spacemit_dp_set_training_pattern(dp, > DP_TRAINING_PATTERN_DISABLE); > return ret; > } [Severity: High] Could this return value check cause link training to fail? drm_dp_dpcd_write_data() returns the number of bytes written on success (a positive value). Since spacemit_dp_link_apply_adjust() returns that value directly, this "if (ret)" check will evaluate to true for successful writes. This causes the driver to interpret successful DPCD writes as failures and immediately abort the training sequence. [ ... ] > @@ -854,6 +854,10 @@ static int spacemit_dp_conn_get_edid_block(void *data, > u8 *buf, > for (retry = 0; retry < 3; retry++) { > ret = spacemit_dp_aux_i2c_write(dp, DDC_ADDR, > &edid_offset, 1); > if (ret) > continue; > > ret = spacemit_dp_aux_i2c_read(dp, DDC_ADDR, > buf + offset, chunk); > if (!ret) > break; [Severity: High] Can splitting the EDID write and read cause corrupted EDID reads? The I2C offset write and data read are split into two separate transactions without the DP_AUX_I2C_MOT (Middle-Of-Transaction) flag. This generates an I2C STOP condition between them, which causes many DP sinks to reset their internal EDID offset to 0. [ ... ] > @@ -954,6 +954,9 @@ static void > spacemit_dp_hw_set_msa_and_enable_video(struct spacemit_dp_dev *dp, > > /* mode->clock unit is kHz, fp unit is MHz */ > if (dp->use_ext_pixel_clock) > fp = dp->pixel_clock / 1000; > else > fp = mode->clock / 1000; > > if (fp == 0) [Severity: High] Can this integer division truncate fractional pixel clocks? If mode->clock is a fractional value like 148500 (148.5 MHz), dividing by 1000 truncates it to 148. This loss of precision skews the subsequent DisplayPort Transfer Unit (tu) and horizontal blanking interval (hb_num) calculations, which could cause display output corruption or video FIFO underflows. [ ... ] > @@ -1650,6 +1650,9 @@ static int spacemit_dp_bridge_atomic_check(struct > drm_bridge *bridge, > > /* No caps yet: leave idx unset and let ->atomic_enable() retry. */ > if (!dp->link.max_rate || !dp->link.max_num_lanes) { > st->link_cfg_idx = -1; > return 0; > } > > for (i = 0; i < ARRAY_SIZE(spacemit_dp_link_priority_table); i++) { [Severity: Medium] Is it safe to read dp->link capabilities here without holding dp->mode_lock? This read races with spacemit_dp_bridge_detect(), which updates these fields during a hotplug event. A concurrent hotplug could cause the validation path to see torn or inconsistent sink capabilities. [ ... ] > @@ -1777,6 +1777,10 @@ static irqreturn_t spacemit_dp_irq_handler(int irq, > void *data) > > /* > * Only compare here: the authoritative connector_status update > * happens in the thread under mode_lock, via .detect(). > */ > if (spacemit_dp_hw_detect_hpd(dp) != READ_ONCE(dp->connector_status)) > ret = IRQ_WAKE_THREAD; /* Call hotplug_event */ > spacemit_dp_hw_clean_hpd(dp); > > drm_dev_exit(idx); > return ret; > } [Severity: Medium] Should this return IRQ_HANDLED when interrupts are explicitly cleared? The handler calls spacemit_dp_hw_clean_hpd(), which reads and clears pending hardware interrupt bits. If the current HPD level matches the cached state (which can happen during physical contact bounce), the handler returns IRQ_NONE. Returning IRQ_NONE after servicing and clearing a hardware interrupt violates the IRQ API and can cause the kernel's spurious interrupt detector to permanently disable the DP interrupt line. [ ... ] > @@ -2129,6 +2129,9 @@ static int spacemit_dp_bind(struct device *dev, struct > device *master, > > dp->connector = drm_bridge_connector_init(drm, &dp->encoder); > if (IS_ERR(dp->connector)) { > ret = PTR_ERR(dp->connector); > dev_err(dev, "failed to init bridge connector: %d\n", ret); > goto err_encoder; > } [Severity: High] Does this initialize the connector before the panel bridge is attached? For eDP, the downstream panel bridge is attached dynamically later via the async probe callback spacemit_dp_edp_link_panel(). Calling drm_bridge_connector_init() here before the panel is attached means the connector is built without the panel's mode-getting function, resulting in it reporting 0 modes to userspace. [ ... ] > @@ -2160,6 +2160,9 @@ static int spacemit_dp_bind(struct device *dev, struct > device *master, > return 0; > > err_encoder: > drm_encoder_cleanup(&dp->encoder); > err_aux: > drm_dp_aux_unregister(&dp->aux); > return ret; > } [Severity: High] Can this cleanup order cause a use-after-free? drm_encoder_cleanup() is called before drm_dp_aux_unregister(). This leaves a window where the eDP panel's asynchronous done_probing callback (spacemit_dp_edp_link_panel) can still execute. It will attempt to dereference dp->bridge.encoder (which has been zeroed here) and the parent DRM device, leading to memory corruption. [ ... ] > @@ -2327,6 +2327,8 @@ static void inno_dp_shutdown(struct platform_device > *pdev) > /* A deferred or failed bind leaves drvdata NULL. */ > if (!dp) > return; > > spacemit_dp_hw_disable(dp); > } [Severity: Medium] Is it safe to modify hardware registers here without acquiring dp->mode_lock? spacemit_dp_hw_disable() modifies numerous DP configuration registers. Running this during system shutdown without holding the lock allows it to race with concurrent atomic modesets executed by the DRM core, potentially causing hardware lockups. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
