Re: [PATCH 2/2] drm/solomon: Add SSD1351 OLED display driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling sleeping functions from within DRM atomic callbacks.
- [High] Passing stack-allocated variables and read-only data (.rodata) to `spi_write()`, violating the SPI subsystem's DMA-safe memory requirements.
- [High] The OLED display is not turned off when the driver module is unloaded due to `drm_dev_unplug()` preventing the `atomic_disable` hook from accessing hardware.
- [High] Coordinate space mismatch in the damage handling loop causes incorrect pixel extraction when panning.
--

commit 830ebdf4bc79cf5af84541c3dcfa33f751296ed5
Author: Amit Barzilai <amit.barzilai22@gmail.com>

drm/solomon: Add SSD1351 OLED display driver

Add a DRM/KMS driver for the Solomon SSD1351, a 128x128 65k-color RGB
OLED controller driven over a 4-wire SPI bus.

> diff --git a/drivers/gpu/drm/solomon/ssd1351.c b/drivers/gpu/drm/solomon/ssd1351.c
> new file mode 100644
> index 0000000000000..7c4a5c461afb8
> --- /dev/null
> +++ b/drivers/gpu/drm/solomon/ssd1351.c
[ ... ]
> +static int ssd1351_write_cmd(struct ssd1351_device *ssd1351, u8 cmd,
> +			     const u8 *params, size_t num)
> +{
> +	int ret;
> +
> +	gpiod_set_value_cansleep(ssd1351->dc, 0);
> +	ret = spi_write(ssd1351->spi, &cmd, 1);

[Severity: High]
Does this code pass a stack-allocated variable to spi_write()?

The SPI subsystem requires that buffers passed to spi_write() be DMA-safe.
Since cmd is a local variable on the stack, it may not be DMA-coherent
(especially if CONFIG_VMAP_STACK is enabled). Could this trigger cache
coherency failures or memory corruption when the SPI core attempts to map it?

> +	if (ret)
> +		return ret;
[ ... ]
> +/* Send a command followed by a fixed list of single-byte parameters. */
> +#define ssd1351_command(ssd1351, cmd, ...)                   \
> +	({                                                   \
> +		static const u8 _params[] = { __VA_ARGS__ }; \
> +		ssd1351_write_cmd((ssd1351), (cmd), _params, \
> +				  ARRAY_SIZE(_params));      \
> +	})

[Severity: High]
Does this macro pass read-only data (.rodata) to spi_write() via the
_params array?

Memory placed in .rodata is not DMA-safe. If the SPI subsystem attempts to map
this buffer for a DMA transfer, could this lead to kernel panics or undefined
behavior?

[ ... ]
> +static void ssd1351_reset(struct ssd1351_device *ssd1351)
> +{
> +	if (!ssd1351->reset)
> +		return;
> +
> +	/*
> +	 * Work in logical levels: 1 asserts reset, 0 releases it. The DT's
> +	 * GPIO_ACTIVE_LOW flag handles the physical inversion, so this pulse is
> +	 * correct regardless of how the board wires the RES# line.
> +	 */
> +	gpiod_set_value_cansleep(ssd1351->reset, 1);
> +	usleep_range(20, 1000);
> +	gpiod_set_value_cansleep(ssd1351->reset, 0);
> +	msleep(120);
> +}

[Severity: High]
Are we safely calling these sleeping functions?

This function is invoked via ssd1351_init_display() from the
ssd1351_crtc_atomic_enable() callback. According to DRM subsystem guidelines,
CRTC atomic callbacks execute in an atomic context where sleeping is strictly
forbidden. Calling msleep() and usleep_range() here could result in system
instability or deadlocks.

[ ... ]
> +static void ssd1351_fb_blit_rect(struct ssd1351_device *ssd1351,
> +				 struct drm_framebuffer *fb,
> +				 const struct iosys_map *vmap,
> +				 struct drm_rect *rect,
> +				 struct drm_format_conv_state *fmtcnv_state)
> +{
> +	unsigned int width = drm_rect_width(rect);
> +	unsigned int height = drm_rect_height(rect);
> +	unsigned int dst_pitch = width * sizeof(u16);
> +	struct iosys_map dst;
> +	u8 range[2];
> +
> +	iosys_map_set_vaddr(&dst, ssd1351->tx_buf);
> +
> +	/*
> +	 * The panel expects RGB565 most-significant byte first; the big-endian
> +	 * conversion produces exactly that byte stream for the 8-bit SPI words.
> +	 */
> +	drm_fb_xrgb8888_to_rgb565be(&dst, &dst_pitch, vmap, fb, rect,
> +				    fmtcnv_state);
> +
> +	range[0] = rect->x1;
> +	range[1] = rect->x2 - 1;
> +	if (ssd1351_write_cmd(ssd1351, SSD1351_SET_COL_ADDRESS, range,
> +			      sizeof(range)))

[Severity: High]
Similar to the issue in ssd1351_write_cmd(), does this pass the
stack-allocated range array down to spi_write()? This memory may not be
DMA-coherent.

[ ... ]
> +static void ssd1351_plane_atomic_update(struct drm_plane *plane,
> +					struct drm_atomic_state *state)
> +{
[ ... ]
> +	if (!drm_dev_enter(&ssd1351->drm, &idx))
> +		return;
> +
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		goto out_drm_dev_exit;
> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		dst_clip = plane_state->dst;
> +
> +		if (!drm_rect_intersect(&dst_clip, &damage))
> +			continue;
> +
> +		ssd1351_fb_blit_rect(ssd1351, fb, &shadow_plane_state->data[0],
> +				     &dst_clip,
> +				     &shadow_plane_state->fmtcnv_state);
> +	}

[Severity: High]
Does this code call sleeping functions from within an atomic context?

According to DRM subsystem guidelines, plane atomic update callbacks execute
in an atomic context during non-blocking commits, where sleeping is strictly
forbidden. However, drm_gem_fb_begin_cpu_access() can block on dma-buf fences,
and ssd1351_fb_blit_rect() performs blocking SPI writes. The standard DRM
pattern for SPI displays is to defer hardware updates to a worker thread.

[Severity: High]
Does this loop also pass destination coordinates to a function expecting source
coordinates?

The helper drm_atomic_for_each_plane_damage() yields damage clips in CRTC
destination coordinates. The driver intersects these with dst_clip and passes
the result to ssd1351_fb_blit_rect(), which then passes it to
drm_fb_xrgb8888_to_rgb565be().

However, drm_fb_xrgb8888_to_rgb565be() expects the clipping rectangle to be
in framebuffer (source) coordinates to accurately extract pixels. If the
framebuffer is panned (i.e., src_x or src_y are non-zero), using destination
coordinates could extract pixels from the wrong location.

[ ... ]
> +static void ssd1351_crtc_atomic_disable(struct drm_crtc *crtc,
> +					struct drm_atomic_state *state)
> +{
> +	struct ssd1351_device *ssd1351 = drm_to_ssd1351(crtc->dev);
> +	int idx;
> +
> +	if (!drm_dev_enter(crtc->dev, &idx))
> +		return;
> +
> +	ssd1351_command(ssd1351, SSD1351_SET_DISPLAY_OFF);
> +
> +	drm_dev_exit(idx);
> +}
[ ... ]
> +static void ssd1351_remove(struct spi_device *spi)
> +{
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	drm_dev_unplug(drm);
> +	drm_atomic_helper_shutdown(drm);
> +}

[Severity: High]
Could the OLED display remain powered on when the driver module is unloaded?

In ssd1351_remove(), drm_dev_unplug() is called before
drm_atomic_helper_shutdown(). The drm_dev_unplug() call immediately sets the
DRM device's unplugged flag.

When drm_atomic_helper_shutdown() subsequently triggers the
ssd1351_crtc_atomic_disable() callback, the drm_dev_enter() check in that
callback will fail because the device is already marked as unplugged. As a
result, the display-off command will never be sent to the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610164428.180091-1-amit.barzilai22@gmail.com?part=2