Patchwork Bug: VHCI + USB 3.0

login
register
mail settings
Submitter Alan Stern
Date April 10, 2019, 2:52 p.m.
Message ID <Pine.LNX.4.44L0.1904101047230.1629-100000@iolanthe.rowland.org>
Download mbox | patch
Permalink /patch/769871/
State New
Headers show

Comments

Alan Stern - April 10, 2019, 2:52 p.m.
On Wed, 10 Apr 2019, Bollinger, Seth wrote:

> > On Apr 9, 2019, at 2:13 PM, Bollinger, Seth <Seth.Bollinger@digi.com<mailto:Seth.Bollinger@digi.com>> wrote:
> >
> > > Seth, you can try adding:
> > >
> > >  blk_queue_virt_boundary(sdev->request_queue, 1024 - 1)
> > >
> > > in drivers/usb/storage/scsiglue.c:slave_alloc(), just after the call to
> > > blk_queue_update_dma_alignment().  Perhaps that will help
> > >
> >
> > I will do that as a test.  However, I’m concerned that we’re solving only a specific case.  Won’t it fail in a similar way if a different subsystem does the same thing (USB modem or > video drivers, etc.)?
> 
> Just to follow up, this does seem to solve the specific problem.

Great!  Okay, here's an actual patch for you to try.  I have removed 
the blk_queue_update_dma_alignment() call because it doesn't seem to 
be necessary.  The starting location of each scatterlist element is 
unimportant; only the length matters.

If this works, I will submit it as a bugfix for the kernel.

Alan Stern


 drivers/usb/storage/scsiglue.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)
Alan Stern - April 12, 2019, 6:01 p.m.
On Wed, 10 Apr 2019, Alan Stern wrote:

> On Wed, 10 Apr 2019, Bollinger, Seth wrote:
> 
> > > On Apr 9, 2019, at 2:13 PM, Bollinger, Seth <Seth.Bollinger@digi.com<mailto:Seth.Bollinger@digi.com>> wrote:
> > >
> > > > Seth, you can try adding:
> > > >
> > > >  blk_queue_virt_boundary(sdev->request_queue, 1024 - 1)
> > > >
> > > > in drivers/usb/storage/scsiglue.c:slave_alloc(), just after the call to
> > > > blk_queue_update_dma_alignment().  Perhaps that will help
> > > >
> > >
> > > I will do that as a test.  However, I’m concerned that we’re solving only a specific case.  Won’t it fail in a similar way if a different subsystem does the same thing (USB modem or > video drivers, etc.)?
> > 
> > Just to follow up, this does seem to solve the specific problem.
> 
> Great!  Okay, here's an actual patch for you to try.  I have removed 
> the blk_queue_update_dma_alignment() call because it doesn't seem to 
> be necessary.  The starting location of each scatterlist element is 
> unimportant; only the length matters.
> 
> If this works, I will submit it as a bugfix for the kernel.

Any more test results?  I need to know that you're confident the patch 
works okay.

Alan Stern

PS: I changed my mind and decided to keep the
blk_queue_update_dma_alignment() call.  It probably doesn't hurt (the
data buffers are most likely already aligned) and some host controllers
may have alignment requirements.
Bollinger, Seth - April 13, 2019, 11:37 a.m.
> On Apr 12, 2019, at 1:01 PM, Alan Stern <stern@rowland.harvard.edu<mailto:stern@rowland.harvard.edu>> wrote:

>

> Any more test results?  I need to know that you're confident the patch

> works okay


The patch didn’t apply for me.  I figured a copy paste problem, so I made the changes by hand.

Your changes fix my problem.

If it’s not too much to ask, please send me a message if you find patches that fix similar issues with SG and vhci.  :)  I’m sure we’ll run into those problems if they exist.

> Alan Stern

>

> PS: I changed my mind and decided to keep the

> blk_queue_update_dma_alignment() call.  It probably doesn't hurt (the

> data buffers are most likely already aligned) and some host controllers

> may have alignment requirements.


Ok, I’ll add that call back.

Thanks!

Seth

Patch

Index: usb-4.x/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-4.x.orig/drivers/usb/storage/scsiglue.c
+++ usb-4.x/drivers/usb/storage/scsiglue.c
@@ -65,6 +65,7 @@  static const char* host_info(struct Scsi
 static int slave_alloc (struct scsi_device *sdev)
 {
 	struct us_data *us = host_to_us(sdev->host);
+	int maxp;
 
 	/*
 	 * Set the INQUIRY transfer length to 36.  We don't use any of
@@ -74,22 +75,16 @@  static int slave_alloc (struct scsi_devi
 	sdev->inquiry_len = 36;
 
 	/*
-	 * USB has unusual DMA-alignment requirements: Although the
-	 * starting address of each scatter-gather element doesn't matter,
+	 * USB has unusual scatter-gather requirements: Although the
+	 * starting address of each scatterlist element doesn't matter,
 	 * the length of each element except the last must be divisible
-	 * by the Bulk maxpacket value.  There's currently no way to
-	 * express this by block-layer constraints, so we'll cop out
-	 * and simply require addresses to be aligned at 512-byte
-	 * boundaries.  This is okay since most block I/O involves
-	 * hardware sectors that are multiples of 512 bytes in length,
-	 * and since host controllers up through USB 2.0 have maxpacket
-	 * values no larger than 512.
+	 * by the Bulk maxpacket value.  Fortunately this value is always
+	 * a power of 2.
 	 *
-	 * But it doesn't suffice for Wireless USB, where Bulk maxpacket
-	 * values can be as large as 2048.  To make that work properly
-	 * will require changes to the block layer.
+	 * Inform the block layer about this requirement.
 	 */
-	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
+	maxp = usb_maxpacket(us->pusb_dev, us->recv_bulk_pipe, 0);
+	blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
 
 	/* Tell the SCSI layer if we know there is more than one LUN */
 	if (us->protocol == USB_PR_BULK && us->max_lun > 0)