On Thu, Aug 01, 2019 at 08:03:59AM -0600, shuah wrote: > On 8/1/19 12:38 AM, Suwan Kim wrote: > > On Mon, Jul 29, 2019 at 10:32:31AM -0600, shuah wrote: > > > On 7/29/19 8:52 AM, Suwan Kim wrote: > > > > Hi Shuah, > > > > > > > > On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote: > > > > > Hi Suwan, > > > > > > > > > > On 7/5/19 10:43 AM, Suwan Kim wrote: > > > > > > There are bugs on vhci with usb 3.0 storage device. Originally, vhci > > > > > > doesn't supported SG, so USB storage driver on vhci breaks SG list > > > > > > into multiple URBs and it causes error that a transfer got terminated > > > > > > too early because the transfer length for one of the URBs was not > > > > > > divisible by the maxpacket size. > > > > > > > > > > > > In this patch, vhci basically support SG and it sends each SG list > > > > > > entry to the stub driver. Then, the stub driver sees the total length > > > > > > of the buffer and allocates SG table and pages according to the total > > > > > > buffer length calling sgl_alloc(). After the stub driver receives > > > > > > completed URB, it again sends each SG list entry to vhci. > > > > > > > > > > > > If HCD of the server doesn't support SG, the stub driver breaks a > > > > > > single SG reqeust into several URBs and submit them to the server's > > > > > > HCD. When all the split URBs are completed, the stub driver > > > > > > reassembles the URBs into a single return command and sends it to > > > > > > vhci. > > > > > > > > > > > > Signed-off-by: Suwan Kim <suwan.kim027@xxxxxxxxx> > > > > > > --- > > > > > > drivers/usb/usbip/stub.h | 7 +- > > > > > > drivers/usb/usbip/stub_main.c | 52 +++++--- > > > > > > drivers/usb/usbip/stub_rx.c | 207 ++++++++++++++++++++++--------- > > > > > > drivers/usb/usbip/stub_tx.c | 108 +++++++++++----- > > > > > > drivers/usb/usbip/usbip_common.c | 60 +++++++-- > drivers/usb/usbip/vhci_hcd.c | 10 +- > > > > > > drivers/usb/usbip/vhci_tx.c | 49 ++++++-- > > > > > > 7 files changed, 372 insertions(+), 121 deletions(-) > > > > > > > > > > While you are working on v3 to fix chekpatch and other issues > > > > > I pointed out, I have more for you. > > > > > > > > > > What happens when you have mismatched server and client side? > > > > > i.e stub does and vhci doesn't and vice versa. > > > > > > > > > > Make sure to run checkpatch. Also check spelling errors in > > > > > comments and your commit log. > > > > > > > > > > I am not sure if your eeror paths are correct. Run usbip_test.sh > > > > > > > > > > tools/testing/selftests/drivers/usb/usbip > > > > > > > > I don't know what mismatch you mentioned. Are you saying > > > > "match busid table" at the end of usbip_test.sh? > > > > How does it relate to SG support of this patch? > > > > Could you tell me more about the problem situation? > > > > > > > > > > What happens when usbip_host is running a kernel without the sg > > > support and vhci_hcd does? Just make sure this works with the > > > checks for sg support status as a one of your tests for this > > > sg feature. > > > > Now I understand. Thanks for the details! > > As a result of testing, in the situation where vhci supports SG, > > but stub does not, or vice versa, usbip works normally. Moreover, > > because there is no protocol modification, there is no problem in > > communication between server and client even if the one has a kernel > > without SG support. > > > > In the case of vhci supports SG and stub doesn't, because vhci sends > > only the total length of the buffer to stub as it did before the > > patch applied, stub only needs to allocate the required length of > > buffers regardless of whether vhci supports SG or not. > > > > If stub needs to send data buffer to vhci because of IN pipe, stub > > also sends only total length of buffer as metadata and then send real > > data as vhci does. Then vhci receive data from stub and store it to > > the corresponding buffer of SG list entry. > > > > In the case of stub that supports SG, if SG buffer is requested by > > vhci, buffer is allocated by sgl_alloc(). However, stub that does > > not support SG allocates buffer using only kmalloc(). Therefore, if > > vhci supports SG and stub doesn't, stub has to allocate buffer with > > kmalloc() as much as the total length of SG buffer which is quite > > huge when vhci sends SG request, so it has big overhead in buffer > > allocation. > > > > And for the case of stub supports SG and vhci doesn't, since the > > USB storage driver checks that vhci doesn't support SG and sends > > the request to stub by splitting the SG list into multiple URBs, > > stub allocates a buffer with kmalloc() as it did before this patch. > > > > Therefore, everything works normally in a mismatch situation. > > Looking for you add a test case for that. Please include this > in the commit log. I'm confused about the test case. Do I add the test case for each SG support status of vhci_hcd and usbip_host in usbip_test.sh? Or, do I implement the test logic in vhci_hcd code that asks if usbip_host supports SG when attaching a remote device? I'm sorry but I don't know what exactly you want to add. Regards Suwan Kim
