question about drivers/media/usb/gspca/kinect.c

question about drivers/media/usb/gspca/kinect.c

[Julia Lawall]

The following code, in the function send_cmd, looks too concise:

        do {
                actual_len = kinect_read(udev, ibuf, 0x200);
        } while (actual_len == 0);
        PDEBUG(D_USBO, "Control reply: %d", res);
        if (actual_len < sizeof(*rhdr)) {
                pr_err("send_cmd: Input control transfer failed (%d)\n", res);
                return res;
        }

It seems that actual_len might be less than sizeof(*rhdr) either because 
an error code is returned by the call to kinect_read or because a shorter 
length is returned than the desired one.  In the error code case, I would 
guess that one would want to return the error code, but I don't know what 
on would want to return in the other case.  In any case, res is not 
defined by this code, so what is returned is whatever the result of the 
previous call to kinect_write happened to be.

How should the code be changed?

thanks,
julia

Fwd: question about drivers/media/usb/gspca/kinect.c

[Hans de Goede]

Hi,

Forwarding this to Antonio, the author of the kinect driver, who is
the best person to answer this.

Regards,

Hans



-------- Original Message --------
Subject: question about drivers/media/usb/gspca/kinect.c
Date: Wed, 25 Dec 2013 20:00:34 +0100 (CET)
From: Julia Lawall <julia.lawall@lip6.fr>
To: hdegoede@redhat.com, m.chehab@samsung.com, linux-media@vger.kernel.org,        linux-kernel@vger.kernel.org

The following code, in the function send_cmd, looks too concise:

         do {
                 actual_len = kinect_read(udev, ibuf, 0x200);
         } while (actual_len == 0);
         PDEBUG(D_USBO, "Control reply: %d", res);
         if (actual_len < sizeof(*rhdr)) {
                 pr_err("send_cmd: Input control transfer failed (%d)\n", res);
                 return res;
         }

It seems that actual_len might be less than sizeof(*rhdr) either because
an error code is returned by the call to kinect_read or because a shorter
length is returned than the desired one.  In the error code case, I would
guess that one would want to return the error code, but I don't know what
on would want to return in the other case.  In any case, res is not
defined by this code, so what is returned is whatever the result of the
previous call to kinect_write happened to be.

How should the code be changed?

thanks,
julia

Re: question about drivers/media/usb/gspca/kinect.c

[Antonio Ospite]

On Wed, 25 Dec 2013 20:00:34 +0100 (CET)
Julia Lawall <julia.lawall@lip6.fr> wrote:

> The following code, in the function send_cmd, looks too concise:
> 
>         do {
>                 actual_len = kinect_read(udev, ibuf, 0x200);
>         } while (actual_len == 0);
>         PDEBUG(D_USBO, "Control reply: %d", res);
>         if (actual_len < sizeof(*rhdr)) {
>                 pr_err("send_cmd: Input control transfer failed (%d)\n", res);
>                 return res;
>         }
> 
> It seems that actual_len might be less than sizeof(*rhdr) either because 
> an error code is returned by the call to kinect_read or because a shorter 
> length is returned than the desired one.  In the error code case, I would 
> guess that one would want to return the error code, but I don't know what 
> on would want to return in the other case.  In any case, res is not 
> defined by this code, so what is returned is whatever the result of the 
> previous call to kinect_write happened to be.
> 
> How should the code be changed?
>

Thanks Julia,

some other drivers return -EIO when the actual transfer length does not
match the requested one[1], and from Documentation/usb/error-codes.txt
[2] it looks like -EREMOTEIO is also used to represent partial
transfers in some cases. So I'd say either one of the two is OK.

The interested code is almost the same used in libfreenect[3], so I'd
stay with a minimal change here:

diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
index 3773a8a..48084736 100644
--- a/drivers/media/usb/gspca/kinect.c
+++ b/drivers/media/usb/gspca/kinect.c
@@ -158,7 +158,7 @@ static int send_cmd(struct gspca_dev *gspca_dev, uint16_t cmd, void *cmdbuf,
        PDEBUG(D_USBO, "Control reply: %d", res);
        if (actual_len < sizeof(*rhdr)) {
                pr_err("send_cmd: Input control transfer failed (%d)\n", res);
-               return res;
+               return actual_len < 0 ? actual_len : -EREMOTEIO;
        }
        actual_len -= sizeof(*rhdr);

Proper patches on their way, to libfreenect too.

Thanks again,
   Antonio

[1]
http://lxr.linux.no/#linux+v3.12.6/drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c#L37
[2]
http://lxr.linux.no/#linux+v3.12.6/Documentation/usb/error-codes.txt#L134
[3]
https://github.com/OpenKinect/libfreenect/blob/master/src/flags.c#L87

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 1/2] gspca_kinect: fix kinect_read() error path

[Antonio Ospite]

The error checking code relative to the invocations of kinect_read()
does not return the actual return code of the function just called, it
returns "res" which still contains the value of the last invocation of
a previous kinect_write().

Return the proper value, and while at it also report with -EREMOTEIO the
case of a partial transfer.

Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/media/usb/gspca/kinect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
index 3773a8a..48084736 100644
--- a/drivers/media/usb/gspca/kinect.c
+++ b/drivers/media/usb/gspca/kinect.c
@@ -158,7 +158,7 @@ static int send_cmd(struct gspca_dev *gspca_dev, uint16_t cmd, void *cmdbuf,
 	PDEBUG(D_USBO, "Control reply: %d", res);
 	if (actual_len < sizeof(*rhdr)) {
 		pr_err("send_cmd: Input control transfer failed (%d)\n", res);
-		return res;
+		return actual_len < 0 ? actual_len : -EREMOTEIO;
 	}
 	actual_len -= sizeof(*rhdr);
 
-- 
1.8.5.2

[PATCH 2/2] gspca_kinect: fix messages about kinect_read() return value

[Antonio Ospite]

Messages relative to kinect_read() are printing "res" which contains the
return value of a previous kinect_write().

Print the correct value in the messages.

Cc: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/media/usb/gspca/kinect.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
index 48084736..081f051 100644
--- a/drivers/media/usb/gspca/kinect.c
+++ b/drivers/media/usb/gspca/kinect.c
@@ -155,9 +155,10 @@ static int send_cmd(struct gspca_dev *gspca_dev, uint16_t cmd, void *cmdbuf,
 	do {
 		actual_len = kinect_read(udev, ibuf, 0x200);
 	} while (actual_len == 0);
-	PDEBUG(D_USBO, "Control reply: %d", res);
+	PDEBUG(D_USBO, "Control reply: %d", actual_len);
 	if (actual_len < sizeof(*rhdr)) {
-		pr_err("send_cmd: Input control transfer failed (%d)\n", res);
+		pr_err("send_cmd: Input control transfer failed (%d)\n",
+		       actual_len);
 		return actual_len < 0 ? actual_len : -EREMOTEIO;
 	}
 	actual_len -= sizeof(*rhdr);
-- 
1.8.5.2

Re: [PATCH 1/2] gspca_kinect: fix kinect_read() error path

[Hans de Goede]

Hi,

Thanks I've added both to my gspca tree for 3.15

Regards,

Hans


On 12/30/2013 05:41 PM, Antonio Ospite wrote:
> The error checking code relative to the invocations of kinect_read()
> does not return the actual return code of the function just called, it
> returns "res" which still contains the value of the last invocation of
> a previous kinect_write().
> 
> Return the proper value, and while at it also report with -EREMOTEIO the
> case of a partial transfer.
> 
> Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> ---
>  drivers/media/usb/gspca/kinect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
> index 3773a8a..48084736 100644
> --- a/drivers/media/usb/gspca/kinect.c
> +++ b/drivers/media/usb/gspca/kinect.c
> @@ -158,7 +158,7 @@ static int send_cmd(struct gspca_dev *gspca_dev, uint16_t cmd, void *cmdbuf,
>  	PDEBUG(D_USBO, "Control reply: %d", res);
>  	if (actual_len < sizeof(*rhdr)) {
>  		pr_err("send_cmd: Input control transfer failed (%d)\n", res);
> -		return res;
> +		return actual_len < 0 ? actual_len : -EREMOTEIO;
>  	}
>  	actual_len -= sizeof(*rhdr);
>  
>