Refactoring for New USB API in El Capitan and later + improvements

[mikhailai]

Summary of major changes:

*** Versioning: Formalizing the X.Y versioning scheme: the current development build will be 9.0d1, and the next stable release shall be 9.0. The project's "MODULE_VERSION" setting is now the authoritative source: the rest is derived from that.

The HoRNDIS.cpp is using auto-generated "kmod_info_t kmod_info" structure to pull the version from it. It's a little hackish - this value seems to be internal - but it allows to avoid hard-coding version in the .cpp file.

Mac OS Kernel Module Versioning: https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/KEXTConcept/Articles/infoplist_keys.html Under "CFBundleVersion" section.

*** Added a "free" function: a placeholder at the moment, but a good reminder to release whatever we allocate in "init"

*** USB Probing, start, stop: NOTE: most of the code is new. Matching on the "USBHostInterface" simplifies the code by a lot: the "provider" argument is now the USBHostInterface, we no longer need to open the device and set the USB configuration - don't even need a persistent reference the device.

• "probe": make sure we have 224/1/3 and 10/?/? interfaces in a single configuration with consecutive interface numbers.
  • "start": as before, minus the device-related stuff.
  • "openUSBInterfaces": we're given "224/1/3" interface. From there, find "10/?/?" interface. Open the interfaces, get the corresponding pipes. Note, the "waitForMatchingUSBInterface", "FindNextMatchingInterface" were discarded.

*** RNDIS Initialization: The "mtu" parameter in "rndis_init" and "rndis_init_c" was a bad naming. Renamed it into "max_transfer_size" (it's actually the maximum size of USB transfer that host and device can support). Also, fixed the logic to calculate the interface MTU. Note, if the device reports max_transfer_size=1580, it is NOT necessary to limit the MTU: it shall pass the 1500-byte Ethernet payload just fine.

For reference, see: https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/WinArchive/[MS-RNDIS].pdf

*** createMediumTables: moved it to "stat" - the tables don't change anyway. We also don't need the instance dict, plus fixed a minor memory leak.

*** createWorkLoop/getWorkLoop are no longer necessary. Existing work loop + ReentryLocker take care of all the synchronization.

*** Transfers: Network -> Host:

• Increased the input buffer size 4K -> 16K. The [MS-RNDIS] spec actually recommends this size. Also, this is necessary to avoid packet drops on "Pixel 2" (looks like it violates the protocol by not obeying the "max_transfer_size" value).
• Added support for double-buffering - possible to have 2 input buffers and 2 reads queued-up (it's currently disabled: not much gain).

*** Transfers: Host -> Network. IMPORTANT: Using "IOGatedOutputQueue" instead of "IOBasicOutputQueue". This removes the last potential source of concurrency: all the method calls shall now be sequential - except for potential re-entries during "commandSleep" calls and sync USB transfers. Note, our bottleneck is USB2 bandwidth or the network, not the CPU, so serializing operations should not hurt performance, while providing way better safety.

• "outputPacket" no longer blocks when we run out of buffers (not allowed to block). It uses the queue stall mechanism to implement flow control.
• Reduced the number of output buffers 16 -> 4. Technically, we need only 2; 3 at the very most.
• Better error handling in "outputPacket".

*** enable/disable: essentially the same as before + ReentryLocker + rearranged some operations + better handling of errors in "enable".

• The "disable" function waits for all the pending read/write operations to finish before exiting (need to double-check the ordering). There is a variable "fCallbackCount" that indicates how many callbacks are currently running.

[mikhailai]

The ball is is your court now.

[jwise]

Ok, I pushed some commits to your branch. LGTM. Test the new version of the package, md5sum b6aa3bfe0bce00392eea4890bdece5c4 , and if it's good, then we'll bump the version to 9.0 and do a release? (Or did you want to block on further changes before doing a release?)

[mikhailai]

Sounds good, no I don't have any more changes for this release. Two things, though:

• I'd still take MACOSX_DEPLOYMENT_TARGET and SDKROOT out of the makefile - revert back to having these properly specified in the project. I can do the change, if you're ok with it.
• What about the rebase and merging the commits? Shall I collapse them into the fewer ones?

[jwise]

Sure, feel free to make that change, as well as rebasing and squashing. I am happy with whatever we end up with, so feel free to bump the version to 9.0, also, and I'll cook a package up when I get home.

On Jul 7, 2018, 21:02, at 21:02, Mikhail Iakhiaev notifications@github.com wrote:

Sounds good, no I don't have any more changes for this release. Two things, though:

• I'd still take MACOSX_DEPLOYMENT_TARGET and SDKROOT out of the makefile - revert back to having these properly specified in the project. I can do the change, if you're ok with it.
• What about the rebase and merging the commits? Shall I collapse them into the fewer ones?

-- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: https://github.com/jwise/HoRNDIS/pull/83#issuecomment-403260603

[mikhailai]

Aaaaaand, it does NOT run on El Cap! :(. Can't resolve "_thread_tid" symbol - though it should be there! Grrr! Looking ...