On 8/4/19 12:09 AM, Keith Pyle wrote:
> `hdpvr_read` attempts to restart streaming if the device is read while
> it is both not ready and not disconnected. However, the device is often
> still not ready immediately after the call to `hdpvr_start_streaming`
> returns, causing the condition `if (buf->status != BUFSTAT_READY)` to
> exit the loop without reading any further data. By itself, this would
> merely cause a short read, which should be easily recoverable. However,
> if no data has been read so far, this causes `hdpvr_read` to return 0,
> which results in an end-of-file for the user application.
>
> Compensate for this by adding the ability to delay after the call to
> `hdpvr_start_streaming`, then `continue;` back to the top, so that
> `hdpvr_read` can call `wait_event_interruptible_timeout` again to wait
> for the device to become ready. This delay complements the prior patch.
> The prior patch delays before issuing the start-streaming command, to
> give the firmware time to stabilize before receiving the command. This
> delay is after the start-streaming command, to give the firmware time to
> bring the device to a ready state. This delay is configurable through a
> new module parameter, `hdpvr_restart_streaming_ms_delay`, which defaults
> to a 100 millisecond delay.
>
> To avoid an infinite loop in `hdpvr_read`, add a limit to how many times
> `hdpvr_read` can restart the device before returning. This limit is
> configurable through a new module parameter,
> `hdpvr_restart_streaming_max_tries`, and defaults to one restart.
> Administrators may set the limit to 0 to request that `hdpvr_read` never
> attempt to restart streaming. Previously, there was no way for
> administrators to opt out of an attempted restart.
>
> Signed-off-by: Keith Pyle <kpyle@xxxxxxxxxxxxx>
> Tested-by: Keith Pyle <kpyle@xxxxxxxxxxxxx>
> ---
> Changes since v2:
> - Rewrapped comments again, per request from Hans.
> - Per advice from checkpatch.pl --strict (suggested by Hans), added
> spacing around `|` for mode permissions. This satisfies checkpatch,
> but reduces consistency in hdpvr-core.c, which had preexisting uses that
> violate checkpatch --strict.
> - Per request from Hans, switched from pre-decrement to post-decrement.
> Changes since v1:
> - Rewrapped output at 80 columns, per request from Hans. Literal strings
> still exceed 80 columns where necessary to keep an entire string together,
> since this makes it easier for grep to find the file and line that
> generates a given message.
> ---
> drivers/media/usb/hdpvr/hdpvr-core.c | 8 ++++++
> drivers/media/usb/hdpvr/hdpvr-video.c | 36 +++++++++++++++++++++++++++
> drivers/media/usb/hdpvr/hdpvr.h | 2 ++
> 3 files changed, 46 insertions(+)
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c b/drivers/media/usb/hdpvr/hdpvr-core.c
> index a3d2f632fe38..1be3911e43ed 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-core.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-core.c
> @@ -43,6 +43,14 @@ uint hdpvr_close_to_open_ms_delay = 4000;
> module_param(hdpvr_close_to_open_ms_delay, uint, S_IRUGO | S_IWUSR);
As per checkpatch output, use octal here.
> MODULE_PARM_DESC(hdpvr_close_to_open_ms_delay, "delay restarting streaming by the specified number of milliseconds");
>
> +uint hdpvr_restart_streaming_max_tries = 1;
> +module_param(hdpvr_restart_streaming_max_tries, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_restart_streaming_max_tries, "restart streaming at most this many times within one read");
> +
> +uint hdpvr_restart_streaming_ms_delay = 100;
> +module_param(hdpvr_restart_streaming_ms_delay, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_restart_streaming_ms_delay, "delay continue by the specified number of milliseconds after restarting streaming");
> +
> static uint default_video_input = HDPVR_VIDEO_INPUTS;
> module_param(default_video_input, uint, S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(default_video_input, "default video input: 0=Component / 1=S-Video / 2=Composite");
> diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c
> index 7e5897dd8dff..aa7b473b501b 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-video.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-video.c
> @@ -441,6 +441,8 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> struct hdpvr_buffer *buf = NULL;
> struct urb *urb;
> unsigned int ret = 0;
> + unsigned int restarts_remaining = hdpvr_restart_streaming_max_tries;
> + unsigned int delay;
> int rem, cnt;
>
> if (*pos)
> @@ -491,6 +493,19 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> goto err;
> }
> if (!err) {
> + if (restarts_remaining == 0) {
> + v4l2_dbg(MSG_BUFFER, hdpvr_debug,
> + &dev->v4l2_dev,
> + "timeout: no further restarts allowed by hdpvr_restart_streaming_max_tries; returning to caller with ret=%u",
> + ret);
> + /* This break will return the count of
As per coding style, put /* on a line by itself.
Besides these two points this patch looks good.
Regards,
Hans
> + * bytes copied so far, which may be 0.
> + * In that situation, the user
> + * application will get an EOF.
> + */
> + break;
> + }
> + restarts_remaining--;
> v4l2_info(&dev->v4l2_dev,
> "timeout: restart streaming\n");
> mutex_lock(&dev->io_mutex);
> @@ -501,6 +516,27 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> ret = err;
> goto err;
> }
> + /* hdpvr_start_streaming instructs the device to
> + * stream, but the device is usually not ready
> + * by the time hdpvr_start_streaming returns.
> + *
> + * Without this continue, the loop would
> + * terminate. If no data had been copied by a
> + * prior iteration of the loop, then hdpvr_read
> + * would return 0, closing the file descriptor
> + * prematurely. Continue back to the top of the
> + * loop to avoid that.
> + *
> + * The device may not be ready within 1 second,
> + * so the wait_event_interruptible_timeout would
> + * then restart streaming a second time. Delay
> + * here to give the device time to stabilize
> + * first.
> + */
> + delay = hdpvr_restart_streaming_ms_delay;
> + if (delay)
> + msleep(delay);
> + continue;
> }
> }
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
> index 32498b7120aa..f6f9ddf89faf 100644
> --- a/drivers/media/usb/hdpvr/hdpvr.h
> +++ b/drivers/media/usb/hdpvr/hdpvr.h
> @@ -44,6 +44,8 @@
>
> extern int hdpvr_debug;
> extern uint hdpvr_close_to_open_ms_delay;
> +extern uint hdpvr_restart_streaming_max_tries;
> +extern uint hdpvr_restart_streaming_ms_delay;
>
> #define MSG_INFO 1
> #define MSG_BUFFER 2
>
