Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Vblank event is leaked on CRTC disable and may be delivered prematurely on CRTC enable. - [Medium] Calling `drm_connector_attach_encoder()` on an uninitialized connector. Pre-existing issues: - [High] Calling `usleep_range()` from a CRTC `atomic_enable` callback. - [High] Direct, lockless access to global state pointers `plane->state` and `crtc->state` in atomic commit callbacks. -- Note: The format of this review has been altered to a free-form summary due to mailing list quoting restrictions. Commit: 2c5e6508a64461d7a207075c4328681142076aef Author: Ze Huang <[email protected]> Subject: drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers Summary: This commit converts the TVE200 driver to use explicit plane, CRTC, and encoder objects instead of the simple display pipe helpers, while keeping generic plane-state validation and GEM framebuffer preparation. [Severity: High] In drivers/gpu/drm/tve200/tve200_display.c, tve200_plane_helper_atomic_update() checks if the CRTC is NULL and returns early. Does this cause the pending vblank event to be leaked during a CRTC disable, since the plane is also disabled (making crtc NULL) and the event is never consumed? Additionally, since commit_planes() executes before commit_modeset_enables(), is the event processed and delivered prematurely on CRTC enable? Could the event consumption be moved to CRTC callbacks like atomic_enable, atomic_disable, or atomic_flush instead of handling it in the plane callback? [Severity: Medium] In drivers/gpu/drm/tve200/tve200_drv.c, tve200_modeset_init() gets the connector using drm_panel_bridge_connector() and immediately passes it to drm_connector_attach_encoder(). Since drm_bridge_attach() has not run yet, is this modifying an uninitialized DRM object structure? Would it be better to rely on panel_bridge_attach(), which already automatically handles attaching the connector to the encoder, making this manual call redundant? [Severity: High] This isn't a bug introduced by this patch, but in drivers/gpu/drm/tve200/tve200_display.c, tve200_crtc_helper_atomic_enable() contains a hardware polling loop that calls usleep_range(30000, 50000). Since CRTC atomic enable callbacks run in atomic context during non-blocking commits where sleeping is strictly forbidden, can this cause kernel warnings or deadlocks? Should this be replaced with non-sleeping delay functions like udelay or ndelay? [Severity: High] This is a pre-existing issue, but it appears to be expanded here. In drivers/gpu/drm/tve200/tve200_display.c, tve200_plane_helper_atomic_update() and tve200_crtc_helper_atomic_enable() directly access the global state pointers plane->state and priv->plane.state. In non-blocking commits, these global pointers can be swapped if userspace submits a concurrent commit. Does this create a TOCTOU race where a concurrent commit setting the framebuffer to NULL could lead to a NULL pointer dereference when accessing fb->format->format? Should this code use the proper drm_atomic_get_new_plane_state() (or equivalent) accessors on the passed state object instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053...@oss.qualcomm.com?part=8
