Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] drm/tiny: atomic context violation by calling sleeping functions - [High] spi: DMA-unsafe stack buffers passed to spi_sync - [High] drm: missing reset callback in drm_crtc_funcs leads to null deref - [High] drm: missing cleanup_fb causes GEM object pin leaks - [Medium] spi: massive contiguous allocation and unbounded spi transfers - [Medium] drm: missing atomic_disable hook causes intentional warnings - [Medium] spi: missing cs_change=1 breaks RA8875 spi protocol --
commit 594ac08dfc437ba3494bf3b5226a6268a7e028be Author: Adam Azuddin <[email protected]> drm/tiny: Add RAiO RA8875 display controller driver Add a DRM tiny driver for the RAiO RA8875 SPI-connected TFT display controller. The driver supports display resolutions up to 800x480 and uses the GEM shmem helper for buffer management. > diff --git a/drivers/gpu/drm/tiny/ra8875.c b/drivers/gpu/drm/tiny/ra8875.c > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/ra8875.c [ ... ] > +static int ra8875_read_reg(struct ra8875_device *ra8875, u8 reg, u8 *val) > +{ > + ra8875->cmd_buf[0] = RA8875_CMDWRITE; > + ra8875->cmd_buf[1] = reg; > + int ret; > + > + u8 tx[2] = { 0x40, 0x00 }; > + u8 rx[2] = { 0x00, 0x00 }; > + > + struct spi_transfer t_cmd = { > + .tx_buf = ra8875->cmd_buf, > + .len = 2, > + .speed_hz = RA8875_REG_SPI_SPEED, > + .cs_change = 1 > + }; > + struct spi_transfer t_read = { > + .tx_buf = tx, > + .rx_buf = rx, > + .len = 2, > + .speed_hz = RA8875_REG_SPI_SPEED, > + }; > + struct spi_message m; > + > + spi_message_init(&m); > + spi_message_add_tail(&t_cmd, &m); > + spi_message_add_tail(&t_read, &m); > + > + ret = spi_sync(ra8875->spi, &m); [Severity: High] Does this code pass stack memory into a DMA-mapped SPI transfer? The Linux SPI subsystem uses the DMA API to map transfer buffers if the SPI controller supports it. Stack memory allocated for tx and rx might not be physically contiguous, leading to memory corruption or cache coherency failures. > + if (ret) > + return ret; [ ... ] > +static int ra8875_hw_init(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ > + u8 hdwr, hndr, hstr, hpwr, vpwr; > + u16 vdhr, vndr, vstr; > + u8 readback; > + int ret; > + > + if (ra8875->rst_gpio) { > + gpiod_set_value_cansleep(ra8875->rst_gpio, 1); > + usleep_range(1000, 1200); > + gpiod_set_value_cansleep(ra8875->rst_gpio, 0); > + msleep(RA8875_RESET_WAIT_MS); > + } [Severity: High] Does this execute sleeping functions in an atomic context? The global review guidelines dictate that CRTC and plane atomic update callbacks execute in an atomic context during non-blocking commits, where hardware sequence paths must not sleep. Since ra8875_hw_init() is called by ra8875_crtc_atomic_enable(), executing msleep(), usleep_range(), and gpiod_set_value_cansleep() here can lead to system instability, lockups, and kernel warnings for scheduling while atomic. > + > + /* Soft Reset Sequence */ > + ret = ra8875_write_reg(ra8875, RA8875_PWRR, RA8875_PWRR_SOFTRST); [ ... ] > +static int ra8875_clear_screen(struct ra8875_device *ra8875, > + struct drm_display_mode *mode) > +{ [ ... ] > + for (y = 0; y < height;) { > + rows = min(rows_per_chunk, height - y); > + > + u8 cmd[2] = { RA8875_CMDWRITE, RA8875_MRWC }; > + struct spi_transfer t_cmd = { > + .tx_buf = cmd, > + .len = 2, > + .speed_hz = RA8875_REG_SPI_SPEED, > + }; [Severity: Medium] Will the missing cs_change = 1 break the RA8875 SPI protocol here? Unlike ra8875_write_reg() which sets cs_change = 1 for command transfers to de-assert the chip select before sending the data payload, this transfer omits it. Without toggling the CS pin between the command and data phases, the hardware might misinterpret the incoming data. [Severity: High] Does this code use stack memory for an SPI transfer buffer? Passing the local stack array cmd into spi_sync() is unsafe because the SPI subsystem may use DMA to map the buffer, which is explicitly prohibited for stack variables. > + struct spi_transfer t_data = { > + .tx_buf = txbuf, > + .len = 1 + rows * width * 2, > + .speed_hz = RA8875_MAX_SPI_SPEED, > + }; > + struct spi_message m; [ ... ] > +static void ra8875_fb_dirty(struct ra8875_device *ra8875, struct iosys_map > *src, > + struct drm_framebuffer *fb, struct drm_rect *rect) > +{ [ ... ] > + memset(&t_data, 0, sizeof(t_data)); > + t_data.tx_buf = ra8875->txbuf; > + t_data.len = 1 + height * width * 2; > + t_data.speed_hz = ra8875->spi->max_speed_hz; [Severity: Medium] Will this result in unbounded SPI transfers that exceed the host controller's limits? The driver attempts to send up to the entire 768 KB buffer (for an 800x480 resolution) in a single spi_transfer depending on the damage rect size. Many SPI controllers impose maximum transfer limits, such as 65535 bytes, and will reject oversized transfers. > + > + spi_message_init(&m); > + spi_message_add_tail(&t_data, &m); > + if (spi_sync(ra8875->spi, &m)) [Severity: High] Does this call a sleeping function from the plane's atomic update callback? spi_sync() sleeps while waiting for hardware I/O to complete. Because ra8875_fb_dirty() is invoked from ra8875_plane_atomic_update(), this executes sleeping routines in an atomic context, leading to kernel warnings and system lockups. > + goto err; > + > + drm_format_conv_state_release(&fmtcnv_state); [ ... ] > +static void ra8875_plane_atomic_update(struct drm_plane *plane, struct > drm_atomic_commit *state) > +{ > + struct ra8875_device *ra8875 = container_of(plane->dev, struct > ra8875_device, drm); > + struct drm_plane_state *old_plane_state = > drm_atomic_get_old_plane_state(state, plane); > + struct drm_plane_state *plane_state = > drm_atomic_get_new_plane_state(state, plane); > + struct drm_shadow_plane_state *shadow_plane_state = > to_drm_shadow_plane_state(plane_state); > + struct drm_framebuffer *fb = plane_state->fb; > + struct drm_rect rect; > + struct iosys_map *map = &shadow_plane_state->data[0]; > + int idx; > + > + if (drm_WARN_ON_ONCE(&ra8875->drm, !fb)) > + return; [Severity: Medium] Is this code triggering intentional warnings during plane disables? Because the driver does not provide an atomic_disable callback in ra8875_plane_helper_funcs, the DRM atomic helpers fall back to calling atomic_update with a NULL fb. This warning will immediately fire on valid plane disable requests, generating a stack trace that could crash the system if panic_on_warn is enabled. > + > + if (!drm_dev_enter(&ra8875->drm, &idx)) [ ... ] > +static const struct drm_crtc_funcs ra8875_crtc_funcs = { > + .atomic_create_state = drm_atomic_helper_crtc_create_state, [Severity: High] Does this structure omit the required .reset callback and mistakenly assign to .atomic_create_state instead of .atomic_duplicate_state (or similar)? There is no .atomic_create_state member in drm_crtc_funcs, causing a build failure. Furthermore, omitting the required .reset callback (typically drm_atomic_helper_crtc_reset) leaves crtc->state as NULL, leading to a kernel panic upon the first atomic check or commit. > + .destroy = drm_crtc_cleanup, > + .set_config = drm_atomic_helper_set_config, > + .page_flip = drm_atomic_helper_page_flip, > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > +}; > + > +static const struct drm_crtc_helper_funcs ra8875_crtc_helper_funcs = { > + .mode_valid = ra8875_mode_valid, > + .atomic_enable = ra8875_crtc_atomic_enable, > + .atomic_disable = ra8875_crtc_atomic_disable, > +}; > + > +static const struct drm_plane_funcs ra8875_plane_funcs = { > + .update_plane = drm_atomic_helper_update_plane, > + .disable_plane = drm_atomic_helper_disable_plane, > + .destroy = drm_plane_cleanup, > + DRM_GEM_SHADOW_PLANE_FUNCS, > +}; > + > +static const struct drm_plane_helper_funcs ra8875_plane_helper_funcs = { > + .prepare_fb = drm_gem_plane_helper_prepare_fb, > + .begin_fb_access = drm_gem_begin_shadow_fb_access, > + .end_fb_access = drm_gem_end_shadow_fb_access, > + .atomic_check = ra8875_plane_atomic_check, > + .atomic_update = ra8875_plane_atomic_update, > +}; [Severity: High] Does this code leak GEM object pins by missing a cleanup_fb hook? The driver specifies .prepare_fb = drm_gem_plane_helper_prepare_fb, which pins the framebuffer during a page flip or mode set. Without providing a matching .cleanup_fb hook (such as drm_gem_plane_helper_cleanup_fb), the framebuffer is never unpinned. The GEM object reference count will perpetually increase, and the memory cannot be freed. > + > +static const struct drm_driver ra8875_driver = { [ ... ] > +static int ra8875_probe(struct spi_device *spi) > +{ [ ... ] > + ret = spi_setup(spi); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to setup SPI:\n"); > + > + buf_size = 1 + RA8875_MAX_HEIGHT * RA8875_MAX_WIDTH * 2; > + ra8875->txbuf = devm_kzalloc(&spi->dev, buf_size, GFP_KERNEL); [Severity: Medium] Will this fail to allocate memory on fragmented systems? The driver allocates ~768 KB of memory for a maximum 800x480 resolution using devm_kzalloc. Because this requires an order-8 contiguous physical allocation, it is highly likely to fail on systems with memory fragmentation. > + if (!ra8875->txbuf) > + return -ENOMEM; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
