[PATCH] Look for DFU functional descriptor in the configuration descriptors
Stefan Schmidt
stefan at datenfreihafen.org
Mon Nov 8 12:21:15 CET 2010
Hello.
First of all thanks for your patches to dfu-util. I'm not reading the
openmoko-devel list that often anymore so a explicit cc might help getting me to
see the patches earlier.
On Fri, 2010-11-05 at 22:32, Tormod Volden wrote:
> From: Tormod Volden <debian.tormod at gmail.com>
>
> Some devices are not able to return non-standard type descriptors when
> asked explicitly, but will send them with the configuration descriptors.
I prefer a commit message starting with the area or source file it touches to
give the commit message a context.
e.g. main: Look for DFU functional descriptor in the configuration descriptors
> Hi, I am looking at the dfu-util code and trying to make it work with
> some devices I have (under Linux).
What devices do you work with? I would like to compile a list with devices
dfu-util can work with or has known problems.
> One issue with these devices is
> that the usb_get_descriptor(dif->dev_handle, 0x21, ...) in main()
> fails so that the transfer_size can not be deducted from the dfu
> functional descriptor (type=0x21). I have confirmed that the device
> firmware is not able to respond to a get_descriptor request of type
> 0x21, but only of the device, config and string types. For reference
> I checked the Maple bootloader code, and they have explicitly added
> support for the 0x21 type.
>
> However, when I read section 9.4.3 of the USB 1.1 specification I am
> not sure that my devices are to blame:
> "The standard request to a device supports three types of descriptors:
> DEVICE, CONFIGURATION, and STRING." and further "Class-specific and/or
> vendor-specific descriptors follow the standard descriptors they
> extend or modify."
Such things could also go into the commit message. I don't mind if the message
is a bit longer as long as the problem and solution is well descripted.
> I have checked that the functional descriptor is returned together
> with the configuration descriptor as required by the specification (as
> I understand it). So should dfu-util look for it there? This patch
> makes it look for it there first, and if it is not found, it will do
> the direct type=0x21 request like before.
Sounds like a good plan to me.
> Actually, libusb keeps track of all descriptors found during its
> enumeration, and it is possible to pull out these "extra" descriptors
> without sending new requests to the device. The libusb 0.1 code
> stuffs the 0x21 descriptor in the "extra" field of one of the interface
> altsettings. However I found it in altsetting[1] while I expected
> it to be in altsetting[0], so either there is a libusb bug or I do not
> understand it well enough.
>
> I think also that libusb should be fixed/enhanced to answer
> usb_get_descriptor calls by looking up its cached info rather than
> sending repeated requests to the device. But that will definitely
> not happen in the no-longer maintained libusb 0.1. So IMO we will
> have to make these workarounds until 1) it is fixed in libusb 1.0
> and 2) we have ported dfu-util to libusb 1.0.
I can life with this workaround. And if anybody is interested in porting it over
to libusb 1.0 let me know. :)
> diff --git a/src/main.c b/src/main.c
> index fd373fc..9d0b72a 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -368,6 +368,33 @@ static int resolve_device_path(struct dfu_if *dif)
>
> #endif /* !HAVE_USBPATH_H */
>
> +/* Look for descriptor in the configuration descriptor output */
> +static int usb_get_extra_descriptor(usb_dev_handle *udev, unsigned char type,
> + unsigned char index, void *resbuf, int size)
> +{
> + char *cbuf;
> + int desclen, conflen, smallest;
> + int p = 0;
> + int ret = 0;
Do we really need the initial set to 0 here?
> +
> + conflen = (usb_device(udev))->config->wTotalLength;
> + cbuf = malloc(conflen);
> + conflen = usb_get_descriptor(udev, USB_DT_CONFIG, index, cbuf, conflen);
> + while (p + 1 < conflen) {
> + desclen = (int) cbuf[p];
> + if (cbuf[p + 1] == type) {
> + smallest = desclen < size ? desclen : size;
> + memcpy(resbuf, &cbuf[p], smallest);
> + ret = smallest;
> + break;
> + }
> + p += desclen;
> + }
> + free(cbuf);
> + if (ret > 1) return ret;
Please use the following instead:
if (ret > 1)
return ret;
> + /* try to retrieve it through usb_get_descriptor directly */
> + return usb_get_descriptor(udev, type, index, resbuf, size);
> +}
>
> static void help(void)
> {
> @@ -790,7 +817,7 @@ status_again:
>
> if (!transfer_size) {
> /* Obtain DFU functional descriptor */
> - ret = usb_get_descriptor(dif->dev_handle, 0x21, dif->interface,
> + ret = usb_get_extra_descriptor(dif->dev_handle, USB_DT_DFU, dif->interface,
> &func_dfu, sizeof(func_dfu));
You also changed it form a magical value to the DEFINE we already have. Thanks.
If you take into account the comments above I'll happily apply this patch. I
already tested the current version with my Openmoko Freerunner and it does not
break anything.
regards
Stefan Schmidt
More information about the devel
mailing list