Re: [PATCH 3/3] drm/tiny: add driver for Valkyrie USB LCD displays

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/tiny: missing drm_atomic_helper_shutdown() in vk_disconnect()
- [Medium] drm/tiny: missing drm_dev_set_dma_dev() in vk_probe()
- [Medium] drm/tiny: vk_crtc_atomic_check() skips validation when CRTC is inactive
- [Medium] drm/tiny: vk_device_enable_pre() leaves device_state true on failure
- [Medium] drm/tiny: valkyrie-lcd: broken resume recovery when drm suspend fails
--

commit 172f750ba1a198c982441dc0256107aec140e020
Author: ZiAo Wang <ziaowang2007@gmail.com>

drm/tiny: add driver for Valkyrie USB LCD displays

This commit adds a DRM/KMS driver for Valkyrie USB LCD displays (320x240 and
360x960 variants) using GEM shmem buffers, atomic modesetting, USB control
transfers for panel initialization, and bulk OUT transfers for UYVY frame
streaming.

> diff --git a/drivers/gpu/drm/tiny/valkyrie-lcd.c b/drivers/gpu/drm/tiny/valkyrie-lcd.c
> new file mode 100644
> index 0000000000000..6478e9246263b
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/valkyrie-lcd.c
[ ... ]
> +static int vk_plane_atomic_check(struct drm_plane *plane,
> +				 struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *new_plane_state =
> +		drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *new_crtc_state;
> +	int ret;
> +
> +	if (!new_plane_state->fb)
> +		return 0;
> +
> +	if (!new_plane_state->crtc)
> +		return -EINVAL;
> +	new_crtc_state =
> +		drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
> +	if (!new_crtc_state)
> +		return -EINVAL;
> +
> +	ret = drm_atomic_helper_check_plane_state(new_plane_state,
> +						  new_crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING, false,
> +						  false);
> +	if (ret)
> +		return ret;
> +
> +	if (new_plane_state->rotation != DRM_MODE_ROTATE_0) {
> +		drm_dbg_kms(plane->dev, "Unsupported plane rotation %#x\n",
> +			    new_plane_state->rotation);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
[ ... ]
> +static int vk_crtc_atomic_check(struct drm_crtc *crtc,
> +				struct drm_atomic_commit *state)
> +{
> +	struct vk_driver *drv = container_of(crtc->dev, struct vk_driver, drm);
> +	struct drm_crtc_state *new_crtc_state =
> +		drm_atomic_get_new_crtc_state(state, crtc);
> +	struct drm_plane *plane = &drv->plane;
> +	struct vk_connector_driver *connector_drv = drv->connector;
> +	struct drm_plane_state *new_plane_state;
> +
> +	if (!new_crtc_state->enable || !new_crtc_state->active)
> +		return 0;
> +
> +	if (new_crtc_state->plane_mask != drm_plane_mask(plane) ||
> +	    new_crtc_state->connector_mask !=
> +		    drm_connector_mask(&connector_drv->connector)) {
> +		drm_dbg_kms(crtc->dev,
> +			    "Active CRTC requires the primary plane and USB connector\n");
> +		return -EINVAL;
> +	}
> +
> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	if (!new_plane_state)
> +		new_plane_state = plane->state;
> +	if (!new_plane_state || new_plane_state->crtc != crtc ||
> +	    !new_plane_state->fb) {
> +		drm_dbg_kms(crtc->dev,
> +			    "Active CRTC has no framebuffer on its primary plane\n");
> +		return -EINVAL;
> +	}

[Severity: Medium]
Should vk_crtc_atomic_check() only return early when !new_crtc_state->enable,
rather than also skipping validation when !new_crtc_state->active?

According to the struct drm_crtc_state documentation, drivers must not consult
new_crtc_state->active in atomic_check to decide whether shared resources are
required because userspace expects a subsequent DPMS ON transition to succeed.

When the CRTC is enabled but inactive (enable is true, active is false),
returning early here allows userspace (for example via DRM_IOCTL_MODE_RMFB or
DRM_IOCTL_MODE_SETPLANE) to detach the primary plane and its framebuffer while
leaving the CRTC enabled with plane_mask == 0. Any subsequent commit setting
active back to true will then fail in vk_crtc_atomic_check() with -EINVAL.

Also, in vk_plane_atomic_check(), returning 0 before calling
drm_atomic_helper_check_plane_state() when !new_plane_state->fb skips clearing
new_plane_state->visible to false when disabling the plane.

[ ... ]
> +	if (!new_enable) {
> +		if (drv->device_state)
> +			ret = vk_device_disable(drv);
> +	} else {
> +		bool enabling = !drv->device_state;
> +
> +		/* Testing found a ~10-second timeout; refresh every 3 seconds. */
> +		mod_delayed_work(system_dfl_wq, &crtc_drv->timeout_flush,
> +				 msecs_to_jiffies(VK_TIMEOUT_FLUSH_INTERVAL_MS));
> +		if (enabling)
> +			ret = vk_device_enable_pre(drv);
> +		ret2 = vk_scan_frame(crtc_drv, connector_drv,
> +				     to_drm_shadow_plane_state(new_plane_state));
> +		ret = ret ? ret : ret2;
> +		if (enabling) {
> +			ret2 = vk_device_enable_post(drv);
> +			ret = ret ? ret : ret2;
> +		}
> +	}

[Severity: Medium]
What happens in vk_crtc_atomic_flush() if vk_device_enable_pre(),
vk_scan_frame(), or vk_device_enable_post() fails when enabling is true?

In vk_device_enable_pre():

	drv->device_state = true;
	return vk_a604_switch(drv, true);

drv->device_state is unconditionally set to true before vk_a604_switch() and
the rest of the enable sequence succeed. If a transient USB error or poll
timeout occurs during vk_device_enable_pre(), vk_crtc_atomic_flush() still
proceeds to call vk_scan_frame() and vk_device_enable_post(), and leaves
drv->device_state set to true. Because drv->device_state stays true,
subsequent page flips and 3-second timeout_flush refreshes will see enabling
as false and will never retry the enable sequence.

Similarly, in vk_device_reset(), vk_device_enable_pre() is called
unconditionally even when vk_device_bootstrap() returns an error.

[ ... ]
> +static int vk_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +{
> +	struct vk_driver *drv = devm_drm_dev_alloc(&intf->dev, &vk_drm_driver,
> +						   struct vk_driver, drm);
> +	int ret;
> +
> +	if (IS_ERR(drv))
> +		return dev_err_probe(&intf->dev, PTR_ERR(drv),
> +				     "Failed to allocate DRM device\n");
> +	mutex_init(&drv->io_lock);
> +
> +	ret = vk_init_usb(drv, intf);
> +	if (ret)
> +		goto fail;

[Severity: Medium]
Does vk_probe() need to configure the DMA device on drv->drm via
usb_intf_get_dma_device(intf) and drm_dev_set_dma_dev()?

vk_drm_driver uses DRM_GEM_SHMEM_DRIVER_OPS, which wires up PRIME DMA-BUF
import via drm_gem_prime_import() -> drm_gem_prime_import_dev(dev, dma_buf,
drm_dev_dma_dev(dev)). Because drv->drm is allocated with &intf->dev as its
parent and a struct usb_interface device has no dma_mask, importing a DMA-BUF
via DRM_IOCTL_PRIME_FD_TO_HANDLE will pass &intf->dev to
dma_buf_map_attachment(), triggering a WARN_ON_ONCE(!dev->dma_mask) and
failing with -EINVAL.

[ ... ]
> +static void vk_disconnect(struct usb_interface *intf)
> +{
> +	struct vk_driver *drv = usb_get_intfdata(intf);
> +	int ret;
> +
> +	drm_dbg_driver(&drv->drm, "Disconnecting display\n");
> +
> +	drm_dev_get(&drv->drm);
> +	drm_dev_unplug(&drv->drm);
> +
> +	mutex_lock(&drv->io_lock);
> +	drv->io_blocking = true;
> +	mutex_unlock(&drv->io_lock);
> +
> +	cancel_delayed_work_sync(&drv->crtc->timeout_flush);
> +
> +	if (drv->drm.mode_config.suspend_state) {
> +		drm_atomic_commit_put(drv->drm.mode_config.suspend_state);
> +		drv->drm.mode_config.suspend_state = NULL;
> +	}
> +
> +	drm_dev_put(&drv->drm);

[Severity: High]
Can skipping drm_atomic_helper_shutdown(&drv->drm) in vk_disconnect() leak the
active shadow-plane vmap and pinned shmem pages on unplug?

When the display is active, drm_gem_begin_shadow_fb_access() vmaps the
framebuffer (incrementing shmem->vmap_use_count, shmem->pages_pin_count, and
shmem->pages_use_count), and vk_crtc_atomic_enable() enables the CRTC vblank
timer. The shadow-plane vmap is only released by drm_gem_end_shadow_fb_access()
when a subsequent atomic commit disables or replaces the plane state.

Because vk_disconnect() never calls drm_atomic_helper_shutdown(&drv->drm), if
the active framebuffer is not explicitly removed by drm_mode_rmfb() before the
final drm_dev_put() (for example when a compositor has called
DRM_IOCTL_MODE_CLOSEFB on its frontbuffer, or when fbdev emulation is not
enabled), drm_mode_config_cleanup() destroys the active shadow_plane_state via
drm_gem_destroy_shadow_plane_state() without calling
drm_gem_end_shadow_fb_access(). When the framebuffer is freed,
drm_gem_shmem_release() hits drm_WARN_ONs for non-zero vmap_use_count,
pages_use_count, and pages_pin_count, leaking the kernel vmap and backing
pages, and drm_vblank_init_release() also warns that vblank->enabled is still
set.

In addition, if vk_timeout_atomic_submit() queued a non-blocking atomic commit
just before cancel_delayed_work_sync(&drv->crtc->timeout_flush), its
commit_work is not drained without drm_atomic_helper_shutdown() before
vk_device_reset() and teardown run.

[ ... ]
> +static int __vk_resume_device(struct usb_interface *intf, bool rebootstrap,
> +			      bool reidentify)
> +{
> +	struct vk_driver *drv = usb_get_intfdata(intf);
> +	int ret = 0;
[ ... ]
> +	drv->io_blocking = false;
> +	mutex_unlock(&drv->io_lock);
> +
> +	if (drv->drm_suspended) {
> +		ret = drm_mode_config_helper_resume(&drv->drm);
> +		drv->drm_suspended = false;
> +		if (ret)
> +			drm_err(&drv->drm, "Failed to restore DRM state: %d\n",
> +				ret);
> +	} else {
> +		drm_dbg_driver(&drv->drm,
> +			       "No saved DRM state; scheduling an immediate refresh\n");
> +		schedule_delayed_work(&drv->crtc->timeout_flush, 0);
> +	}
[ ... ]
> +static int __vk_suspend_device(struct usb_interface *intf, bool force)
> +{
> +	struct vk_driver *drv = usb_get_intfdata(intf);
> +	int ret = 0;
> +
> +	drm_dbg_driver(&drv->drm,
> +		       "Suspending display (force=%d, DRM suspended=%d)\n",
> +		       force, drv->drm_suspended);
> +
> +	if (!drv->drm_suspended) {
> +		ret = drm_mode_config_helper_suspend(&drv->drm);
> +		if (ret)
> +			drm_err(&drv->drm, "Failed to suspend DRM state: %d\n",
> +				ret);
> +		if (ret && !force)
> +			return ret;
> +		if (!ret)
> +			drv->drm_suspended = true;
> +	}
> +
> +	mutex_lock(&drv->io_lock);
> +	drv->io_blocking = true;
> +	mutex_unlock(&drv->io_lock);
> +	cancel_delayed_work_sync(&drv->crtc->timeout_flush);
> +	drm_dbg_driver(&drv->drm, "Display I/O suspended\n");
> +	return ret;
> +}

[Severity: Medium]
In __vk_suspend_device(), if force is true (from vk_suspend()) and
drm_mode_config_helper_suspend() fails while the display is active, can the
display recover on vk_resume()?

When drm_mode_config_helper_suspend() fails, vk_device_disable() is not
called, so the panel remains enabled with drv->device_state == true and
drv->drm_suspended == false while timeout_flush is cancelled across suspend.
Once the panel's ~10-second hardware watchdog expires during suspend and
resets the device, vk_resume() calls __vk_resume_device(intf, false, false)
and takes the !drv->drm_suspended fallback branch to schedule timeout_flush.
However, when timeout_flush runs vk_crtc_atomic_flush(), drv->device_state is
still true, so enabling (!drv->device_state) is false and neither
vk_device_bootstrap() nor vk_device_enable_pre() / vk_device_enable_post() is
executed.

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