Re: [PATCH v2 05/14] drm/solomon: ssd16xx: Add clear_on_init/close/disable session management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm: sleeping in atomic context during CRTC enable/disable
- [High] drm/solomon: unsynchronized clear in master_drop races with atomic commits
- [High] drm/solomon: clear_display hardcodes RAM address counters to 0x00
- [Medium] drm/solomon: clear_display writes 0xFF to RED RAM causing red screen
- [Medium] drm/solomon: display_cleared_on_deinit flag is not reset on enable
- [Medium] drm/solomon: clear_on_init bypasses active CRTCs during master handover
--

commit 90ac1ffc13e17b560cc95b731a7e77c4602e7b77
Author: Devarsh Thakkar <devarsht@ti.com>

drm/solomon: ssd16xx: Add clear_on_init/close/disable session management

E-paper displays are bistable: the last rendered image persists indefinitely
across sessions and power cycles. This creates a session management choice
with no LCD/OLED equivalent since unlike volatile displays, content
visibility outlives the rendering process. Add session-lifetime display
clearing with three configurable hooks: clear_on_init, clear_on_close, and
clear_on_disable.

> diff --git a/drivers/gpu/drm/solomon/ssd16xx.c b/drivers/gpu/drm/solomon/ssd16xx.c
> index d3af055c67398..c478309d08e9f 100644
> --- a/drivers/gpu/drm/solomon/ssd16xx.c
> +++ b/drivers/gpu/drm/solomon/ssd16xx.c
[ ... ]
> @@ -746,6 +766,111 @@ static int ssd16xx_hw_init(struct ssd16xx_device *device)
>  	return err;
>  }
>  
> +/*
> + * Clear display by writing all-white to both BW and RED RAM.
> + * The ctrl2 argument selects the waveform (full/partial/fast refresh).
> + * Border waveform is set to init value before clearing, then restored
> + * to the update value to preserve the border during subsequent updates.
> + */
> +static int ssd16xx_clear_display(struct ssd16xx_device *device, u8 ctrl2)
> +{
> +	const u8 *bw_tbl = device->controller_cfg->border_waveform_table;
> +	int err = 0;
> +	unsigned int data_size = (device->width * device->height) / 8;
> +	u8 *white_buffer = device->tx_buf;
> +
> +	memset(white_buffer, 0xFF, data_size);

[Severity: High]
Can this unsynchronized memset cause a data race? 

When a DRM master drops, ssd16xx_drm_master_drop calls
ssd16xx_clear_display_on_exit. If an atomic commit is concurrently executing
in a background worker, both paths will mutate device->tx_buf without locks,
which could interleave SPI commands and corrupt the controller state.

> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_SET_RAM_X_ADDRESS_COUNTER, &err);
> +	ssd16xx_send_x_param(device, 0x00, &err);
> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_SET_RAM_Y_ADDRESS_COUNTER, &err);
> +	ssd16xx_send_y_param(device, 0x00, &err);

[Severity: High]
Does hardcoding the RAM X and Y address counters to 0x00 break decrement-mode
writes and omit window setup? 

In portrait orientation (decrement entry modes), starting at 0x00 might
immediately underflow the counter and write out-of-bounds. 

Also, if the previous operation was a partial update, could the RAM window
remain restricted, causing the clear operation to wrap around and fail to
clear the full screen?

> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_WRITE_RAM_BW, &err);
> +	ssd16xx_send_data_bulk(device, white_buffer, data_size, &err);
> +
> +	ssd16xx_send_cmd(device, SSD1683_CMD_WRITE_RAM_RED, &err);
> +	ssd16xx_send_data_bulk(device, white_buffer, data_size, &err);

[Severity: Medium]
Does this write 0xFF to the RED RAM unconditionally? 

On 3-color (BWR) panels where a 1 bit represents red ink, this would clear
the screen to solid red instead of white. On 2-color panels, it sends an
unsupported command (0x26), which could corrupt hardware state.

[ ... ]
> @@ -1356,11 +1481,26 @@ static void ssd16xx_crtc_atomic_disable(struct drm_crtc *crtc,
>  					struct drm_atomic_commit *state)
>  {
>  	struct ssd16xx_device *device = crtc_to_ssd16xx_device(crtc);
> -	int idx;
> +	int ret, idx;
>  
>  	if (!drm_dev_enter(&device->drm, &idx))
>  		return;
>  
> +	if (device->clear_on_disable < 0 || device->display_cleared_on_deinit)
> +		goto out;
> +
> +	drm_dbg(&device->drm, "clear_on_disable: running, mode=%d\n",
> +		device->clear_on_disable);
> +	ret = ssd16xx_clear_display(device,
> +				    ssd16xx_refresh_mode_to_ctrl2(device,
> +								  device->clear_on_disable));

[Severity: High]
Can this cause a sleep in atomic context? 

The CRTC atomic_disable callback executes in atomic context where sleeping is
forbidden, but it conditionally calls ssd16xx_clear_display, which relies on
SPI transactions and polling with usleep_range in ssd16xx_wait_for_device.

> +	if (ret) {
> +		drm_err(&device->drm, "atomic_disable: clear failed: %d\n", ret);
> +		goto out;
> +	}
> +
> +	device->display_cleared_on_deinit = true;

[Severity: Medium]
Is the display_cleared_on_deinit flag missing a reset on enable? 

DPMS off sets the flag to true, but since atomic_enable does not reset it,
subsequent DPMS toggles during a single session might silently skip the clear
operation.

> +out:
>  	drm_dev_exit(idx);
>  }
>  
> @@ -1383,6 +1523,11 @@ static void ssd16xx_crtc_atomic_enable(struct drm_crtc *crtc,
>  	}
>  	device->initialized = true;
>  
> +	/* Clear display on first app launch if configured */
> +	ret = ssd16xx_clear_display_on_init(device);

[Severity: High]
Can this cause a sleep in atomic context? 

Similar to atomic_disable, the CRTC atomic_enable callback executes in atomic
context where sleeping is forbidden, but calling ssd16xx_clear_display_on_init
leads to ssd16xx_clear_display which eventually polls with usleep_range.

[Severity: Medium]
Does this bypass active CRTCs during master handover? 

The clear_on_init hook is inside atomic_enable, which is only called on
transitions from disabled to enabled. If a new DRM master takes over without
triggering a full modeset (e.g. the CRTC remains active), the clean-slate
clear might not fire.

> +	if (ret)
> +		drm_err(&device->drm, "crtc_atomic_enable: clear on init failed: %d\n", ret);
> +
>  	/*
>  	 * If refresh_mode_init is set, arm init_refresh_pending so
>  	 * plane_atomic_update uses the specified mode for the first frame

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927182329.4193961-1-devarsht@ti.com?part=5