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 <[email protected]> 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/[email protected]?part=1
