Re: Using default LANGID = 0 causes a regression

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jul 09, 2009 at 01:35:51PM -0700, Steve Calfee wrote:
> > The device STALLS EP0 when asked for string descriptor 0, so it tells
> > it's not willing to hand out any string at all. Haven't checked with the
> > USB specs whether this is correct behaviour, but this is what it looks
> > like.
> >
> > When an endpoint communication stalls, the return value is -EPIPE, and
> > in case of a malformed string descriptor we seem to be getting -ENODATA.
> >
> > Handling those two case seperately should make both sides happy.
> >
> > Could you test the patch below? It's only compile tested for now.
> >
> > Thanks,
> > Daniel
> >
> >
> Your patch is wrong and maybe so is the current code, I haven't looked.

Yes, as Alan pointed out, there was a braino.

> 1)  String descriptors are optional, a device does not have to have any.
> 
> b)  When a device cannot handle a control command, it will return a STALL pid.
> 
> iii) When windows xp enumerates a gadget and it gets a stall on the
> string desc "0" fetch, it stops fetching device strings. Since most
> gadgets are only tested under windows, if you then request a string
> after it has told you there are no strings, some gadgets will hang.

Ok, I see. 

> I believe that the proper response after getting a STALL from a fetch
> of string desc 0, the system should flag the device as not having
> strings and fail all future requests with a STALL (EPIPE) - without
> going out on the bus. I am working with a quite old kernel and cannot
> provide a patch to the latest kernel to try this.

What the kernel did before my patch is

- upon each string read, check whether we received the LANGID already
- if not, try to get it
- bail out if that fails
- if we got one, remember the LANGID and save a flag for that, so the
  LANGID will not be retrieved a second time

The problem here is that the code was trying to unnecessarily read
LANGID over and over again, spitting an error every time.

Below is a new patch that changed the bahviour to this:

- upon the first string read, go get the LANGID descriptor
- if that fails with -EPIPE (caused by a STALL), mark the device as
  unable to respond to string descriptor request
- if that fails with -ENODATA or if the LANGID descriptor is bogus, fall
  back to the 0x0409 default
- on each subsequent string request, return -EPIPE immediately if the
  device is not able to provide strings

Let's see if that works.

I factored out this logic to a new function to make it more readable.

Thanks,
Daniel


diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 2bed83c..9720e69 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -806,6 +806,48 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid,
 	return rc;
 }
 
+static int usb_get_langid(struct usb_device *dev, unsigned char *tbuf)
+{
+	int err;
+
+	if (dev->have_langid)
+		return 0;
+
+	if (dev->string_langid < 0)
+		return -EPIPE;
+
+	err = usb_string_sub(dev, 0, 0, tbuf);
+
+	/* If the string was reported but is malformed, default to english
+	 * (0x0409) */
+	if (err == -ENODATA || (err > 0 && err < 4)) {
+		dev->string_langid = 0x0409;
+		dev->have_langid = 1;
+		dev_err(&dev->dev,
+			"string descriptor 0 malformed (err = %d), "
+			"defaulting to 0x%04x\n",
+				err, dev->string_langid);
+		return 0;
+	}
+
+	/* In case of all other errors, we assume the device is not able to
+	 * deal with strings at all. Set string_langid to -1 in order to
+	 * prevent any string to be retrieved from the device */
+	if (err < 0) {
+		dev_err(&dev->dev, "string descriptor 0 read error: %d\n",
+					err);
+		dev->string_langid = -1;
+		return -EPIPE;
+	}
+
+	/* always use the first langid listed */
+	dev->string_langid = tbuf[2] | (tbuf[3] << 8);
+	dev->have_langid = 1;
+	dev_dbg(&dev->dev, "default language 0x%04x\n",
+				dev->string_langid);
+	return 0;
+}
+
 /**
  * usb_string - returns UTF-8 version of a string descriptor
  * @dev: the device whose string descriptor is being retrieved
@@ -837,24 +879,9 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 	if (!tbuf)
 		return -ENOMEM;
 
-	/* get langid for strings if it's not yet known */
-	if (!dev->have_langid) {
-		err = usb_string_sub(dev, 0, 0, tbuf);
-		if (err < 0) {
-			dev_err(&dev->dev,
-				"string descriptor 0 read error: %d\n",
-				err);
-		} else if (err < 4) {
-			dev_err(&dev->dev, "string descriptor 0 too short\n");
-		} else {
-			dev->string_langid = tbuf[2] | (tbuf[3] << 8);
-			/* always use the first langid listed */
-			dev_dbg(&dev->dev, "default language 0x%04x\n",
-				dev->string_langid);
-		}
-
-		dev->have_langid = 1;
-	}
+	err = usb_get_langid(dev, tbuf);
+	if (err < 0)
+		goto errout;
 
 	err = usb_string_sub(dev, dev->string_langid, index, tbuf);
 	if (err < 0)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux