Re: usb_control_msg() reads 0 bytes in Linux 2.6.23-rc1

On Tue, 24 Jul 2007, Pavel Roskin wrote:

> Hi, Pete!
> 
> On Tue, 2007-07-24 at 13:09 -0700, Pete Zaitcev wrote:
> > On Tue, 24 Jul 2007 01:55:26 -0400, Pavel Roskin <[email protected]> wrote:
> 
> > > I was able to check that the data buffer remains unchanged even though
> > > the data size is not 0.  Actually, the return code is 0, which indicates
> > > that 0 bytes have been read.  But why?  And why is it not an error?
> 
> It turns out usb_control_msg() returns 0 in two cases where it returns
> -EPIPE in Linux 2.6.22.  Return value 0 is not expected to be an error,
> which prevents second stage of the firmware download, normally triggered
> by an error reading the firmware version.
> 
> Since the firmware version is not loaded, other usb_control_msg() calls
> fail as well in the same fashion, by returning 0.  Treating return code
> 0 as an error fixes the driver in Linux 2.6.23-rc1.
> 
> However, I think it's wrong to return 0 if more than 0 bytes were
> requested.  usb_control_msg() is synchronous, and the caller is supposed
> to wait for the data.  If the function returns and the data is not
> there, it should be an error IMHO.
> 
> > I looked at the changes in -rc1 and the only two vaguely suspicious areas
> > are the suspend and the ehci_hcd. So, I'm afraid someone has to bisect.
> 
> Oh well.  I'll try.
> 
> > The URB_SHORT_NOT_OK flag is not default. I don't remember exactly why.
> 
> I guess you mean that the temporary URB in usb_internal_control_msg()
> should use that flag?  That sounds like a good idea, at least on the
> surface.
> 
> > > I'll appreciate if some of the USB developers have a brief look at the
> > > driver.  Sure, I can debug deeper into usb_control_msg(), but most
> > > likely the driver is just doing something stupid.
> > > 
> > > The patch for the kernel:
> > > http://80211libre.org/at76/at76_usb.patch
> > 
> > I don't see anything suspicious in the patch.
> 
> Thanks!

I found the problem.  Greg's urb->status conversion was somewhat hasty.  
Here's an untested patch to fix it.

Alan Stern


Index: 2.6.23-rc1/drivers/usb/core/message.c
===================================================================
--- 2.6.23-rc1.orig/drivers/usb/core/message.c
+++ 2.6.23-rc1/drivers/usb/core/message.c
@@ -18,9 +18,17 @@
 #include "hcd.h"	/* for usbcore internals */
 #include "usb.h"
 
+struct api_context {
+	struct completion	done;
+	int			status;
+};
+
 static void usb_api_blocking_completion(struct urb *urb)
 {
-	complete((struct completion *)urb->context);
+	struct api_context *ctx = urb->context;
+
+	ctx->status = urb->status;
+	complete(&ctx->done);
 }
 
 
@@ -32,20 +40,21 @@ static void usb_api_blocking_completion(
  */
 static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
 { 
-	struct completion done;
+	struct api_context ctx;
 	unsigned long expire;
 	int retval;
-	int status = urb->status;
 
-	init_completion(&done); 	
-	urb->context = &done;
+	init_completion(&ctx.done);
+	urb->context = &ctx;
 	urb->actual_length = 0;
 	retval = usb_submit_urb(urb, GFP_NOIO);
 	if (unlikely(retval))
 		goto out;
 
 	expire = timeout ? msecs_to_jiffies(timeout) : MAX_SCHEDULE_TIMEOUT;
-	if (!wait_for_completion_timeout(&done, expire)) {
+	if (!wait_for_completion_timeout(&ctx.done, expire)) {
+		usb_kill_urb(urb);
+		retval = (ctx.status == -ENOENT ? -ETIMEDOUT : ctx.status);
 
 		dev_dbg(&urb->dev->dev,
 			"%s timed out on ep%d%s len=%d/%d\n",
@@ -54,11 +63,8 @@ static int usb_start_wait_urb(struct urb
 			usb_pipein(urb->pipe) ? "in" : "out",
 			urb->actual_length,
 			urb->transfer_buffer_length);
-
-		usb_kill_urb(urb);
-		retval = status == -ENOENT ? -ETIMEDOUT : status;
 	} else
-		retval = status;
+		retval = ctx.status;
 out:
 	if (actual_length)
 		*actual_length = urb->actual_length;


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel