[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