vpelletier / python-functionfs

Pythonic API for linux's functionfs
GNU General Public License v3.0
40 stars 13 forks source link

HID descriptors? #11

Closed ali1234 closed 3 years ago

ali1234 commented 5 years ago

I'm trying to create a HID device through functionfs. I have it registering and sending the HID Device Descriptor, but later when the host asks for the HID Report Descriptor... well, I don't see any way to send it. Does functionfs even support this? The documentation specifically gives HID as a hypothetical use-case along with MTP.

I got your library to send the HID Device Descriptor by defining a new descriptor class:

class USBHIDDescriptor(functionfs.USBDescriptorHeader):
    """
    USB_DT_HID: HID descriptor
    """
    _bDescriptorType = 33
    _fields_ = [
        ('bcdHID', le16),
        ('bCountryCode', u8),
        ('bNumDescriptors', u8),
        ('bDescriptorxType', u8),
        ('wDescriptorLength', le16),
    ]

And then putting one of these in between the interface descriptor and the endpoint descriptors in the fs_list. But I don't know where I am supposed to put the report descriptor, and there is no documentation. :(

ali1234 commented 5 years ago

Never mind, I figured it out. I wasn't starting the async loop so I never received onSetup() events. Fixed that and I can now send the report descriptor when the host asks for it.

That said, what do you think about adding support for HID descriptors?

vpelletier commented 5 years ago

But I don't know where I am supposed to put the report descriptor, and there is no documentation. :(

python-functionfs wraps descriptor serialisation and event handling. The rest (what to put in descriptors, how to organise descriptor) depends on developer intent. It is constrained by what descriptor types the kernel supports (look for ffs_do_single_desc) and guided by the USB spec if a compliant device is expected (maybe the goal is fuzzing a host, so maybe implementation voluntarily deviates from spec). Hence the absence of documentation at python-functionfs level.

In the case of HID, HID spec 1.11 defines HID-specific descriptor structure in chapter 6.2, order of generic descriptors in chapter 7.1, and protocol to retrieve specific descriptors in chapter 7 (especially 7.1.1 Get_Descriptor request). In my understanding (I did not implement a USB gadget with functionfs, nor interface with a HID device from host side - at least not at a low enough level to have to care about these details), an HID descriptor (6.2.1) the only descriptor you need to provide to functionfs as part of the initialisation. All further descriptors must be provided on-demand by implementing onSetup method. 7.1.1 Get_Descriptor Request specifies the values to expect for this, and how each should behave.

That said, what do you think about adding support for HID descriptors?

Could be nice, yes, with a definition of descriptor structures (BTW, the one you quoted above only allows one pair of type & length at the end, while spec describes an array of them of length >=1 - which incidentally will help with the field naming conflict with standard USB descriptor header) a compliant implementation of onSetup intercepting standard requests and dispatching them to separate methods for easy subclassing and implementing the descriptor lookup logic (so subclass only have to declare their descriptors and not how they are to be looked up).

I just implemented a crude and not-immediately-functional proof-of-concept in the new hid branch (a lot more thought must be put in __init__) to give you an idea of how I would implement this at python-functionfs level. I will push-force in this branch if I get around to finish such implementation. Patches welcome.

ali1234 commented 5 years ago

For reference this is what I came up with:

https://github.com/ali1234/switch-controller/blob/master/switchcon/functionfs/function.py

It uses a lot of hard coded magic values that aren't defined in python-functionfs because I didn't want to look up the names of them... it also probably fails to handle many of the things the spec says it should, but it works despite this.

vpelletier commented 5 years ago

it also probably fails to handle many of the things the spec says it should, but it works despite this.

I believe that this is the reason why USB devices of all kinds are so often HID devices.

I'm impressed that you implemented onSetup the way I intended. While I was implementing my PoC earlier I thought this API was easy to misuse (by not falling back on superclass'). Maybe it is not so obscure after all...

vpelletier commented 5 years ago

I updated hid branch, it should be functional now. Thanks for the example code.

The next step, if it actually happens, would be to automate generation of REPORT and PHYSICAL descriptors. The spec is hairy, so the API in its current state expects user to bring their own byte arrays.

vpelletier commented 5 years ago

I did some extensive changes in hid branch, which should simplify gadget writing: I finally figured an API for using AIO and non-blocking files everywhere. Could you take a look at the result and give me you opinion ?

Sadly, the HID demo still does not work for me yet: kernel rejects my descriptors, and as is (sadly) usual it is very secretive about what is wrong. I still haven't figured it out.

