Hi,
"Andrew P. Lentvorski" <bsder@xxxxxxxxxxx> writes:
>>>>>> When I set my gadget up on my Beaglebone Black (uname -a: Linux
>>>>>> beaglebone 4.14.108-ti-r113 #1 SMP PREEMPT Wed Jul 31 00:01:10 UTC
>>>>>> 2019 armv7l GNU/Linux).
>>>>>>
>>>>>> I get (output from lsusb):
>>>>>>
>>>>>> iInterface 5 HID Interface
>>>>>>
>>>>>>
>>>>>> But I want it to be something like:
>>>>>>
>>>>>> iInterface 4 LPC-LINK2 CMSIS-DAP V5.224
>>
>> Why? Oh, you want your beaglebone to behave as a CMSIS-DAP to trick some
>> other SW?
>>
>> Do we need to support that upstream, though? It seems like this is a
>> one-off thing. Does anybody else need to configure interface string
>> descriptor?
>
> I ... don't know, actually. That's probably a good question.
>
> What I *can* tell you is that a quick survey of the various USB devices
> in my office with lsusb shows the majority of iInterface are 0x00, but
> that everything else is a smattering of all manner of stuff: "Integrated
> Camera", "Bulk Control Interface", "Volume Control", "Firmware Upgrade
> Port", etc.
>
> Microchip's PIC32 USB HID sample code sets the value to 0x00.
>
> USB Complete (5th Edition) also shows this as 0x00 on page 267 for a
> configuration_descriptor for a vendor-specific HID.
>
>
>>> C) This is an actual bug *that should be fixed* and isn't that way
>>> intentionally for some Linux reason.
>>
>> Up until now, it has been intentional. Currently, I don't see a need to
>> change it.
>
> Clearly, some HID devices *are* using this field. So, this probably
> should be made changeable if we want the HID gadget to be maximally useful.
>
> As for the technical details I can see from the code:
>
>
> A bit of code archaeology shows that .iInterface was expected to be
> dynamic as of the origin commit for f_hid.c (commit
> 71adf118946957839a13aa4d1094183e05c6c094). This still persists in the
> file today.
What you're missing, though, is that iInterface is merely an *index*
into an array of strings. What's dynamic is the index, not the string
itself.
The reason for this is that we want to build different gadgets by
combining the different f_* functions in different ways which will, in
turn, change the location of $this function's strings in that array.
> The allocation of usb_gstrings_attach comes in at Nov 6, 2014 as of
> (commit 5ca8d3ec9970f4798e68bd21a9d44db3d0ff4da7) with the message:
>
> "Before configfs is integrated the usb_gstrings_attach() interface
> must be used."
>
> This looks like a case of something that simply got lost in the shuffle
> in the upgrade path to configfs, and I'm just the first poor slob who
> tripped over the oversight.
>
>
> However, I'm a little concerned as to why this was set *at all* (which
> seems to be what is actually tripping me up), and I'd be interested as
> to what drove the specific choice of 0x05 "HID Interface". I don't see
> this in any spec anywhere obvious and this seems to be a deliberate
> change from previous behavior.
why? No behavior changed. Look at the very first commit on
f_hid.c. commit 71adf118946957839a13aa4d1094183e05c6c094 contains the
following:
+static struct usb_string ct_func_string_defs[] = {
+ [CT_FUNC_HID_IDX].s = "HID Interface",
+ {}, /* end of list */
+};
+
+static struct usb_gadget_strings ct_func_string_table = {
+ .language = 0x0409, /* en-US */
+ .strings = ct_func_string_defs,
+};
+
+static struct usb_gadget_strings *ct_func_strings[] = {
+ &ct_func_string_table,
+ NULL,
+};
The string has *always* been "HID Interface"
> If this isn't user configurable, then it's probably a highly
> questionable choice to give this any default value other than 0x00.
why?
> This would have been quite a bit easier to code, too, so I'm *really*
> scratching my head about this.
what would've been easier to code?
>> I may be persuaded otherwise, but I need to see arguments
>> other than "I want to trick some SW into thinking I'm something else".
>
> Obviously I'm biased and scratching my own itch, but why would you
> consider that not to be an important argument?
Because you're asking me to accept code that will be used only by you
for, perhaps, a few weeks while you make (possibly) openocd think your
device is a CMSIS-DAP. If that's the only usecase, you can modify the
string in your own kernel tree. If it's a one-off thing, we don't need
that code in upstream.
Now, if there's a real usecase for this. Something that can attract, as
per Dave B., 3 or more users, then we can consider adding it
upstream. Remember that if we add support for changing interface
strings, it has to be implemented for *all* functions and validated on
all functions, then properly documented as a configfs ABI which can
never change anymore.
> Being able to fake being a particular piece of USB hardware that's
> currently tied to some ancient Windows binary driver is a great way to
> insert Linux into industrial and lab control pipelines. Giving a
fair enough, but your making a claim that help you and only you :-)
> control board the ability to now also be accessed via ethernet or
> wireless (or even a better USB protocol) and thus now has an upgrade or
> higher performance path is a *really* useful thing. And the Beaglebone
> Black is a really good "protocol engine". Finally, after making the usb
> gadget emulation work, I can probably blow a bunch of Windows machines
> away completely since something like a Beaglebone Black is more than
> sufficient to handle the control without any outside intervention.
You're getting to a point where things start to get interesting. What
exactly are you trying to do?
> My end target isn't "CMSIS-DAP"--there are a zillion really cheap
> CMSIS-DAP devices so emulating CMSIS-DAP would be mostly pointless
> pedantry (although perhaps a good usb gadget tutorial). CMSIS-DAP just
> happens to be a really good test case as it is an extremely well
> documented HID-based protocol and has extensive tests to validate it.
> Consequently, I can be sure that most bugs and difficulties are
> localized to my gadget implementation code or the gadget driver.
Fair enough
> Anyhow, let me know whether I should attack the problem or not. I
I can't prevent you from working on it, I just think that:
a) perhaps you haven't considered all the intricacies around supporting
this upstream (hint: you can't implement for f_hid alone)
b) you haven't convinced me that this has the potential for more than
one user
c) you haven't considered the extra that comes with making this into an
ABI
> suspect my biggest issue is simply that I will have to do all the work
> on a Beaglebone Black as I really don't know of any other way to test
> that gadget subsystem. This will probably be okay if I can build it as
you could use dummy_hcd as well.
> a module--if I have to do a full kernel build it will probably demand
> that I set up a cross-compiling environment (and that isn't trivial).
cross-compilation for the kernel is trivial in most distributions. Just
install arm-linux-gnueabihf toolchain which should already be packaged
for you.
--
balbi
Attachment:
signature.asc
Description: PGP signature
