Re: [PATCH 2/4] Input: sur40 - fix V4L error path cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `vb2_wait_for_all_buffers` in `sur40_stop_streaming` causes a system deadlock.
--

commit 1d8742f82651a7f0160c799526e70838d3c9fa29
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

    Input: sur40 - fix V4L error path cleanup
    
    In sur40_probe(), if video_register_device() fails, the error path jumps to
    err_unreg_video. This incorrectly attempts to unregister a video device
    that was never successfully registered, and fails to free the V4L2 control
    handler (v4l2_ctrl_handler_free) that was initialized immediately prior.

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 8639ec3ad7038..e9089b0c3e2f4 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the current implementation of
sur40_stop_streaming() cause a system deadlock?

Looking at drivers/input/touchscreen/sur40.c:sur40_stop_streaming():

	vb2_wait_for_all_buffers(vq);
	sur40->sequence = -1;

	/* Release all active buffers */
	return_all_buffers(sur40, VB2_BUF_STATE_ERROR);

The driver unconditionally calls vb2_wait_for_all_buffers() before forcibly
returning the active buffers. This blocks infinitely until all queued buffers
are returned via vb2_buffer_done().

The driver expects the poller thread (sur40_poll) to process and return these
buffers. Is it possible for the poller thread to be stopped or not running
when this is called?

For instance, if userspace opens only /dev/videoX without ever opening
/dev/input/eventX, the input-bound poller never starts.

Additionally, if the device is abruptly unplugged while streaming is active,
sur40_disconnect() calls input_unregister_device(), which synchronously stops
the poller. When the video file descriptor is subsequently closed,
sur40_stop_streaming() is triggered.

In both of these scenarios, won't vb2_wait_for_all_buffers() wait forever for
buffers to be returned by a thread that is not running? Should the driver
synchronously cancel transfers and immediately return all queued buffers with
VB2_BUF_STATE_ERROR instead of waiting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616051235.1549517-1-dmitry.torokhov@gmail.com?part=2