Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Sleeping functions and blocking memory allocations are called within DRM atomic commit callbacks, which are strictly defined as atomic context paths by subsystem guidelines. - [High] trigger5_crtc_create_state() returns ERR_PTR(-ENOMEM) instead of NULL on allocation failure, causing a crash in the DRM core. - [High] Missing cache flush for the vmalloc DMA buffer before initiating USB transfer. - [Medium] The USB bulk header's horizontal and vertical offsets are populated with raw framebuffer coordinates instead of screen coordinates. --
commit e87cbb95a349285fc94e00546c2aa86f1ff4d509 Author: Ho Jie Feng <[email protected]> drm/trigger5: Add MCT Trigger 5 USB display driver Add a DRM driver for MCT Trigger 5 USB display adapters. The driver implements modesetting and USB framebuffer updates, connector detection, and EDID retrieval. > diff --git a/drivers/gpu/drm/trigger5/trigger5_drv.c > b/drivers/gpu/drm/trigger5/trigger5_drv.c > --- a/drivers/gpu/drm/trigger5/trigger5_drv.c > +++ b/drivers/gpu/drm/trigger5/trigger5_drv.c > @@ -248,8 +248,9 @@ static void trigger5_atomic_commit_tail(struct > drm_atomic_commit *state) > > if (!drm_dev_enter(state->dev, &idx)) > goto commit; > > trigger5_stop_io(trigger5); [Severity: High] Is it safe to block in this atomic commit tail callback? The DRM subsystem guidelines explicitly designate drm_atomic_helper_commit_tail() as an atomic context path where sleeping or blocking operations (like flushing workqueues inside trigger5_stop_io()) are strictly forbidden, which could lead to deadlocks or kernel warnings. [ ... ] > @@ -282,10 +282,10 @@ static int trigger5_alloc_bulk_buffer(struct > trigger5_transfer_buf *buf, > > /* Large transfer buffer requires vmalloc and a scatterlist. */ > data = vmalloc_32(len); > if (!data) > return -ENOMEM; > > num_pages = DIV_ROUND_UP(len, PAGE_SIZE); > pages = kmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL); > if (!pages) { > ret = -ENOMEM; > goto err_vfree; > } > for (i = 0, ptr = data; i < num_pages; i++, ptr += PAGE_SIZE) > pages[i] = vmalloc_to_page(ptr); > ret = sg_alloc_table_from_pages(&buf->sgt, pages, > num_pages, 0, len, GFP_KERNEL); [Severity: High] Since this DMA buffer is allocated with vmalloc_32() and mapped to an SG table, will the CPU cache for the vmalloc alias need to be explicitly flushed after writes? [ ... ] > @@ -324,10 +324,10 @@ static struct drm_crtc_state * > trigger5_crtc_create_state(struct drm_crtc *crtc) > { > struct trigger5_crtc_state *tstate = kzalloc_obj(*tstate); > > if (!tstate) > return ERR_PTR(-ENOMEM); [Severity: High] Could this cause a crash in the DRM core? The DRM API contract requires atomic_create_state callbacks to return NULL on memory allocation failure. Returning ERR_PTR(-ENOMEM) creates a non-NULL pointer that could evade existing checks, leading to a dereference of the error pointer. [ ... ] > @@ -377,10 +377,10 @@ static void trigger5_crtc_atomic_enable(struct drm_crtc > *crtc, > trigger5_stop_io(trigger5); > > /* Sequence recovered from USB captures. */ > ret = usb_control_msg_recv(udev, 0, > TRIGGER5_REQUEST_FIRMWARE_RESET, > USB_DIR_IN | USB_TYPE_VENDOR | > USB_RECIP_DEVICE, > 0x0000, 0x0000, data, 1, > USB_CTRL_GET_TIMEOUT, GFP_KERNEL); [Severity: High] Could calling synchronous USB control message APIs with GFP_KERNEL here cause system instability? The CRTC atomic enable callback runs in an atomic context path where sleeping is strictly forbidden. [ ... ] > @@ -456,10 +456,10 @@ static void trigger5_crtc_atomic_disable(struct > drm_crtc *crtc, > trigger5_stop_io(trigger5); > > ret = usb_control_msg_recv(udev, 0, > TRIGGER5_REQUEST_FIRMWARE_RESET, > USB_DIR_IN | USB_TYPE_VENDOR | > USB_RECIP_DEVICE, > 0x0001, 0x0000, &data, 1, > USB_CTRL_GET_TIMEOUT, GFP_KERNEL); [Severity: High] Could this sleep in atomic context? Similar to the enable callback, this disable callback runs in a path where sleeping is forbidden. [ ... ] > @@ -635,10 +635,10 @@ static void trigger5_plane_atomic_update(struct > drm_plane *plane, > /* > * This should almost never wait because we have should have a > * pending transfer ready to be de-queued above in case the transfer > * hasn't finished, but do a bounded wait just in case it gets stuck > */ > if (!wait_for_completion_timeout(¤t_transfer->frame_complete, > msecs_to_jiffies(20))) > goto exit_save_pending; [Severity: High] Is it safe to wait on a completion here? wait_for_completion_timeout() can sleep, but plane atomic update callbacks run in an atomic context. > current_transfer->transfer_rect = current_rect; > > current_transfer->frame_len = frame_len; > header = current_transfer->buf.data; > header->magic = 0xfb; > header->length = 0x14; > /* flags 0: uncompressed 24-bit RGB888. */ > header->counter = > cpu_to_le16((trigger5->frame_counter++) & 0xfff); > header->horizontal_offset = cpu_to_le16(current_rect.x1); > header->vertical_offset = cpu_to_le16(current_rect.y1); [Severity: Medium] Does this correctly populate the USB bulk header coordinates? If the display is panned (i.e. has a non-zero source offset via state->src_x or state->src_y), the hardware typically needs to receive screen coordinates rather than raw framebuffer coordinates. Since current_rect.x1 and current_rect.y1 are in the coordinate space of the framebuffer, could this result in visual corruption when panning? > header->width = cpu_to_le16(width); > header->height = cpu_to_le16(height); > header->payload_length = cpu_to_le32((u32)payload_len); > header->flags = 0x1; > header->unknown1 = 0; > header->unknown2 = 0; > header->checksum = trigger5_bulk_header_checksum(header); > > iosys_map_set_vaddr(&data_map, > current_transfer->buf.data + sizeof(*header)); > > ret = drm_gem_fb_begin_cpu_access(state->fb, DMA_FROM_DEVICE); [Severity: High] Could drm_gem_fb_begin_cpu_access() sleep here? This is also called in the atomic context path. > if (ret < 0) { > complete(¤t_transfer->frame_complete); > goto exit_save_pending; > } > > drm_fb_xrgb8888_to_rgb888(&data_map, NULL, > &shadow_plane_state->data[0], > state->fb, ¤t_rect, > &shadow_plane_state->fmtcnv_state); > > drm_gem_fb_end_cpu_access(state->fb, DMA_FROM_DEVICE); > > queue_work(trigger5->transfer_wq, ¤t_transfer->transfer_work); [Severity: High] Does the vmap cache need to be flushed before queueing the transfer work? The DMA API manages cache coherency for the direct physical map, but not the vmalloc alias used when the CPU writes the pixel data. Should flush_kernel_vmap_range() be called here to avoid silent data corruption on architectures with non-coherent DMA and virtually indexed caches? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
