Re: [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Jeremy White <jwhite@codeweavers.com>]

On 07/01/2015 12:44 AM, Greg KH wrote:
> On Tue, Jun 30, 2015 at 10:34:25PM -0500, Jeremy White wrote:
>>   1.  Is the basic premise reasonable?  Is Hans correct in asserting that an
>> alternate USB over IP module will be considered?
> 
> I have no idea, if it fully replaces the usbip functionality, I don't
> see why that would be rejected.  But why can't you just fix up usbip for
> the issues you find lacking?

This is what Hans said 5 years ago: [1]

> 
> 3) The protocol itself is far from ideal.
> 
> Number 3 is the big deal breaker for me. I've looked at the
> (undocumented) protocol by sifting through the source. And it is
> very low level, all it does is shove usb packets back and forth
> over the network. It has no concept of configuration
> setting (the docs say make sure the device is in the right
> configuration before sharing it). No concept of caching things
> like descriptors, active configuration, per interface alt setting,
> etc.
> 
> Besides missing a lot of useful smarts the whole one packet at a
> time approach does not really fly when it comes to isoc endpoints.
> As there paper states, the vm-host / guest os drivers need to
> make sure enough packets are submitted / queued at all time
> to gap the network delay / fill the network pipe.
> 
> For iso endpoints it makes much more sense to have a start / stop
> stream model, where the usb-host "pumpes" the urb ringbuffer and
> sends out data received from the usb device to the vm-host
> (isoc input endp case), or sends data received from the vm-host
> (through a buffer to deal with network jitter) to the isoc output
> endpoint.
> 
> This also halves the number of packets which need to be
> send over the network, as their is no need for the vm-host to send
> a request for each packet once an input stream has started / for
> the usb-host to send an ack for each delivered packet for an ouput
> stream. It would still send an error when an error occurs, but their
> is no reason to ack all delivered packets. Given the delay
> caused by buffering, etc. not being able to match up the error to
> an exact packet is not important, as from the vm-host emulated usb
> hc (host controller), the packet has long been delivered already.
> 
> Instead we will simply report the error to the guest os for the
> next packet enqueued by the guest after receiving notification of
> the error from the usb-host.

The protocol is now documented, so that part is out of date.  I don't
see any evidence that the bulk of his other concerns have been
addressed, however.


> 
>>   2.  Do I correctly understand that there are no circumstances where copied
>> code can be left unmodified?  Even in the case where the copied code is
>> working, production code, and the changes would be just for style?
> 
> I doubt the changes would just be for "style" if you are craming it into
> the kernel tree, as it's a totally different environment from any other
> place this code might have been running in before.

Well, the checkpatch.pl reports were all style (and mostly whitespace);
roughly 3000 of them against 3000 lines of code :-/.  I did review the
code, looking for areas where I thought it would badly cram into the
kernel, and I adjusted the few I found (and sent changes upstream).

The ideal, of course, is to not want to copy this code at all.  Daniel
makes an alternate point that might lead to that; I'll reply to that thread.

> 
>>> Please do the most basic of polite things and fix this up before posting
>>> things.
>>
>> It is often difficult for a newcomer to know what the polite thing is, even
>> after studying FAQs and documentation.
> 
> Did you read Documentation/SubmittingPatches?

Yes, and SubmittingDrivers, SubmitChecklist, every link listed on
#kernelnewbies, and the entire lkml FAQ as well.

In hindsight, I think it's mostly a failure of common sense on my part.

The one constructive suggestion I would offer is that the 'RFC PATCH' is
used heavily by the linux kernel community, but I didn't find much
discussion of it in the documentation or FAQs.  I think I jumped to some
erroneous conclusions about it's use.  I'm willing to try to add a note
on that, if that would be helpful.

> 
> Remember you need to make this trivial to review in order to get it
> accepted.  You have to do extra work because of this because our limited
> resource is reviewers and maintainers, not developers.

Yes, understood.

Cheers,

Jeremy

[1] https://lists.gnu.org/archive/html/qemu-devel/2010-12/msg00008.html