[REVIEWv2 PATCH 0/3] hdpvr: various fixes

[REVIEWv2 PATCH 0/3] hdpvr: various fixes

[Hans Verkuil]

The first patch fixes a bug in querystd: if there is no signal, then
querystd should return V4L2_STD_UNKNOWN. There are more drivers that
return the wrong value here, I have a patch series pending to fix that
and also to improve the spec.

The second does a code cleanup that improves readability, but it doesn't
change the logic.

The third patch is based on a patch from Mauro and a patch from Leo:

https://patchwork.linuxtv.org/patch/18573/
https://linuxtv.org/patch/18399/

This improves the error handling in case usb_control_msg() returns an
error.

Changes since v1:

- Also return the low-level usb_control_msg error in the vidioc_g_fmt_vid_cap
  case.

Regards,

        Hans

[REVIEWv2 PATCH 1/3] hdpvr: fix querystd 'unknown format' return.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If no format has been detected, then querystd should return V4L2_STD_UNKNOWN,
not V4L2_STD_ALL.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/hdpvr/hdpvr-video.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c
index 2d02b49..81018c4 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -613,7 +613,7 @@ static int vidioc_querystd(struct file *file, void *_fh, v4l2_std_id *a)
 	struct hdpvr_fh *fh = _fh;
 	int ret;
 
-	*a = V4L2_STD_ALL;
+	*a = V4L2_STD_UNKNOWN;
 	if (dev->options.video_input == HDPVR_COMPONENT)
 		return fh->legacy_mode ? 0 : -ENODATA;
 	ret = get_video_info(dev, &vid_info);
-- 
1.7.10.4

[REVIEWv2 PATCH 2/3] hdpvr: code cleanup

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Remove an unnecessary 'else' and invert a condition which makes the code
more readable.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/hdpvr/hdpvr-video.c |   54 ++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c
index 81018c4..e947105 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -281,43 +281,43 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 
 	if (dev->status == STATUS_STREAMING)
 		return 0;
-	else if (dev->status != STATUS_IDLE)
+	if (dev->status != STATUS_IDLE)
 		return -EAGAIN;
 
 	ret = get_video_info(dev, &vidinf);
+	if (ret) {
+		msleep(250);
+		v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
+				"no video signal at input %d\n", dev->options.video_input);
+		return -EAGAIN;
+	}
 
-	if (!ret) {
-		v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
-			 "video signal: %dx%d@%dhz\n", vidinf.width,
-			 vidinf.height, vidinf.fps);
+	v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
+			"video signal: %dx%d@%dhz\n", vidinf.width,
+			vidinf.height, vidinf.fps);
 
-		/* start streaming 2 request */
-		ret = usb_control_msg(dev->udev,
-				      usb_sndctrlpipe(dev->udev, 0),
-				      0xb8, 0x38, 0x1, 0, NULL, 0, 8000);
-		v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
-			 "encoder start control request returned %d\n", ret);
-		if (ret < 0)
-			return ret;
+	/* start streaming 2 request */
+	ret = usb_control_msg(dev->udev,
+			usb_sndctrlpipe(dev->udev, 0),
+			0xb8, 0x38, 0x1, 0, NULL, 0, 8000);
+	v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
+			"encoder start control request returned %d\n", ret);
+	if (ret < 0)
+		return ret;
 
-		ret = hdpvr_config_call(dev, CTRL_START_STREAMING_VALUE, 0x00);
-		if (ret)
-			return ret;
+	ret = hdpvr_config_call(dev, CTRL_START_STREAMING_VALUE, 0x00);
+	if (ret)
+		return ret;
 
-		dev->status = STATUS_STREAMING;
+	dev->status = STATUS_STREAMING;
 
-		INIT_WORK(&dev->worker, hdpvr_transmit_buffers);
-		queue_work(dev->workqueue, &dev->worker);
+	INIT_WORK(&dev->worker, hdpvr_transmit_buffers);
+	queue_work(dev->workqueue, &dev->worker);
 
-		v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
-			 "streaming started\n");
+	v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
+			"streaming started\n");
 
-		return 0;
-	}
-	msleep(250);
-	v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
-		 "no video signal at input %d\n", dev->options.video_input);
-	return -EAGAIN;
+	return 0;
 }
 
 
-- 
1.7.10.4

[REVIEWv2 PATCH 3/3] hdpvr: improve error handling

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

get_video_info() should never return EFAULT, instead it should return
the low-level usb_control_msg() error. Add a valid field to the hdpvr_video_info
struct so the driver can easily check if a valid format was detected.

