On Sat, 25 Jul 2026 05:04:37 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 11 potential
> issue(s) to consider:
> - [High] Custom EDID read function bypasses DRM core and breaks I2C
> repeated-start (MOT) rules, corrupting EDID reads for blocks > 0 or on active
> adapters.
> - [High] The hardirq handler returns `IRQ_NONE` after clearing hardware
> interrupt flags, permanently disabling the IRQ line.
> - [High] Late attachment of the eDP panel bridge violates
> drm_bridge_connector architecture and breaks panel support if the panel
> probes asynchronously.
> - [High] DP Transfer Unit (TU) miscalculation due to pixel clock truncation.
> - [High] The IRQ handler returns `IRQ_NONE` without clearing the hardware
> interrupt when `drm_dev_enter()` fails, leading to an interrupt storm.
> - [High] `spacemit_dp_bridge_atomic_create_state` returns an error pointer
> instead of `NULL` on allocation failure, causing a kernel crash in the DRM
> core.
> - [High] The hardirq handler calls regmap functions which use a sleepable
> lock on PREEMPT_RT.
> - [Medium] The atomic_check and mode_valid callbacks access DPCD link
> capabilities without holding mode_lock, leading to data races with the
> hotplug IRQ handler.
> - [Medium] The DP IRQ handler silently drops hotplug notifications because
> drm_bridge_hpd_notify() is a no-op when DRM_BRIDGE_OP_HPD is omitted.
> - [Medium] Calling clk_set_parent() on an already enabled clock.
> - [Medium] Resetting the AUX controller without holding aux->hw_mutex races
> with concurrent AUX transfers.
> --
>
> --- Patch [12]: [PATCH RESEND 12/17] drm/spacemit: add Innosilicon DP/eDP
> controller bridge driver ---
> commit b7eace372254ab27e120213d38c3a656b2a195b8
> 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.
>
> > 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
> > @@ -248,16 +248,10 @@
> > static inline struct spacemit_dp_bridge_state *
> > to_spacemit_dp_bridge_state(struct drm_bridge_state *state)
> > {
> > return container_of(state, struct spacemit_dp_bridge_state, base);
> > }
> >
> > static const struct regmap_config spacemit_dp_regmap_config = {
> > .name = "dp",
> > .reg_bits = 32,
> > .val_bits = 32,
> > .reg_stride = 4,
> > .max_register = 0x3ffc, /* full 0x4000 reg region from binding
> > */
> > };
>
> [Severity: High]
> Is it safe to omit use_raw_spinlock from the regmap configuration? Because
> this is not set to true, the regmap defaults to a sleepable lock on
> PREEMPT_RT. This causes a BUG when regmap read/write functions are called
> from the hardirq handler spacemit_dp_irq_handler().
The handler is force-threaded on PREEMPT_RT (no IRQF_NO_THREAD), so
the regmap spinlock is fine there.
> > [ ... ]
> > @@ -661,16 +661,10 @@
> > static void spacemit_dp_link_disable(struct spacemit_dp_dev *dp)
> > {
> > regmap_write_bits(dp->regs, DP_VIDEO_VSAMPLE_REG,
> > DP_VIDEO_STREAM_ENABLE,
> > FIELD_PREP(DP_VIDEO_STREAM_ENABLE, 0));
> > spacemit_dp_phy_off(dp);
> >
> > regmap_write_bits(dp->regs, DP_SOFT_RESET, DP_AUX_RESET,
> > FIELD_PREP(DP_AUX_RESET, 0x1));
>
> [Severity: Medium]
> Could this reset corrupt concurrent AUX transfers? Modifying the AUX
> hardware in spacemit_dp_link_disable() without holding the aux->hw_mutex
> could race against a userspace or deferred transfer, potentially wedging
> the controller.
All driver-internal AUX users serialize on mode_lock; the EDID
rework below also serializes with the userspace i2c node.
> > [ ... ]
> > @@ -936,15 +936,10 @@
> > static void spacemit_dp_hw_set_msa_and_enable_video(struct spacemit_dp_dev
> > *dp,
> > const struct
> > drm_display_mode *mode,
> > enum spacemit_dp_link_rate rate,
> > enum spacemit_dp_lane_count
> > lanes,
> > u32 color_format)
> > {
> > u64 hb_num;
> > u32 link_rate;
> > u32 fp; /* Pixel clock in MHz */
> > u32 bpp, misc0;
> > u32 tu, tu_frac, tu_int, rd_thres;
> > u32 hsync_len;
> >
> > /* 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;
>
> [Severity: High]
> Can this truncation of the pixel clock to an integer MHz value cause DP
> FIFO underflows or overflows? For non-integer MHz pixel clocks like
> 148.5 MHz, this truncates to 148, introducing precision errors in the
> Transfer Unit (TU) calculations which results in corrupted video.
The values are vendor-faithful and board-proven at 4K; computing in
kHz is a possible later refinement, not a v2 change.
> > [ ... ]
> > @@ -1286,13 +1286,10 @@
> > static int spacemit_dp_conn_get_edid_block(void *data, u8 *buf,
> > unsigned int block, size_t len)
> > {
> > struct spacemit_dp_dev *dp = data;
> > unsigned int start = block * EDID_LENGTH;
> > u8 segment = block >> 1;
> > int ret, retry;
> > size_t offset;
> >
> > if (segment) {
> > for (retry = 0; retry < 3; retry++) {
> > ret = spacemit_dp_aux_i2c_write(dp,
> > SPACEMIT_DP_DDC_SEGMENT_ADDR, &segment, 1);
>
> [Severity: High]
> Does this custom EDID read function bypass the Middle-Of-Transaction (MOT)
> rules for I2C repeated-start? Issuing writes and reads without the MOT flag
> resets the DDC segment pointer to 0, breaking EDID reads for displays with
> multiple EDID blocks (block >= 1) or on active adapters.
Will rework in v2: the custom reader becomes one i2c_transfer per
block on the aux I2C adapter, so the DP I2C-over-AUX core chains
segment, offset and data with MOT, and 3+ block EDIDs work.
> > [ ... ]
> > @@ -1632,15 +1632,10 @@
> > static int spacemit_dp_bridge_atomic_check(struct drm_bridge *bridge,
> > struct drm_bridge_state
> > *bridge_state,
> > struct drm_crtc_state *crtc_state,
> > struct drm_connector_state *conn_state)
> > {
> > struct spacemit_dp_dev *dp = container_of(bridge,
> > struct spacemit_dp_dev,
> > bridge);
> > struct spacemit_dp_bridge_state *st =
> > to_spacemit_dp_bridge_state(bridge_state);
> > const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> > int bpp;
> > u32 req_bw;
> > int i;
> >
> > /* A single output bus format, see ->atomic_get_output_bus_fmts(). */
> > st->color_format = SOC_VIDEO_RGB_8BIT;
> > bpp = spacemit_dp_get_bpp(dp, st->color_format);
> > req_bw = spacemit_dp_calc_required_bw(mode, bpp);
> >
> > /* No caps yet: leave idx unset and let ->atomic_enable() retry. */
> > if (!dp->link.max_rate || !dp->link.max_num_lanes) {
>
> [Severity: Medium]
> Could reading the DPCD link capabilities here race with the hotplug IRQ
> handler? Accessing the dp->link capabilities in
> spacemit_dp_bridge_atomic_check() without holding mode_lock might observe
> intermediate or torn states if a concurrent interrupt triggers a capability
> re-read, leading to transient rejections of valid modes.
detect(), enable and the EDID paths all serialize on mode_lock.
> > [ ... ]
> > @@ -1705,10 +1705,10 @@
> > static struct drm_bridge_state *
> > spacemit_dp_bridge_atomic_create_state(struct drm_bridge *bridge)
> > {
> > struct spacemit_dp_bridge_state *new;
> >
> > new = kzalloc_obj(*new);
> > if (!new)
> > return ERR_PTR(-ENOMEM);
>
> [Severity: High]
> Should this return NULL on allocation failure instead of an error pointer?
> The DRM core's reset helper assigns the returned state directly without
> checking for errors. Returning an error pointer here causes a fatal page
> fault when the DRM core later dereferences it.
ERR_PTR is what the bridge core expects here;
drm_bridge_atomic_create_priv_state() checks IS_ERR().
> > [ ... ]
> > @@ -1742,16 +1742,10 @@
> > static int spacemit_dp_edp_link_panel(struct drm_dp_aux *aux)
> > {
> > struct spacemit_dp_dev *dp = container_of(aux, struct spacemit_dp_dev,
> > aux);
> > struct drm_bridge *next;
> > int ret;
> >
> > next = devm_drm_of_get_bridge(dp->dev, dp->dev->of_node, 1, 0);
> > if (IS_ERR(next))
> > return PTR_ERR(next);
> >
> > dp->next_bridge = next;
> >
> > /*
> > * A non-NULL bridge.encoder means .attach already ran: late-chain
> > * panel-edp.
> > */
> > if (dp->bridge.encoder) {
> > ret = drm_bridge_attach(dp->bridge.encoder, next,
> > &dp->bridge,
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>
> [Severity: High]
> Does dynamically attaching the panel bridge late bypass connector
> initialization? If the eDP panel defers its probe,
> drm_bridge_connector_init() builds the connector without the panel bridge.
> Attaching it here later violates the drm_bridge_connector architecture and
> breaks panel support, because the connector will lack the panel's mode or
> EDID operations.
Handled by design: the aux-bus done-probing callback late-chains
with DRM_BRIDGE_ATTACH_NO_CONNECTOR and re-fires detection; both
probe orders are exercised on hardware. See the comment above
spacemit_dp_edp_link_panel().
> > [ ... ]
> > @@ -1782,10 +1782,10 @@
> > static irqreturn_t spacemit_dp_irq_handler(int irq, void *data)
> > {
> > struct spacemit_dp_dev *dp = data;
> > irqreturn_t ret = IRQ_NONE;
> > int idx;
> >
> > if (!drm_dev_enter(dp->drm, &idx))
> > return IRQ_NONE;
>
> [Severity: High]
> Will returning IRQ_NONE here without clearing the hardware interrupt cause
> an interrupt storm? If an interrupt fires while the DRM device is
> unregistered (such as during module unload), the handler exits without
> acknowledging the hardware interrupt, causing it to continuously fire and
> stall the CPU.
Only reachable during unbind while the line is being torn down.
> > [ ... ]
> > @@ -1796,11 +1796,10 @@
> > /*
> > * 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: High]
> Is it safe to return IRQ_NONE after clearing the hardware interrupt flags?
> If the logical state has not changed (e.g. from a transient hardware
> glitch), returning IRQ_NONE causes the kernel's spurious interrupt detector
> to eventually disable the IRQ line, which results in all future hotplug
> events being permanently missed.
Real; will return IRQ_HANDLED in v2 when a latched HPD event was
consumed.
> > [ ... ]
> > @@ -1813,11 +1813,10 @@
> > static irqreturn_t spacemit_dp_hotplug_event_handler(int irq, void *data)
> > {
> > struct spacemit_dp_dev *dp = data;
> > enum drm_connector_status status;
> > int idx;
> >
> > if (!drm_dev_enter(dp->drm, &idx))
> > return IRQ_HANDLED;
> >
> > /*
> > * .detect() owns mode_lock and re-reads the sink caps on a fresh plug
> > */
> > status = spacemit_dp_bridge_detect(&dp->bridge, dp->connector);
> > drm_bridge_hpd_notify(&dp->bridge, status);
>
> [Severity: Medium]
> Will this hotplug notification be silently dropped? Since
> DRM_BRIDGE_OP_HPD is not set for the DP bridge during inno_dp_probe(), the
> bridge->hpd_cb is NULL, making drm_bridge_hpd_notify() a no-op and forcing
> the DRM core to rely entirely on periodic polling.
Deliberate: the HPD interrupt is gated by the DP pixel clock, so
the connector is polled instead, and the IRQ path still refreshes
status early when it does fire. Documented at the ops assignment.
> > [ ... ]
> > @@ -2338,10 +2338,15 @@
> > static int inno_dp_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > [ ... ]
> > if (dp->pxclk) {
> > ret = clk_prepare_enable(dp->pxclk);
> > if (ret) {
> > dev_err(dev, "failed to enable pxclk: %d\n", ret);
> > goto err_reset;
> > }
> > }
> > [ ... ]
> > if (dp->pxclk) {
> > ret = clk_set_parent(dp->pxclk, dp->pll_clk);
>
> [Severity: Medium]
> Can calling clk_set_parent() on an already enabled clock cause hardware
> glitches? The pxclk is prepared and enabled earlier in inno_dp_probe(), and
> reparenting a running clock without hardware-specific glitch-free muxing
> can lock up downstream logic.
The mux is reparented before the display is enabled; nothing scans
while it switches at probe.
Cody
