Re: [PATCH v5 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] DMA on stack memory via regmap_bulk_write in ssd130x_write_cmds.

Pre-existing issues:
- [High] drm_gem_fb_begin_cpu_access sleeps in atomic context.
- [High] regulator_enable sleeps in atomic context.
- [High] Backlight and DRM atomic commit unprotected SPI updates interleave.
--

--- Patch [6]: [PATCH v5 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support ---
commit c25fd0a18b10199afa7a08a23e60b6d45a463381
Author: Amit Barzilai <[email protected]>

drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

This commit adds support for the SSD1351 controller by introducing a new
SSD135X_FAMILY to the ssd130x driver. It adds a new init sequence, custom
command protocols that pass parameters on the data path, and family-specific
plane and encoder callbacks.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
[ ... ]
> @@ -271,6 +310,17 @@ static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
>  	unsigned int i;
>  	int ret;
>  
> +	if (ssd130x->device_info->cmd_params_are_data) {
> +		if (!len)
> +			return 0;
> +
> +		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[0]);
> +		if (ret || len == 1)
> +			return ret;
> +
> +		return ssd130x_write_data(ssd130x, cmd + 1, len - 1);
> +	}

[Severity: High]
Will this cause a DMA fault on stack memory? Callers like ssd130x_write_cmd()
allocate the command array on the kernel stack (e.g. u8 buf[8]). Passing
cmd + 1 to ssd130x_write_data() results in a regmap_bulk_write(). On SPI
devices, this passes the buffer directly to the SPI DMA engine. Because
vmalloc-backed kernel stacks forbid DMA, this might cause an immediate
kernel panic or fault during device probe and runtime updates.

[Severity: High]
This is a pre-existing issue, but does this split transaction allow
concurrent updates to interleave on the SPI bus? If a user-space write to
the sysfs backlight interface triggers ssd135x_update_bl() concurrently with
a display update from ssd135x_update_rect(), another thread's regmap_write()
could execute in between this regmap_write() and ssd130x_write_data(). This
could cause the controller to misinterpret parameters as opcodes or vice
versa.

[ ... ]
> @@ -1247,6 +1431,45 @@ static void ssd133x_primary_plane_atomic_update(struct drm_plane *plane,
>  	drm_dev_exit(idx);
>  }
>  
> +static void ssd135x_primary_plane_atomic_update(struct drm_plane *plane,
> +						struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		goto out_drm_dev_exit;

[Severity: High]
This isn't a bug introduced by this patch, but does this call to
drm_gem_fb_begin_cpu_access() sleep in an atomic context? Plane atomic
update callbacks run in atomic context during non-blocking commit tails.
Since drm_gem_fb_begin_cpu_access() can sleep while waiting on dma-buf
fences or acquiring locks, this might cause kernel warnings or deadlocks
during screen updates.

[ ... ]
> @@ -1629,6 +1887,31 @@ static void ssd133x_encoder_atomic_enable(struct drm_encoder *encoder,
>  	ssd130x_power_off(ssd130x);
>  }
>  
> +static void ssd135x_encoder_atomic_enable(struct drm_encoder *encoder,
> +					  struct drm_atomic_commit *state)
> +{
> +	struct drm_device *drm = encoder->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	int ret;
> +
> +	ret = ssd130x_power_on(ssd130x);

[Severity: High]
This is a pre-existing issue, but can this power management sequence sleep
in an atomic context? Encoder atomic enable callbacks must not sleep, yet
ssd130x_power_on() calls regulator_enable() which uses mutexes internally.
Could this lead to deadlocks or crashes when the display is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6