On Sun, 19 May 2013, Hans de Goede wrote: > Hi, Greg, Alan, All, > > On 05/18/2013 06:17 PM, Greg KH wrote: > > On Sat, May 18, 2013 at 12:14:08PM -0400, Alan Stern wrote: > >> On Sat, 18 May 2013, Hans de Goede wrote: > >>> But the sysfs descriptors file will just packs the > >>> rawdescriptors one behind the other, using > >>> usb_device->config[x].desc.wTotalLength, where as > >>> userspace only sees the length advertised by > >>> the rawdescriptors, which may be different, and when > >>> it is userspace will this have no idea where the next > >>> descriptor starts. > >>> > >>> I believe the proper way to fix this is to make the > >>> sysfs code deal with this the same way the usbfs code > >>> does (filling the holes with 0 to avoid leaking kmem), > >>> if people agree I can write a patch for this. > >> > >> That's okay with me. > > > > Sounds good to me as well. > > Thanks for the input. I've been thinking about this some-more, > and I see a possible other solution which I think we need to > consider. > > Although the config.wTotalLength value in /sys/.../descriptors > can not be trusted, the kernel does sanitize things to such a > degree that the standard descriptor header bLength can be > trusted to always be valid inside /sys/.../descriptors, so > alternatively to using config.wTotalLength userspace could > simple parse things one descriptor at a time, until it > encounters another config descriptor or EOF, and reliable figure > out where the next config descriptor starts that way. > > I'm sort of tending towards using that solution instead > > Advantages: > > 1) If libusb is modified to do thing this way it will work with > older kernels, without needing to switch to using usbfs (which > in some rough benchmarks I've run turns out to be 7 times as slow > when it comes to open file, read desc, close file). Furthermore, usbfs is not guaranteed to be present in all systems, whereas sysfs pretty much is. > 2) We're not breaking the kernel ABI, which technically my > other proposal does. I know devices with multiple configs are > rare, and one could argue the old behavior is a bug, but we > may have something out there depending on it. That's a very good point. In fact, I'd say it trumps the other considerations. > Disadvantages: > > 1) It means that there will be some inconsistencies to how this > is handled between usbfs and sysfs (note there already is such > an inconsistency with regards to the endian-ness of the device > descriptor). In light of the existing inconsistency, this doesn't bother me. People shouldn't be using the usbfs interface for cached descriptors, if they can possibly avoid it. > As said I tend towards using the alternative proposal I've > just suggested, input very much welcome! > > Either way I noticed that descriptors is absent from > Documentation/ABI/testing/sysfs-bus-usb > > Once we've a decision on which way to go, I'll document the > behavior there. I'd say keep it the way it is and add the Documentation file. If I were designing it from scratch, I might add a "true length" field just before the start of each descriptor. But that's not an option now. By the way, Greg, at what point does it make sense to move things from Documenation/ABI/testing to Documentation/ABI/stable? I get the impression that nothing ever makes this jump. Alan Stern -- 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
