Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754581AbbGAP4I (ORCPT ); Wed, 1 Jul 2015 11:56:08 -0400 Received: from mail.codeweavers.com ([216.251.189.131]:33691 "EHLO mail.codeweavers.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754226AbbGAPzv (ORCPT ); Wed, 1 Jul 2015 11:55:51 -0400 Message-ID: <55940D85.9030702@codeweavers.com> Date: Wed, 01 Jul 2015 10:55:49 -0500 From: Jeremy White User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Greg KH 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. 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> In-Reply-To: <20150701054416.GA23370@kroah.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4749 Lines: 109 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 -- 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/