Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] Usage of sleep-capable functions in atomic CRTC and plane hardware programming callbacks. - [High] Unchecked return value of `clk_round_rate` leads to hardware misconfiguration. - [High] Data race on `flip_done` between the timeout handler and ISR leading to vblank refcount underflow. - [High] Unconditional dereference of `state->fb` triggers kernel panic for solid fill planes. - [High] When the cmdlist buffer overflows, `cl->nod_len` is incremented despite the row write being aborted, causing the hardware to read out-of-bounds DMA memory. - [High] The `cl_rdma` register buffer is never packed into the cmdlist and gets corrupted when shared across planes. - [High] Signed plane coordinates are directly cast to u32, causing massive out-of-bounds register programming when off-screen. - [Medium] The `INT_VSYNC_UPDATE` interrupt is missing from the `irq_list` mapping array, preventing its status from being cleared. --
--- Patch [9]: [PATCH 09/17] drm/spacemit: add Saturn DPU hardware backend --- commit 1cf09ad0cc7d13639bcbfc63e90d8f431590bfbd Author: Cody Kang <[email protected]> drm/spacemit: add Saturn DPU hardware backend Add the register-level programming of one Saturn instance: clock and reset bring-up, scene control and timing generator setup, RDMA channel and composer layer programming, FBC line-buffer budgeting, interrupt dispatch and the cfg-ready/vsync handshake that paces commits. The backend is exposed as an ops table and hardware descriptor to be aggregated by the KMS layer in an upcoming patch. > diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c > b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c > index 0000000000000..8d5f13bc5373c > --- /dev/null > +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c [ ... ] > +static int dpu_enable_clocks(struct spacemit_crtc *a_crtc) > +{ > + struct dpu_clk_context *clk_ctx = &a_crtc->clk_ctx; > + struct drm_crtc *crtc = &a_crtc->crtc; > + struct drm_display_mode *mode = &crtc->mode; > + u64 clk_val; > + u64 set_clk_val; > + int ret; > + > + ret = clk_prepare_enable(clk_ctx->pxclk); > + if (ret) > + goto err; > + set_clk_val = mode->clock * 1000; > + if (set_clk_val) { > + set_clk_val = clk_round_rate(clk_ctx->pxclk, set_clk_val); [Severity: High] Does this code properly handle negative error codes from clk_round_rate()? If clk_round_rate() fails and returns an error like -EINVAL, the unchecked value is cast directly to u64 set_clk_val. This will sign-extend the error code into a massive positive integer before it gets passed to clk_set_rate(), which can lead to hardware misconfiguration. > + clk_val = clk_get_rate(clk_ctx->pxclk); > + if (clk_val != set_clk_val) { > + clk_set_rate(clk_ctx->pxclk, set_clk_val); > + drm_dbg(crtc->dev, "pxclk=%lld\n", clk_val); > + } > + } [ ... ] > +static inline void dpu_isr_vblank(struct spacemit_crtc *a_crtc, bool *flip) > +{ > + struct drm_crtc *crtc = &a_crtc->crtc; > + struct drm_device *drm = crtc->dev; > + struct drm_pending_vblank_event *event; > + > + drm_crtc_handle_vblank(crtc); > + > + if (*flip) > + return; > + *flip = true; [Severity: High] Can this race condition cause a vblank reference underflow? The timer handler spacemit_cfg_rdy_timer_handler() sets flip_done to true and calls drm_crtc_vblank_put() asynchronously. If the ISR runs concurrently, both threads can observe flip_done as false due to the lockless TOCTOU check here. They will both set it to true and drop the vblank reference, putting the reference twice. > + > + /* > + * event_lock serialises this against the commit arming > + * crtc->state->event. > + */ > + spin_lock(&drm->event_lock); [ ... ] > +static u32 dpu_online_isr(struct spacemit_crtc *a_crtc) > +{ > + u32 irq_raw, irq_bit, irq_ur_bit; > + struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private; > + struct spacemit_hw_device *hwdev = priv->hwdev; > + int dev_id = a_crtc->dev_id; > + > + trace_dpu_isr(a_crtc->dev_id); [ ... ] > + irq_bit = hwdev->get_irq_bit(INT_REST, dev_id) | irq_ur_bit; > + if (irq_raw & irq_bit) > + hwdev->clr_int_sts(a_crtc, irq_bit, dev_id); > + > + irq_bit = hwdev->get_irq_bit(INT_VSYNC_UPDATE, dev_id); > + if (irq_raw & irq_bit) > + hwdev->clr_int_sts(a_crtc, irq_bit, dev_id); [Severity: Medium] Is it possible to clear the INT_VSYNC_UPDATE interrupt? The irq_list array in dpu_saturn_hee.c only defines up to INT_REST. When get_irq_bit() is called with INT_VSYNC_UPDATE (index 5), it triggers the ARRAY_SIZE(irq_list) check and returns 0. Consequently, the interrupt status can never be cleared by this handler. > + > + return 0; > +} [ ... ] > diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c > b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c > index 0000000000000..c23e4ca36d26b > --- /dev/null > +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c [ ... ] > +void saturn_hee_plane_update_hw_channel(struct drm_plane *plane) > +{ > + struct drm_plane_state *state = plane->state; > + struct spacemit_crtc *a_crtc = to_spacemit_crtc(state->crtc); > + struct drm_framebuffer *fb = plane->state->fb; [ ... ] > + src_w = state->src_w >> 16; > + src_h = state->src_h >> 16; > + src_x = state->src_x >> 16; > + src_y = state->src_y >> 16; > + > + crtc_w = state->crtc_w; > + crtc_h = state->crtc_h; > + crtc_x = state->crtc_x; > + crtc_y = state->crtc_y; [Severity: High] Can this cause massive out-of-bounds register programming if the plane is partially off-screen? The plane coordinates state->crtc_x and state->crtc_y are signed 32-bit integers. Directly assigning them to unsigned 32-bit variables will cause negative values to overflow into extremely large positive integers. When these are written to the hardware area control registers, it creates invalid constraints like area_left being greater than area_right. > + > + drm_dbg(plane->dev, "crtc_x %u crtc_y %u\n", crtc_x, crtc_y); > + > + if (rdma_id == RDMA_INVALID_ID) > + solid_en = true; > + > + trace_dpu_plane_info(state, fb, rdma_id, alpha, state->rotation); > + > + /* For solid color both src_w and src_h are 0 */ > + if (!solid_en) { > + base = RDMA_BASE_ADDR[rdma_id]; > + /* linear scanout only: the FBC decode path is not exposed */ > + dpu_write(hwdev, RDMA_PATH_X_REG, base, layer_mode, 0, cl_rdma); [Severity: High] Is the cl_rdma register buffer ever flushed to the command list? RDMA configurations are written to a_crtc->cl_rdma, but cmdlist_regs_packing() is never called for this buffer, meaning the configurations are discarded and never reach the hardware. Furthermore, does sharing a_crtc->cl_rdma across planes corrupt the state? When dpu_write() is called, it unconditionally overwrites cl_p->base. If multiple planes are updated concurrently, the last plane will overwrite the base address for all prior planes' registers in this shared buffer. > + dpu_write(hwdev, RDMA_PATH_X_REG, base, layer_cmpsr_id, > + a_crtc->dev_id, cl_rdma); [ ... ] > + } else { > + /* Constant-fill layer: zero-size source, transparent black. */ > + solid_r = 0; > + solid_g = 0; > + solid_b = 0; > + solid_a = 0; > + } > + > + switch (pixel_alpha) { [ ... ] > + } > + > + if (state->fb->format && state->fb->format->has_alpha && [Severity: High] Will this crash if the plane is a solid fill plane? In DRM, solid fill planes do not require an attached framebuffer, meaning state->fb can be NULL. Unconditionally dereferencing state->fb->format here without verifying if state->fb is valid will trigger a kernel panic. > + pixel_alpha != DRM_MODE_BLEND_PIXEL_NONE) { > + if (alpha != 0xff) > + alpha_sel = 0x2; > + else > + alpha_sel = 0x1; > + } else { > + blend_mode = 0x0; > + alpha_sel = 0x0; > + } > + > + cl_cmp = alloc_cmdlist_regs(CMPS_X_REG); [Severity: High] Since plane atomic update callbacks execute in atomic context during non-blocking commits, is it safe to allocate memory here? The alloc_cmdlist_regs() macro uses kzalloc() with GFP_KERNEL. Calling sleeping functions from an atomic context causes kernel warnings, system instability, and potential deadlocks. > + > + base = CMP_BASE_ADDR[a_crtc->dev_id]; > + dpu_write(hwdev, CMPS_X_REG, base, dst_w, mode->hdisplay, cl_cmp); [ ... ] > +void saturn_hee_cmdlist_fill_data_row(struct cmdlist *cl, u32 strobe, > + u32 offset, u32 value[]) > +{ > + struct cmdlist_row *row; > + u8 i; > + > + /* > + * packing counts rows blindly; drop the row rather than the buffer end > + */ > + if (WARN_ONCE(CL_HEADER_SZ + (cl->nod_len + 1) * CL_ROW_SZ > cl->size, > + "cmdlist buffer full (rows %u)\n", cl->nod_len)) > + return; [Severity: High] Does this correctly handle command list buffer overflows? When the cmdlist buffer is full, this function issues a warning and returns early without writing the row. However, its caller in cmdlist_regs_packing() ignores this failure and still increments cl->nod_len. This artificially inflates the length in the header. When the DPU hardware parses the cmdlist, it will attempt to read out-of-bounds DMA memory. > + > + row = (struct cmdlist_row *)((char *)cl->va + CL_HEADER_SZ) + > cl->nod_len; [ ... ] > diff --git a/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c > b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c > index 0000000000000..144716ec9b80c > --- /dev/null > +++ b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c [ ... ] > +int saturn_cal_layer_fbcmem_size(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + struct spacemit_plane_state *pstate = to_spacemit_plane_state(state); > + u32 drm_4cc_fmt = pstate->state.fb->format->format; [Severity: High] Will this crash if the plane is a solid fill plane? In DRM, solid fill planes do not require an attached framebuffer, meaning state->fb can be NULL. Unconditionally dereferencing state->fb->format here without verifying if state->fb is valid will trigger a kernel panic. > + u32 crop_w = pstate->state.src_w >> 16; > + > + /* Only linear sizing: the compressed path is not exposed. */ > + return get_raw_data_plane_rdma_mem_size(drm_4cc_fmt, crop_w, > + &pstate->fbcmem_size); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
