Re: [PATCH v2 04/14] drm/solomon: Add DRM driver for Solomon SSD16xx e-paper display controllers

[sashiko-bot@kernel.org]

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 <devarsht@ti.com>

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/20260927182329.4193961-1-devarsht@ti.com?part=4