Whenever get_video_info is called and it returns an error (e.g. usb_control_msg
failed), then return that error to userspace as well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/hdpvr/hdpvr-control.c |   21 +++++++++------------
 drivers/media/usb/hdpvr/hdpvr-video.c   |   18 +++++++++++-------
 drivers/media/usb/hdpvr/hdpvr.h         |    1 +
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-control.c b/drivers/media/usb/hdpvr/hdpvr-control.c
index df6bcb5..6053661 100644
--- a/drivers/media/usb/hdpvr/hdpvr-control.c
+++ b/drivers/media/usb/hdpvr/hdpvr-control.c
@@ -49,6 +49,7 @@ int get_video_info(struct hdpvr_device *dev, struct hdpvr_video_info *vidinf)
 {
 	int ret;
 
+	vidinf->valid = false;
 	mutex_lock(&dev->usbc_mutex);
 	ret = usb_control_msg(dev->udev,
 			      usb_rcvctrlpipe(dev->udev, 0),
@@ -56,11 +57,6 @@ int get_video_info(struct hdpvr_device *dev, struct hdpvr_video_info *vidinf)
 			      0x1400, 0x0003,
 			      dev->usbc_buf, 5,
 			      1000);
-	if (ret == 5) {
-		vidinf->width	= dev->usbc_buf[1] << 8 | dev->usbc_buf[0];
-		vidinf->height	= dev->usbc_buf[3] << 8 | dev->usbc_buf[2];
-		vidinf->fps	= dev->usbc_buf[4];
-	}
 
 #ifdef HDPVR_DEBUG
 	if (hdpvr_debug & MSG_INFO) {
@@ -73,14 +69,15 @@ int get_video_info(struct hdpvr_device *dev, struct hdpvr_video_info *vidinf)
 #endif
 	mutex_unlock(&dev->usbc_mutex);
 
-	if ((ret > 0 && ret != 5) ||/* fail if unexpected byte count returned */
-	    !vidinf->width ||	/* preserve original behavior -  */
-	    !vidinf->height ||	/* fail if no signal is detected */
-	    !vidinf->fps) {
-		ret = -EFAULT;
-	}
+	if (ret < 0)
+		return ret;
 
-	return ret < 0 ? ret : 0;
+	vidinf->width	= dev->usbc_buf[1] << 8 | dev->usbc_buf[0];
+	vidinf->height	= dev->usbc_buf[3] << 8 | dev->usbc_buf[2];
+	vidinf->fps	= dev->usbc_buf[4];
+	vidinf->valid   = vidinf->width && vidinf->height && vidinf->fps;
+
+	return 0;
 }
 
 int get_input_lines_info(struct hdpvr_device *dev)
diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c
index e947105..4f8567a 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -285,7 +285,10 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 		return -EAGAIN;
 
 	ret = get_video_info(dev, &vidinf);
-	if (ret) {
+	if (ret < 0)
+		return ret;
+
+	if (!vidinf.valid) {
 		msleep(250);
 		v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
 				"no video signal at input %d\n", dev->options.video_input);
@@ -617,15 +620,12 @@ static int vidioc_querystd(struct file *file, void *_fh, v4l2_std_id *a)
 	if (dev->options.video_input == HDPVR_COMPONENT)
 		return fh->legacy_mode ? 0 : -ENODATA;
 	ret = get_video_info(dev, &vid_info);
-	if (ret)
-		return 0;
-	if (vid_info.width == 720 &&
+	if (vid_info.valid && vid_info.width == 720 &&
 	    (vid_info.height == 480 || vid_info.height == 576)) {
 		*a = (vid_info.height == 480) ?
 			V4L2_STD_525_60 : V4L2_STD_625_50;
 	}
-
-	return 0;
+	return ret;
 }
 
 static int vidioc_s_dv_timings(struct file *file, void *_fh,
@@ -679,6 +679,8 @@ static int vidioc_query_dv_timings(struct file *file, void *_fh,
 		return -ENODATA;
 	ret = get_video_info(dev, &vid_info);
 	if (ret)
+		return ret;
+	if (!vid_info.valid)
 		return -ENOLCK;
 	interlaced = vid_info.fps <= 30;
 	for (i = 0; i < ARRAY_SIZE(hdpvr_dv_timings); i++) {
@@ -1008,7 +1010,9 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *_fh,
 		struct hdpvr_video_info vid_info;
 
 		ret = get_video_info(dev, &vid_info);
-		if (ret)
+		if (ret < 0)
+			return ret;
+		if (!vid_info.valid)
 			return -EFAULT;
 		f->fmt.pix.width = vid_info.width;
 		f->fmt.pix.height = vid_info.height;
diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
index 808ea7a..dc685d4 100644
--- a/drivers/media/usb/hdpvr/hdpvr.h
+++ b/drivers/media/usb/hdpvr/hdpvr.h
@@ -154,6 +154,7 @@ struct hdpvr_video_info {
 	u16	width;
 	u16	height;
 	u8	fps;
+	bool	valid;
 };
 
 enum {
-- 
1.7.10.4