I just tested this patch and it works for me.
-jouni
On Friday 10 July 2009 01:46:36 Daniel Mack wrote:
> 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
