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 <[email protected]> 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(¤t_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/[email protected]?part=1
