Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [Critical] drm/solomon: critical heap buffer overflow in format conversion - [High] drm/solomon: sleeping in atomic context during plane and crtc updates - [High] drm/solomon: redundant hardware reset and full redraw on every atomic commit - [High] drm/solomon: mapping local stack variables for SPI DMA transfers - [High] drm/solomon: illegal direct dereference of iosys_map vaddr - [High] drm/solomon: use-after-free of last_fb during crtc_atomic_flush - [Medium] drm/solomon: ssd16xx_convert_fb_to_3color ignores orientation causing scrambled output - [Medium] drm/solomon: DRM_SSD16XX selects DRM_GEM_SHMEM_HELPER without MMU dependency - [Medium] drm/solomon: scrambled image on reverse rotations due to missing intra-byte bit reversal - [Medium] drm/solomon: missing drm_dev_enter protection in plane_atomic_update - [Medium] drm/solomon: data race on device->init_refresh_pending flag - [Medium] drm/solomon: incorrect endianness conversion reading XRGB8888 pixels --
commit c0d6d51051f544c36f758eda4ee2441c54d494b8 Author: Devarsh Thakkar <[email protected]> drm/solomon: Add DRM driver for Solomon SSD16xx e-paper display controllers This commit introduces a new DRM driver for the Solomon Systech SSD16xx family of e-paper display controllers. It supports panels like the Gooddisplay GDEY042T81 and implements hardware features such as deep sleep modes, 4-way rotation, and single primary plane updates. > diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig > --- a/drivers/gpu/drm/solomon/Kconfig > +++ b/drivers/gpu/drm/solomon/Kconfig [ ... ] > @@ -30,3 +30,16 @@ config DRM_SSD130X_SPI > +config DRM_SSD16XX > + tristate "DRM support for Solomon SSD16xx e-ink display controllers" > + depends on DRM && SPI > + select DRM_CLIENT_SELECTION > + select DRM_KMS_HELPER > + select DRM_GEM_SHMEM_HELPER [Severity: Medium] Does selecting DRM_GEM_SHMEM_HELPER here without inheriting its required depends on MMU dependency cause Kconfig unmet direct dependency warnings and potential build failures on MMU-less architectures? > diff --git a/drivers/gpu/drm/solomon/ssd16xx.c > b/drivers/gpu/drm/solomon/ssd16xx.c > --- /dev/null > +++ b/drivers/gpu/drm/solomon/ssd16xx.c [ ... ] > +static void ssd16xx_wait_for_device(struct ssd16xx_device *device, > + int *err) > +{ [ ... ] > + while (gpiod_get_value_cansleep(device->busy) == 1) { > + if (time_after(jiffies, timeout_jiffies)) { > + drm_err(&device->drm, "Busy wait timed out after > %lums\n", > + jiffies_to_msecs(jiffies) - start_ms); > + *err = -ETIMEDOUT; > + return; > + } > + usleep_range(1000, 2000); [Severity: High] Can this sleep inside an atomic context? The DRM subsystem guidelines strictly dictate that plane_atomic_update and crtc_atomic_flush are atomic context paths where sleeping is forbidden. This driver calls usleep_range, spi_sync, and drm_gem_vmap (which takes a sleeping ww_mutex) from within these callbacks. Violating this invariant causes system instability, kernel warnings, and potential deadlocks when these functions run in non-preemptible contexts. [ ... ] > +static void ssd16xx_send_cmd(struct ssd16xx_device *device, u8 cmd, > + int *err) > +{ > + u16 word; > + struct spi_transfer xfer = {}; > + struct spi_message msg; > + > + if (*err) > + return; > + > + spi_message_init(&msg); > + spi_message_add_tail(&xfer, &msg); > + > + if (device->dc) { > + /* 4-wire SPI: D/C# GPIO low selects command mode */ > + xfer.tx_buf = &cmd; [Severity: High] Is it safe to assign pointers to local stack variables (like &cmd, &word, and &data) to the tx_buf of an spi_transfer? The SPI subsystem maps these buffers for DMA via spi_sync. Passing stack memory to the DMA API is explicitly forbidden; on architectures without coherent DMA caches, this causes cache-line sharing corruption and triggers DMA API debug panics. [ ... ] > +static u8 ssd16xx_pixel_luma(struct iosys_map *src, > + struct drm_framebuffer *fb, > + unsigned int x, unsigned int y) > +{ > + u32 *line = (u32 *)(src->vaddr + y * fb->pitches[0]); > + u32 px = line[x]; [Severity: High] Does this directly dereference an iosys_map structure bypassing the required abstraction API? iosys_map is explicitly designed to abstract memory that may reside in I/O space. Direct CPU dereference of an IOMEM pointer without iosys_map_rd or similar helpers will trigger exceptions and kernel panics on architectures that require specialized I/O accessors. [Severity: Medium] Will this native-endian u32 memory dereference convert endianness correctly on big-endian hardware? DRM formats like XRGB8888 are strictly little-endian byte arrays in memory. On big-endian CPUs, this native memory load will reverse the byte sequence, causing the subsequent bitwise shifts to extract incorrect colors. [ ... ] > +static void ssd16xx_convert_fb_to_3color(u8 *bw_dst, u8 *red_dst, > + struct iosys_map *src, > + struct drm_framebuffer *fb, > + struct drm_rect *rect) > +{ > + unsigned int x, y; > + u8 bw_byte = 0, red_byte = 0; > + unsigned int bit_pos = 0; > + unsigned int dst_idx = 0; [ ... ] > + /* XRGB8888 */ > + for (y = rect->y1; y < rect->y2; y++) { > + for (x = rect->x1; x < rect->x2; x++) { [Severity: Medium] Does this hardcoded row-major traversal ignore the device orientation? While the 1bpp monochrome conversion correctly alters pixel packing geometry based on display orientation, this function seems to ignore it completely. This can result in the RED RAM being populated in an incompatible geometric layout, rendering scrambled red ink for non-native orientations. [ ... ] > +static void ssd16xx_convert_fb_to_1bpp(u8 *dst, struct iosys_map *src, > + struct drm_framebuffer *fb, > + struct drm_rect *rect, > + unsigned int orientation) > +{ [ ... ] > + for (x = rect->x2 - 1; x >= (int)rect->x1; x--) { > + for (y = rect->y1; y < rect->y2; y++) { > + if (ssd16xx_pixel_is_white(src, fb, x, y)) > + byte |= (1 << (7 - bit_pos)); [Severity: Medium] Does this bit packing logic properly handle reverse rotations? To implement 90 and 180 degree rotations, the hardware is configured to decrement RAM addresses (XDEC_YDEC), but the hardware does not internally reverse the pixel-to-bit assignment within each 8-pixel byte. Packing pixels in a strictly forward bit sequence here means the first pixel from the reversed iteration ends up in bit 7 instead of bit 0, resulting in the 8 pixels of every byte being displayed in reverse order. [ ... ] > +static int ssd16xx_fb_dirty(struct drm_framebuffer *fb, struct drm_rect > *rect, > + struct ssd16xx_device *device, > + const struct iosys_map *src_map) > +{ > + const u8 *ctrl2_tbl = device->controller_cfg->ctrl2_refresh; > + struct iosys_map map = *src_map; > + int err = 0; > + unsigned int data_size = (device->width * device->height) / 8; > + u8 *mono_buffer = NULL; > + u8 *red_buffer = NULL; > + u16 ram_x_start, ram_x_end, ram_y_start, ram_y_end; > + > + /* > + * Process full display area. The rect for convert_fb_to_1bpp uses > + * the framebuffer (logical) dimensions - the pixel iteration walks > + * the source fb coordinate space. RAM window registers below use > + * the physical panel dimensions (device->width/height). > + */ > + rect->x1 = 0; > + rect->y1 = 0; > + rect->x2 = fb->width; > + rect->y2 = fb->height; [Severity: Critical] Can this cause a heap buffer overflow in the format conversion? The TX buffers are statically allocated to exactly fit the physical panel dimensions in ssd16xx_alloc_tx_bufs. By overriding the provided CRTC plane boundaries with the full, unconstrained framebuffer dimensions here, if userspace supplies a framebuffer larger than the CRTC, the conversion loop will write out of bounds. Additionally, at native resolutions using portrait orientations (like 300x400), the 37.5-byte rows are padded to 38 bytes per row (400 * 38 = 15,200 bytes), which overflows the 15,000 byte allocation. [ ... ] > +static void ssd16xx_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ [ ... ] > + saved_mode = device->refresh_mode; > + saved_border_waveform_idx = device->border_waveform_update_idx; > + if (device->init_refresh_pending) { > + device->refresh_mode = device->refresh_mode_init; > + device->border_waveform_update_idx = > device->border_waveform_init_idx; > + } [Severity: Medium] Is there a data race on the device->init_refresh_pending flag? The boolean flag is modified in ssd16xx_drm_master_set and ssd16xx_drm_master_drop under the protection of the DRM core's master_mutex. Simultaneously, it is read and modified here under the DRM modeset locks. Since these are separate lock classes, a concurrent atomic commit and SET_MASTER ioctl could trigger a data race. [ ... ] > + if (device->fast_lut_pending) { > + ret = ssd16xx_preload_fast_lut(device); > + if (ret) { > + drm_err(&device->drm, > + "plane_atomic_update: fast LUT preload failed: > %d\n", ret); > + } > + > + device->fast_lut_pending = false; > + } > + > + ret = ssd16xx_fb_dirty(fb, &rect, device, &shadow_state->data[0]); [Severity: Medium] Is this missing drm_dev_enter protection? Initiating hardware interactions (such as SPI writes and GPIO polls) without drm_dev_enter validation can lead to race conditions during teardown from the commit workqueue. If the SPI device is unbound, devres frees hardware resources, and omitting drm_dev_enter here could trigger a use-after-free on the SPI device or GPIOs. > + if (ret) > + drm_err(&device->drm, "plane_atomic_update: display update > failed: %d\n", ret); > + else > + device->last_fb = fb; [Severity: High] Does storing a raw pointer to the framebuffer without incrementing its reference count risk a use-after-free? When the plane is later disabled by userspace, last_fb can become a dangling pointer if the framebuffer is cleaned up by the core. A subsequent atomic commit on the CRTC will trigger ssd16xx_crtc_atomic_flush, which falls back to this dangling last_fb pointer. [ ... ] > +static void ssd16xx_crtc_atomic_flush(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ > + struct ssd16xx_device *device = crtc_to_ssd16xx_device(crtc); > + struct drm_framebuffer *fb; > + struct drm_rect full; > + int ret, idx; > + > + if (!device->initialized) > + return; > + > + if (!drm_dev_enter(&device->drm, &idx)) > + return; > + > + drm_dbg(&device->drm, "atomic_flush: reinit, orientation=%u°\n", > + device->orientation); > + > + ret = ssd16xx_hw_init(device); [Severity: High] Does this perform a redundant hardware reset and full redraw on every atomic commit? Because this is called by the DRM core on every atomic commit that affects the CRTC, its unconditional execution of ssd16xx_hw_init and ssd16xx_fb_dirty is highly destructive. plane_atomic_update already transmits the damaged regions; triggering a full hardware reset and complete framebuffer re-transmission here completely breaks damage tracking, causes display flicker, and doubles the transmission latency. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