vpelletier commented 5 years ago

Found it, and the example now works. It's of course a very stupid example (fake being a mouse moving constantly to the right by the smallest increment possible) but I'm satisfied by how short it is. Next I will need feedback from someone familiar with real devices (wink wink, nudge nudge).

I'm not too satisfied by the HID report descriptor blob, but generating these looks like it will be taking a significant amount of effort. And it may even belong to a separate module.

ali1234 commented 5 years ago

Well on the topic of separate modules... I managed to port my HID code over to gadgetfs instead of functionfs: https://github.com/ali1234/switch-controller/blob/master/switchcon/gadgetfs/gadget.py

Not many changes were needed. The descriptors are pushed in a slightly different order, plus you need to send the device descriptors. The ch9.py was very useful as it has the device descriptor despite functionfs not seeming to need it, and I was able to extend onSetup with the extra requests similar to how it is done for HID.

A few thoughts:

I think function should be a pure context manager. It would simplify some things if I could make an object but not have it start up until later.

I find it difficult to understand your AIO code (and really ctypes in general, despite knowing C.) I made a wrapper that works on any file. You've seen before and it hasn't really changed, but here it is again for reference: https://github.com/ali1234/switch-controller/blob/master/switchcon/kaio.py

I suspect your AIO code is a lot more robust than mine, which I am not totally convinced is not leaking and/or using memory after it has been freed.

I keep hearing that cffi should be used instead of ctypes. And I have had a lot of success using construct for building structured data in my previous pymtpd work which now supports most of MTP.

It looks like the project I'm currently working on is going to end up sticking with an Arduino as the USB device and sending the data to it over serial. This seems a lot more reliable that a Linux gadget. I have had no end of problems with the dwc2 UDC code - see https://github.com/raspberrypi/linux/issues/2684 and with dummy_hcd, any mistake tends to lock up the whole host USB stack and force a hard reboot.

An interesting thought I had is making the Arduino accept the device descriptors at run time over serial, similar to how functionfs and gadgetfs do. Maybe it would even be possible to make the kernel see it as a generic UDC? Simply talking to it directly over serial seems a lot less error prone though.

vpelletier commented 5 years ago

(sorry, this became a monster post...)

I managed to port my HID code over to gadgetfs instead of functionfs

I did not study gadgetfs. As it lives in drivers/usb/gadget/legacy I guessed it should be less powerful.

As you mention needing a device descriptor, I suppose that FS handles whole-devices instead of just separate functions ?

This risk bringing a lot of complexity from speed-specific behaviours (not to mention all the device requests which need to be served). I'm already not too happy that I need to semi-hardcode max packet lengths, but have no idea how the kernel could handle this on its own either.

OTOH, it may allow fuzzing more host features, which I like. I try to keep python-functionfs open to non-standard cases, while automating standard cases.

I think function should be a pure context manager. It would simplify some things if I could make an object but not have it start up until later.

So store parameters during __init__, but actually open ep0 to push descriptors only in __enter__ ? This makes a lot of sense indeed.

I find it difficult to understand your AIO code (and really ctypes in general, despite knowing C.)

In my experience, here are the hard things with ctypes:

What I can live with but is sometimes pointed as a ctypes issue:

I made a wrapper that works on any file. You've seen before and it hasn't really changed, but here it is again for reference: https://github.com/ali1234/switch-controller/blob/master/switchcon/kaio.py

You are creating one context per file, this seems suboptimal although I have not seen it being dicouraged. The bigger issue may be that KAIOWriter is requesting 12k aio slots... My kernel has an (apparent, I did not try to reach it) hard limit at 64k. Those 12k may make a large dent and prevent other software packages from running (although very few use AIO, and those which do seem not too relevant to a gadget: mysql/mariadb, ioping...).

And as I said, leak avoidance: if close is not carefully called you will leak AIO contexts. I tend to not trust python developpers to be careful on this (and why would they, garbage collection does it for them on 100% of standard objects) so I prefer to go the extra mile in the code and not get bug reports about user-self-inflicted leaks.

And the other side of this GC coin: early destruction. iocb and iocpbptr going out of scope destroys them, and while it works in this code, it limits what you can do on transfer completion: kernel could at best call into a ctypes callback (which forces the user to touch ctype instances, which I try to avoid, except when user can treat them as opaque values). Such callback would then have very little context to give to the developer: this transfer that failed, what was it ? This is why python-libaio keeps track of in-flight transfers. If "buf" gets out of scope I think there will be bigger problems. I expect it is kept alive by caller, but eventually garbage may happen.

