don't use eventfd for usb_f_fs ep0

DBlauhut commented 2 years ago

Hello, this patch works for rockchips rk3399 kernel 4.19, i don't get the eventfd_ctx_put related kernel panic anymore. avoiding the use of the eventfd for ep0 should make python-functionfs useable for all kernels < 5.10. :)

DBlauhut commented 2 years ago

i have no clue how to link the proper issue to this pull request

vpelletier commented 2 years ago

No worry about the bug linking, I am not a fan of the automation behind it (closing a bug when a merge request is applied) anyway.

I am not through with my thoughts and checks on this, but here is a snapshot before I fall asleep:

a negative effect of this change is that code using this class, if they implement their own event handler, they will now miss ep0 events if they only monitor the eventfd
...but even then, this is not what I document in processEvents: it is supposed to be called also if ep0 becomes readable.
...but then again, if possible I would rather let the code outside this module tell my code what it is ready to handle, giving control on whether eventfd is given to f_fs and hence the effect on an external event loop. But I have no idea how to make such thing in a reasonable way: a constructor parameter seem (to my current superficial thinking anyway) would look very ugly. Maybe a class attribute ?...
I need to check how processEvent reacts with this change: wouldn't self._eventfd.read() raise IOError(EAGAIN) if f_fs does not have an eventfd to signal ?

DBlauhut commented 2 years ago

I can understand all your points. i only tried functionfs.tests.* to evaluate my changes.

i had the idea to use libaio for ep0 too. Than ep0 may use the same eventfd, but it's a bigger change in the code, because all synchron reads and writes to ep0 have to be changed and i had not enough time yet to do and test it.

vpelletier commented 2 years ago

So far, I could test in one of my application (my openpgp smartcard implementation), taking care to poll ep0. I was surprised that reading eventfd did not raise, and I do not understand why yet (the manpage indicates it should).

i had the idea to use libaio for ep0 too. Than ep0 may use the same eventfd, but it's a bigger change in the code, because all synchron reads and writes to ep0 have to be changed and i had not enough time yet to do and test it.

I was wondering about this, and had the same impression: it is a lot of effort, and it seems likely to have consequences to any code using python-functionfs: any function implementing control transactions has to be able to read setup request payload and to send responses, so the endpoint becoming non-blocking would have a visible effect.

For now, my feeling is that I prefer a minimalist approach: anyone who can get a fixed kernel can just keep using my module as it is, and anyone wanting/having to run on an older, buggy kernel would be able to enable some "quirk mode".

vpelletier commented 2 years ago

Replying to myself, having re-read the code:

any function implementing control transactions has to be able to read setup request payload and to send responses, so the endpoint becoming non-blocking would have a visible effect.

...except this has nothing to do with being non-blocking, and ep0 is already non-blocking.

So I believe a quirks-mode AIO codepath with eventfd is possible. I did a proof-of-concept (pushed in the temporary f_fs_eventfd_workaround branch) which seems to be working. It is not final code yet, especially as there is no known-good kernel version. I also tried pushing further with AIO: instead of a POLL block, using a READ block, which should bring the number of syscalls the same as the current event handling codepath. But this change fails (io_submit returns error 22), and I seem to remember EP0 does not support AIO read/write operations.

vpelletier commented 2 years ago

Some updates:

I checked the kernel code, and indeed ep0 does not support AIO. For reference: it implements .read and .write members of struct file_operations, while AIO support requires .read_iter and .write_iter. I suspect this is because of how many "personalities" EP0 has (write descriptors, then receive events, and after SETUP-type events read or write the data state of the setup transaction...) so queuing IO in advance is non-trivial.

I received the notification yesterday that my fix (its v2 anyway) has been pulled by GregKH.

I polished the compatibility commit some more, and I think the main missing thing is an actual known-good kernel version.

I also did some more-or-less related cleanups and improvements to the code, already pushed in master. [EDIT]: The most visible one may be the functionfs/tests/*.py changes, which on my pi zero W improved my crude bandwidth measurements, reaching ~40MB/s for IN transactions and ~20MB/s for OUT transactions.

Testing (especially of the f_fs_eventfd_workaround branch) is very welcome.

vpelletier commented 2 years ago

I applied my fix on master, which should supersede this version.

vpelletier / python-functionfs

don't use eventfd for usb_f_fs ep0 #24