Messages in this thread | | | Subject | Re: [PATCH v2 2/2] usbip: Implement SG support to vhci | From | shuah <> | Date | Thu, 1 Aug 2019 08:03:59 -0600 |
| |
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@gmail.com> >>>>> --- >>>>> 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 will send the v3 soon. >
Please send it soon. It would be nice to have this done as soon as possible.
thanks, -- Shuah
| |