Hi Hans,
Passing on the actual error code was intentional.
My main goal was to give user space ability to
distinguish between the "no-lock" and "usb failure"
conditions. HDPVR firmware instability usually
manifests itself as a "usb failure", and passing
the error code on gives the application ability,
for example to use this device
(https://sites.google.com/site/hdpvrkillerdevice/)
as soon as firmware crash is detected and not
loose the recording. Unfortunately today because
of inability to see the usb failures the failed
recording is only detected by the zero-length
file after it's too late...
If you remember I did a some research and testing
with MythTV (email from Apr 17):
***************
...I checked MythTV function that is
monitoring video signal (AnalogSignalMonitor::handleHDPVR() in
analogsignalmonitor.cpp). Please see below that the no-signal
condition is checked as both - the ioctl failure and
pix.width is equal to 0:
if ((ioctl(videofd, VIDIOC_G_FMT, &vfmt) == 0) &&
vfmt.fmt.pix.width &&...
Then I tested MythTV for no-signal condition in two cases:
1. Loosing signal during live preview.
2. Trying to enter preview with no signal.
Both cases showed the same behavior as with the current
driver...
***************
So based on this, returning the actual error code
will NOT break MythTV logic, it still will handle
it as a no-lock condition.
One thing I have not done though is I did not contact
MythTV to confirm the above conclusions, but I
wouldn't mind if that would help to shift the decision...
Everything else in the patches looks good to me!
Best regards,
-Leo.
-------- Original Message --------
Subject: Re: [PATCH] [media] hdpvr: Simplify the logic that checks for
error
From: Hans Verkuil <hverk...@xs4all.nl>
Date: Mon, May 27, 2013 11:58 pm
To: Mauro Carvalho Chehab <mche...@redhat.com>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
l...@lumanate.com
On Mon May 27 2013 14:04:29 Mauro Carvalho Chehab wrote:
> At get_video_info, there's a somewhat complex logic that checks
> for error.
>
> That logic can be highly simplified, as usb_control_msg will
> only return a negative value, or the buffer length, as it does
> the transfers via DMA.
>
> While here, document why this particular driver is returning -EFAULT,
> instead of the USB error code.
Nacked-by: Hans Verkuil <hans.verk...@cisco.com>
The EFAULT comment is wrong. The way it is done today is that the error
return of this function is never passed on to userspace.
It's getting messy, so I think it is best if I make two patches based on
this
patch and on Leo's fourth patch and post those. If everyone agrees on my
solution,
then they can be merged.
Sorry Leo, I wasn't aware when we discussed the usb_control_msg return
values
before that usb_control_msg() will either return an error or the buffer
length,
and nothing else.
Your fourth patch introduced some bugs which I hadn't realized until
yesterday.
Which is why it wasn't merged. The main problem with your fourth patch
was that
it passed on the get_video_info error code to userspace, but that error
code was
for internal use only, and -EFAULT is an inappropriate error code to
pass on.
Regards,
Hans
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@redhat.com>
> ---
> drivers/media/usb/hdpvr/hdpvr-control.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr-control.c
> b/drivers/media/usb/hdpvr/hdpvr-control.c
> index d1a3d84..a015a24 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-control.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-control.c
> @@ -56,12 +56,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) {
> char print_buf[15];
> @@ -73,11 +67,20 @@ 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 */
> - ret = -EFAULT;
> - }
> + /*
> + * Returning EFAULT is wrong. Unfortunately, MythTV hdpvr
> + * handling code was written to expect this specific error,
> + * instead of accepting any error code. So, we can't fix it
> + * in Kernel without breaking userspace.
> + */
> + if (ret < 0)
> + return -EFAULT;
>
> - 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];
> +
> + return 0;
> }
>
> int get_input_lines_info(struct hdpvr_device *dev)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
