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

[sashiko-bot@kernel.org]

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 <hjf3108@gmail.com>

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(&current_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(&current_transfer->frame_complete);
>  		goto exit_save_pending;
>  	}
>  
>  	drm_fb_xrgb8888_to_rgb888(&data_map, NULL,
>  				  &shadow_plane_state->data[0],
>  				  state->fb, &current_rect,
>  				  &shadow_plane_state->fmtcnv_state);
>  
>  	drm_gem_fb_end_cpu_access(state->fb, DMA_FROM_DEVICE);
>  
>  	queue_work(trigger5->transfer_wq, &current_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/20260914131924.196251-1-hjf3108@gmail.com?part=1