Re: [PATCH RESEND 1/2] drm/tiny: sharp-memory: fix line address assignment on partial update

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Background kthread is not stopped on probe error paths, leading to a critical Use-After-Free.
- [High] Calling sleeping functions from a plane atomic_update callback violates DRM atomic context rules.
- [High] Line addresses are truncated to 8 bits, breaking hardware support for panels with more than 255 lines.
- [High] Missing IS_ERR check for kthread_run leads to a kernel panic during device removal.
- [High] Truncation in pitch calculation for non-byte-aligned widths causes missing hardware dummy bits.
--

commit c42f51bc1dfa5c55a57f8a64e3aa97d5ec5bc008
Author: Tobias Johansson <tobias.johansson@axis.com>
Subject: drm/tiny: sharp-memory: fix line address assignment on partial update

This patch modifies sharp_memory_set_tx_buffer_addresses() to only iterate
over the damaged line count and offset the assigned addresses by the clip
start. This fixes corrupted address-to-data mapping on partial updates where
incorrect line addresses were being sent to the panel.

> diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
> index cbf69460ebf32..595926ed660e5 100644
> --- a/drivers/gpu/drm/tiny/sharp-memory.c
> +++ b/drivers/gpu/drm/tiny/sharp-memory.c
> @@ -120,8 +120,8 @@ static inline void sharp_memory_set_tx_buffer_addresses(u8 *buffer,
>  							struct drm_rect clip,
>  							u32 pitch)
>  {
> -	for (u32 line = 0; line < clip.y2; ++line)
> -		buffer[line * pitch] = line + 1;
> +	for (u32 line = 0; line < drm_rect_height(&clip); ++line)
> +		buffer[line * pitch] = clip.y1 + line + 1;

[Severity: High]
This isn't a bug introduced by this patch, but does the computed line address
get truncated?

Since buffer is an 8-bit u8 array and SHARP_ADDR_PERIOD is hardcoded to
8 bits, addresses > 255 might overflow and be truncated. Could this break
hardware support for panels with more than 255 lines, like ls032b7dd02 which
can have up to 536 lines?

>  }

I also noticed some other pre-existing issues in this file:

[Severity: High]
This is a pre-existing issue, but does this violate DRM atomic context rules?

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_plane_atomic_update() {
    ...
	if (drm_atomic_helper_damage_merged(old_state, plane_state, &rect))
		sharp_memory_fb_dirty(plane_state->fb, shadow_plane_state->data,
				      &rect, &shadow_plane_state->fmtcnv_state);
    ...
}

sharp_memory_plane_atomic_update() executes in an atomic context where
sleeping is forbidden. However, it delegates to
sharp_memory_update_display() which acquires a mutex:

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_update_display() {
    ...
	mutex_lock(&smd->tx_mutex);
    ...
}

Additionally, it calls drm_fb_xrgb8888_to_mono() which allocates memory with
GFP_KERNEL:

drivers/gpu/drm/drm_format_helper.c:drm_fb_xrgb8888_to_mono() {
    ...
	src32 = drm_format_conv_state_reserve(state, len_src32 + linepixels,
					      GFP_KERNEL);
    ...
}

Could this lead to system instability, kernel warnings, and potential deadlocks
during non-blocking atomic commits?


[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free on the probe error
path if initialization fails?

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_probe() {
    ...
	smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
					  smd, "sw_vcom_signal");
    ...
	return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n");
    ...
}

If subsequent steps like sharp_memory_pipe_init() fail, the probe function
returns directly without stopping the sw_vcom_signal kthread. The devres
framework automatically frees the smd structure, but the still-running kthread
might dereference it to toggle vcom and update the display.


[Severity: High]
This is a pre-existing issue, but what happens if kthread_run() fails and
returns an ERR_PTR?

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_probe() {
    ...
	smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
					  smd, "sw_vcom_signal");
    ...
}

There doesn't appear to be an IS_ERR() check on the return value. If thread
creation fails due to memory limits, the ERR_PTR is stored in
smd->sw_vcom_signal.

When the device is later removed, it seems to unconditionally call
kthread_stop() on that unvalidated error pointer, which could cause a kernel
panic:

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_remove() {
    ...
	kthread_stop(smd->sw_vcom_signal);
    ...
}


[Severity: High]
This is a pre-existing issue, but does this pitch calculation truncate instead
of rounding up?

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_probe() {
    ...
	smd->pitch = (SHARP_ADDR_PERIOD + smd->mode->hdisplay + SHARP_DUMMY_PERIOD) / 8;
    ...
}

For non-byte-aligned widths (e.g., the ls018b7dh02 panel with hdisplay = 230),
this integer division evaluates to 246 / 8 = 30 bytes (240 bits).

However, the panel requires 230 bits of data, which drm_fb_xrgb8888_to_mono()
expands to 29 bytes. With 1 byte for the address and 29 bytes for data,
the 30 bytes of pitch are fully consumed. This appears to entirely omit
the required 8-bit dummy period.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-flickering-v1-0-eaf7bbf9c000@axis.com?part=1