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