Re: [PATCH v3] 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] drm/trigger5: sleeping functions called from atomic context in commit callbacks
--

commit 4bfd327671381f9e3b72403f14f247b30db4659a
Author: Ho Jie Feng <hjf3108@gmail.com>
Subject: drm/trigger5: Add MCT Trigger 5 USB display driver

This commit introduces a DRM driver for MCT Trigger 5 USB display adapters. 
It implements modesetting, USB framebuffer updates, connector detection, 
and EDID retrieval, utilizing reverse-engineered USB protocol structures.

> 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_atomic_commit_tail(struct drm_atomic_commit *state)
> +{
> +	struct trigger5_device *trigger5 = to_trigger5(state->dev);
> +	struct drm_crtc_state *crtc_state;
> +	struct trigger5_crtc_state *tstate;
> +	int idx, i;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, &trigger5->crtc);
> +	if (!crtc_state)
> +		goto commit;
> +
> +	tstate = to_trigger5_crtc_state(crtc_state);
> +	if (!tstate->bufs[0].data)
> +		goto commit;
> +
> +	if (!drm_dev_enter(state->dev, &idx))
> +		goto commit;
> +
> +	trigger5_stop_io(trigger5);

[Severity: High]
Is it safe to call trigger5_stop_io() from trigger5_atomic_commit_tail()? 
trigger5_stop_io() invokes flush_workqueue() and cancel_delayed_work_sync(),
both of which sleep. Calling sleeping functions from atomic commit paths can 
lead to system deadlocks and "sleeping function called from invalid context" 
kernel panics.

[ ... ]

> +static void trigger5_crtc_atomic_enable(struct drm_crtc *crtc,
> +					struct drm_atomic_commit *state)
> +{
> +	struct trigger5_device *trigger5 = to_trigger5(crtc->dev);
> +	struct usb_device *udev = interface_to_usbdev(trigger5->intf);
> +	struct drm_crtc_state *crtc_state =
> +		drm_atomic_get_new_crtc_state(state, crtc);
> +	struct drm_display_mode *mode = &crtc_state->mode;
> +	struct trigger5_mode_request request = {};
> +	u8 data[4];
> +	u64 clk;
> +	int idx, ret;
> +
> +	if (!drm_dev_enter(crtc->dev, &idx))
> +		return;
> +
> +	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 this usb_control_msg_recv() call sleep in atomic context? The 
trigger5_crtc_atomic_enable() callback executes in atomic context where
sleeping is forbidden. However, it performs synchronous USB I/O and uses a
GFP_KERNEL allocation, both of which sleep and may cause instability.

[ ... ]

> +	trigger5_calculate_pll(&request.pll, mode->clock);
> +	clk = div_u64(10000000ULL * request.pll.mul1 * request.pll.mul2,
> +		      (u32)request.pll.prediv * request.pll.div1 *
> +			      request.pll.div2 * 1000);
> +	drm_dbg_kms(&trigger5->drm,
> +		    "pll: %02x %02x %02x %02x %02x -> %llu kHz (want %d kHz)\n",
> +		    request.pll.prediv, request.pll.mul1, request.pll.mul2,
> +		    request.pll.div1, request.pll.div2, clk, mode->clock);
> +
> +	/* wValue can be any value since we are sending a custom mode */
> +	ret = usb_control_msg_send(udev, 0, TRIGGER5_REQUEST_SET_MODE,
> +				   USB_DIR_OUT | USB_TYPE_VENDOR |
> +					   USB_RECIP_DEVICE,
> +				   0, 0, &request, sizeof(request),
> +				   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);

[Severity: High]
Does this sequence trigger a sleep inside the CRTC atomic enable callback?
usb_control_msg_send() is a sleeping function and also performs a GFP_KERNEL
allocation, which can lead to deadlocks since this code path executes in 
atomic context.

[ ... ]

> +static void trigger5_crtc_atomic_disable(struct drm_crtc *crtc,
> +					 struct drm_atomic_commit *state)
> +{
> +	struct trigger5_device *trigger5 = to_trigger5(crtc->dev);
> +	struct usb_device *udev = interface_to_usbdev(trigger5->intf);
> +	u8 data;
> +	int idx, ret;
> +
> +	if (!drm_dev_enter(crtc->dev, &idx))
> +		return;
> +
> +	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]
Can this control message execution cause a kernel panic? Like atomic_enable,
trigger5_crtc_atomic_disable() runs in atomic context. Executing a 
synchronous USB control message with a GFP_KERNEL allocation here will sleep 
and may cause issues.

[ ... ]

> +static void trigger5_plane_atomic_update(struct drm_plane *plane,
> +					 struct drm_atomic_commit *atomic_state)
> +{
> +	struct drm_plane_state *old_state =
> +		drm_atomic_get_old_plane_state(atomic_state, plane);

[ ... ]

> +	/* Buffers are sized for the full mode in crtc atomic_check. */
> +	if (drm_WARN_ON_ONCE(plane->dev, frame_len > current_transfer->buf.len))
> +		goto exit;
> +
> +	/*
> +	 * 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]
Could this bounded wait result in sleeping in atomic context?
trigger5_plane_atomic_update() runs as a plane atomic update callback, which
executes in atomic context. Calling wait_for_completion_timeout() directly 
inside this path violates the DRM subsystem guidelines against sleeping in 
atomic commit paths.

[ ... ]

> +	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]
Is it possible for drm_gem_fb_begin_cpu_access() to block here?
This function takes DMA reservation locks and may sleep waiting for fences. 
Because trigger5_plane_atomic_update() executes in atomic context, blocking 
here might lead to instability or system deadlocks.

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