FYI, pypy was unexpectedly helpful at finding ctypes misuses when I developped python-libusb1. For whatever reason (their different approach to garbage collection ?) things go down spectacularly while cpython would be more subtle (little corruption here and there). And AFAIR, it was always ultimately my fault.

If you could point obscure points in python-libaio maybe I can explain - or I can realise it's really not how it should be done :) .

in my previous pymtpd work which now supports most of MTP.

I am going off-topic, but I have an android phone with which kde's mtp is failing very hard... It takes tens of minutes of unplugging/plugging/"give access as MTP"/"device actions -> browse" before I can maybe copy one picture. I can usually list the tree fine, but anything past that fails much more often than not. I should read your code to learn about this protocol and maybe eventually understand what kde's mtp connector needs to do.

I have had no end of problems with the dwc2 UDC code

And I have had also my share of issues with its brother, dwc3 (sadly not working as USB3 because it lacks needed companion chip on the intel Edison board). I believe the whole gadget subsystem needs some love - and I kind of hope writing python wrappers may help with this by giving it more visibility. And the pi zero would be a much more affordable gadget than (discontinued) Edison boards. It's disappointing to hear it also has such issues (I have one, but never been able to build a working-enough kernel for it. Also, the Edison has built-in on-line battery charger circuitry which I believe it impossible to achieve on any rpi).

ali1234 commented 5 years ago

As you mention needing a device descriptor, I suppose that FS handles whole-devices instead of just separate functions ?

So, under functionfs you write the device parameters into the sysfs thing (or put in in the board file or whatever) and then open a virtual ep0 and send the config and endpoint descriptors. Then you open virtual endpoints and read/write them.

Under gadgetfs you open the real ep0 and write the device descriptors, then you open the real endpoints and write the endpoint descriptors to them. After that's all done you end up in the same place: you have some endpoint files that you read and write, and an ep0 where you have to handle certain requests. The only difference is that under gadgetfs you have to handle a couple more request types on ep0. edit: specifically the config request, which functionfs handles automatically, and it is your signal to open the other endpoints and configure them.

So store parameters during __init__, but actually open ep0 to push descriptors only in __enter__ ?

Yes, exactly. This would be useful specifically in my gadget wrapper (sets up the sysfs stuff). Currently I pass in a function that creates a Function object and returns it, so that I can call it later on in my __enter__. It would be much nicer to just pass in a Function object directly and then pass through the __enter__ call.

KAIOWriter is requesting 12k aio slots

Well spotted. That is actually a piece of forgotten debug code. When I was trying to handle mediating between inotify on a directory tree and MTP notifications I kept filling up the queue so I made it huge to test something and then forgot to put it back. For the HID code one slot would be enough and pymtpd probably only needs like 10 at the very most.

ali1234 commented 5 years ago

Oh and about MTP protocol: Its another one of those "standards" that nobody implements properly (especially Microsoft) and it isn't particularly well designed to begin with. It is a hack on top of an older protocol too. All the Linux GUI tools use libmtp ultimately so they all work equally well. And libmtp has a huge list of quirks for phones that don't implement it properly. It's very likely that yours needs some workaround. libmtp comes with a large suite of test utilities that can probably tell you what is happening.

vpelletier commented 5 years ago

Yes, exactly. This would be useful specifically in my gadget wrapper (sets up the sysfs stuff). Currently I pass in a function that creates a Function object and returns it, so that I can call it later on in my enter. It would be much nicer to just pass in a Function object directly and then pass through the enter call.

Implemented - but not tested yet - in hid branch. It requires latest python-libaio master: I added support for modifying a created AIO block. The reason I did not enable it before was to avoid freeing buffer memory too early. Instead, AIOContext now keeps a strong reference to buffer & buffer list objects. Also, it accepts not setting target_file in its constructor, allowing to set it in __enter__ when endpoint files are actually opened.

libmtp comes with a large suite of test utilities that can probably tell you what is happening.

Thanks, I'll run it and report the quirks found to libmtp.

vpelletier commented 5 years ago

Now it's tested and works fine for me. I am rater satisfied with the API, although I did not develop very complex gadgets yet. Especially I did not try to integrate with configfs: my intended use-case would set configfs up using root access at device boot, and then start functions as normal user services.

If you spot issues in the API, please tell me. Otherwise, I think current hid branch should be close to become master, and then a new release (along with a new python-libaio release).

vpelletier commented 3 years ago

My HID work is now in master, as is configfs integration. Not released yet, though.

vpelletier commented 3 years ago

Released as 0.5 .