Date: Wed, 1 Jul 2015 09:13:42 -0700
From: Greg KH <greg@kroah.com>
To: Jeremy White <jwhite@codeweavers.com>
Cc: hdegoede@redhat.com, spice-devel@lists.freedesktop.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect
 USB devices over IP.
Message-ID: <20150701161342.GB31389@kroah.com>
References: <1435700650-640-1-git-send-email-jwhite@codeweavers.com>
 <1435700650-640-2-git-send-email-jwhite@codeweavers.com>
 <20150630234805.GB13573@kroah.com>
 <55935FC1.8090203@codeweavers.com>
 <20150701054416.GA23370@kroah.com>
 <55940D85.9030702@codeweavers.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <55940D85.9030702@codeweavers.com>
User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5282
Lines: 111

On Wed, Jul 01, 2015 at 10:55:49AM -0500, Jeremy White wrote:
> 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.

Because no one has cared to.  Now it seems you care, so I'd prefer to
see someone fix this up instead of adding another protocol that does
much the same thing.

> >>   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).

style matters, as it's a thing with your brain.  You learn patterns and
if the patterns change, you have to do more work and don't see the real
issues involved.  So by ignoring our style you are saying you don't want
anyone else in the kernel community to ever review or work on the code,
which isn't ok.

> >>> 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.

I personally ignore RFC patches unless they are tiny and in an area that
I happened to be worried about / working on at the moment as it feels
that the submitter doesn't think they are good enough to be merged
as-is, so I'll wait until they feel it is worthy for submission to
review it.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/