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
