Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Sleeping functions and blocking waits are called from atomic DRM callbacks (plane update, CRTC enable/disable), violating KMS atomic context rules. --
commit d1b842a91c5fa632149bc045bac81f4636faed1b Author: Ho Jie Feng <[email protected]> drm/trigger5: Add MCT Trigger 5 USB display driver This commit adds a DRM driver for MCT Trigger 5 USB display adapters. It implements modesetting, 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 > --- /dev/null > +++ b/drivers/gpu/drm/trigger5/trigger5_drv.c [ ... ] > +static void trigger5_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ [ ... ] > + if (!drm_dev_enter(crtc->dev, &idx)) > + return; > + > + trigger5_stop_io(trigger5); [Severity: High] Can this cause a "scheduling while atomic" bug? The CRTC atomic enable callback runs in atomic context, but trigger5_stop_io() calls flush_workqueue() and cancel_delayed_work_sync(), both of which can sleep. > + > + /* 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] Does this synchronous USB transfer sleep? usb_control_msg_recv() allocates with GFP_KERNEL and blocks for the transfer. Running this in the CRTC atomic enable path violates the non-blocking requirement of atomic context. [ ... ] > +static void trigger5_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ [ ... ] > + 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] Similarly, could this cause a deadlock or crash? The CRTC atomic disable callback runs in atomic context, but this executes a blocking USB transfer and uses GFP_KERNEL allocations. [ ... ] > +static void trigger5_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *atomic_state) > +{ [ ... ] > + /* > + * 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))) [Severity: High] Is it safe to perform a blocking wait directly inside the plane's atomic update callback? Plane atomic updates run in atomic context and must not sleep. [ ... ] > + /* Resize buffer to the current resolution for lower memory footprint */ > + max_len = array3_size(drm_rect_width(&src_rect), > + drm_rect_height(&src_rect), 3); > + max_len = size_add(max_len, sizeof(*header)); > + > + /* > + * Allocation failure leaves the old buffer available for smaller > + * partial updates. > + */ > + if (max_len != current_transfer->frame_alloc_len) > + trigger5_resize_bulk_buffer(current_transfer, max_len); [Severity: High] Does this invoke memory allocations from an atomic context? trigger5_resize_bulk_buffer() calls vmalloc_32() and kmalloc_array() with GFP_KERNEL, which will sleep and lead to system instability in the atomic update path. [ ... ] > + iosys_map_set_vaddr(&data_map, > + current_transfer->frame_data + sizeof(*header)); > + > + ret = drm_gem_fb_begin_cpu_access(state->fb, DMA_FROM_DEVICE); [Severity: High] Can this attempt to take dma_resv locks and wait for fences in atomic context? Also, is this call redundant here since the DRM_GEM_SHADOW_PLANE_HELPER_FUNCS prepare_fb callback already maps it asynchronously? > + if (ret < 0) { > + complete(¤t_transfer->frame_complete); > + goto exit_save_pending; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
