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 <hverkuil@xxxxxxxxx> Date: Mon, May 27, 2013 11:58 pm To: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>, leo@xxxxxxxxxxxx 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.verkuil@xxxxxxxxx> 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 <mchehab@xxxxxxxxxx> > --- > 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 majordomo@xxxxxxxxxxxxxxx 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
