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(). > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] > @@ -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. > [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
