Re: [PATCH] drm/trigger5: Add MCT Trigger 5 USB display driver

[sashiko-bot@kernel.org]

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 <hjf3108@gmail.com>
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(&current_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(&current_transfer->frame_complete);
> +		goto exit_save_pending;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910171709.396071-1-hjf3108@gmail.com?part=1