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 <[email protected]> > > 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 _______________________________________________ devel mailing list [email protected] https://lists.openmoko.org/mailman/listinfo/devel
