UAS: fix alignment of scatter/gather segments

UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

This is the UAS version of

747668dbc061b3e62bc1982767a3a1f9815fcf0e
usb-storage: Set virt_boundary_mask to avoid SG overflows

We are not as likely to be vulnerable as storage, as it is unlikelier
that UAS is run over a controller without native support for SG,
but the issue exists.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/storage/uas.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6d71b8fff9df..ec9c1c7bb156 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -789,24 +789,22 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 {
 	struct uas_dev_info *devinfo =
 		(struct uas_dev_info *)sdev->host->hostdata;
+	int maxp;
 
 	sdev->hostdata = devinfo;
 
 	/*
-	 * USB has unusual DMA-alignment requirements: Although the
-	 * starting address of each scatter-gather 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.
-	 *
-	 * 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.
+	 * USB has unusual scatter-gather requirements: the length of each
+	 * scatterlist element except the last must be divisible by the
+	 * Bulk maxpacket value.  Fortunately this value is always a
+	 * power of 2.  Inform the block layer about this requirement.
+	 */
+
+	maxp = usb_maxpacket(devinfo->udev, devinfo->data_in_pipe, 0);
+	blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
+
+	/*
+	 * This one is for the controllers. We assume 512 is always good.
 	 */
 	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
 

[PATCH] UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

This is the UAS version of

747668dbc061b3e62bc1982767a3a1f9815fcf0e
usb-storage: Set virt_boundary_mask to avoid SG overflows

We are not as likely to be vulnerable as storage, as it is unlikelier
that UAS is run over a controller without native support for SG,
but the issue exists.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/storage/uas.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6d71b8fff9df..ec9c1c7bb156 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -789,24 +789,22 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 {
 	struct uas_dev_info *devinfo =
 		(struct uas_dev_info *)sdev->host->hostdata;
+	int maxp;
 
 	sdev->hostdata = devinfo;
 
 	/*
-	 * USB has unusual DMA-alignment requirements: Although the
-	 * starting address of each scatter-gather 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.
-	 *
-	 * 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.
+	 * USB has unusual scatter-gather requirements: the length of each
+	 * scatterlist element except the last must be divisible by the
+	 * Bulk maxpacket value.  Fortunately this value is always a
+	 * power of 2.  Inform the block layer about this requirement.
+	 */
+
+	maxp = usb_maxpacket(devinfo->udev, devinfo->data_in_pipe, 0);
+	blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
+
+	/*
+	 * This one is for the controllers. We assume 512 is always good.
 	 */
 	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
 
-- 
2.16.4

UAS: fix alignment of scatter/gather segments

[David Laight]

From: Oliver Neukum
> Sent: 29 April 2019 13:20
> This is the UAS version of
> 
> 747668dbc061b3e62bc1982767a3a1f9815fcf0e
> usb-storage: Set virt_boundary_mask to avoid SG overflows
> 
> We are not as likely to be vulnerable as storage, as it is unlikelier
> that UAS is run over a controller without native support for SG,
> but the issue exists.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/storage/uas.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 6d71b8fff9df..ec9c1c7bb156 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -789,24 +789,22 @@ static int uas_slave_alloc(struct scsi_device *sdev)
>  {
>  	struct uas_dev_info *devinfo =
>  		(struct uas_dev_info *)sdev->host->hostdata;
> +	int maxp;
> 
>  	sdev->hostdata = devinfo;
> 
>  	/*
> -	 * USB has unusual DMA-alignment requirements: Although the
> -	 * starting address of each scatter-gather 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.
> -	 *
> -	 * 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.
> +	 * USB has unusual scatter-gather requirements: the length of each
> +	 * scatterlist element except the last must be divisible by the
> +	 * Bulk maxpacket value.  Fortunately this value is always a
> +	 * power of 2.  Inform the block layer about this requirement.
> +	 */

That isn't the correct restriction for XHCI.
It has its own perverse restrictions.
I think they are all handled within the xhci driver.

	David


> +
> +	maxp = usb_maxpacket(devinfo->udev, devinfo->data_in_pipe, 0);
> +	blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
> +
> +	/*
> +	 * This one is for the controllers. We assume 512 is always good.
>  	 */
>  	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> 
> --
> 2.16.4

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] UAS: fix alignment of scatter/gather segments

[David Laight]

From: Oliver Neukum
> Sent: 29 April 2019 13:20
> This is the UAS version of
> 
> 747668dbc061b3e62bc1982767a3a1f9815fcf0e
> usb-storage: Set virt_boundary_mask to avoid SG overflows
> 
> We are not as likely to be vulnerable as storage, as it is unlikelier
> that UAS is run over a controller without native support for SG,
> but the issue exists.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/storage/uas.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 6d71b8fff9df..ec9c1c7bb156 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -789,24 +789,22 @@ static int uas_slave_alloc(struct scsi_device *sdev)
>  {
>  	struct uas_dev_info *devinfo =
>  		(struct uas_dev_info *)sdev->host->hostdata;
> +	int maxp;
> 
>  	sdev->hostdata = devinfo;
> 
>  	/*
> -	 * USB has unusual DMA-alignment requirements: Although the
> -	 * starting address of each scatter-gather 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.
> -	 *
> -	 * 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.
> +	 * USB has unusual scatter-gather requirements: the length of each
> +	 * scatterlist element except the last must be divisible by the
> +	 * Bulk maxpacket value.  Fortunately this value is always a
> +	 * power of 2.  Inform the block layer about this requirement.
> +	 */

That isn't the correct restriction for XHCI.
It has its own perverse restrictions.
I think they are all handled within the xhci driver.

	David


> +
> +	maxp = usb_maxpacket(devinfo->udev, devinfo->data_in_pipe, 0);
> +	blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
> +
> +	/*
> +	 * This one is for the controllers. We assume 512 is always good.
>  	 */
>  	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> 
> --
> 2.16.4

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> From: Oliver Neukum
> > 
> > +	 * USB has unusual scatter-gather requirements: the length of each
> > +	 * scatterlist element except the last must be divisible by the
> > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > +	 * power of 2.  Inform the block layer about this requirement.
> > +	 */
> 
> That isn't the correct restriction for XHCI.
> It has its own perverse restrictions.
> I think they are all handled within the xhci driver.

Yes, but that does not matter. You just cannot assume that only
XHCI will be used with UAS. In particular virtual drivers will
be used.

	Regards
		Oliver

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> From: Oliver Neukum
> > 
> > +	 * USB has unusual scatter-gather requirements: the length of each
> > +	 * scatterlist element except the last must be divisible by the
> > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > +	 * power of 2.  Inform the block layer about this requirement.
> > +	 */
> 
> That isn't the correct restriction for XHCI.
> It has its own perverse restrictions.
> I think they are all handled within the xhci driver.

Yes, but that does not matter. You just cannot assume that only
XHCI will be used with UAS. In particular virtual drivers will
be used.

	Regards
		Oliver

UAS: fix alignment of scatter/gather segments

[David Laight]

From: Oliver Neukum
> Sent: 29 April 2019 14:38
> On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> > From: Oliver Neukum
> > >
> > > +	 * USB has unusual scatter-gather requirements: the length of each
> > > +	 * scatterlist element except the last must be divisible by the
> > > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > > +	 * power of 2.  Inform the block layer about this requirement.
> > > +	 */
> >
> > That isn't the correct restriction for XHCI.
> > It has its own perverse restrictions.
> > I think they are all handled within the xhci driver.
> 
> Yes, but that does not matter. You just cannot assume that only
> XHCI will be used with UAS. In particular virtual drivers will
> be used.

True, but there is no need to enforce a 2k (IIRC) alignment for XHCI.
Perhaps you need a different property from the controller.

Even if you decide the code is 'good enough' (I don't know what the
cost is of enforcing a 2k alignment instead of 512 bytes)
the comment is just plain wrong.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] UAS: fix alignment of scatter/gather segments

[David Laight]

From: Oliver Neukum
> Sent: 29 April 2019 14:38
> On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> > From: Oliver Neukum
> > >
> > > +	 * USB has unusual scatter-gather requirements: the length of each
> > > +	 * scatterlist element except the last must be divisible by the
> > > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > > +	 * power of 2.  Inform the block layer about this requirement.
> > > +	 */
> >
> > That isn't the correct restriction for XHCI.
> > It has its own perverse restrictions.
> > I think they are all handled within the xhci driver.
> 
> Yes, but that does not matter. You just cannot assume that only
> XHCI will be used with UAS. In particular virtual drivers will
> be used.

True, but there is no need to enforce a 2k (IIRC) alignment for XHCI.
Perhaps you need a different property from the controller.

Even if you decide the code is 'good enough' (I don't know what the
cost is of enforcing a 2k alignment instead of 512 bytes)
the comment is just plain wrong.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:
> From: Oliver Neukum
> > Sent: 29 April 2019 14:38
> > On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> > > From: Oliver Neukum
> > > > 
> > > > +	 * USB has unusual scatter-gather requirements: the length of each
> > > > +	 * scatterlist element except the last must be divisible by the
> > > > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > > > +	 * power of 2.  Inform the block layer about this requirement.
> > > > +	 */
> > > 
> > > That isn't the correct restriction for XHCI.
> > > It has its own perverse restrictions.
> > > I think they are all handled within the xhci driver.
> > 
> > Yes, but that does not matter. You just cannot assume that only
> > XHCI will be used with UAS. In particular virtual drivers will
> > be used.
> 
> True, but there is no need to enforce a 2k (IIRC) alignment for XHCI.
> Perhaps you need a different property from the controller.

AFAICT controllers do not export that property.

> Even if you decide the code is 'good enough' (I don't know what the
> cost is of enforcing a 2k alignment instead of 512 bytes)
> the comment is just plain wrong.

Usually block IO will be pages. They are 4K aligned.
In terms of performance this code is unlikely to matter.
But it is needed for correctness.

What would you want for the comment?

	Regards
		Oliver

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:
> From: Oliver Neukum
> > Sent: 29 April 2019 14:38
> > On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> > > From: Oliver Neukum
> > > > 
> > > > +	 * USB has unusual scatter-gather requirements: the length of each
> > > > +	 * scatterlist element except the last must be divisible by the
> > > > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > > > +	 * power of 2.  Inform the block layer about this requirement.
> > > > +	 */
> > > 
> > > That isn't the correct restriction for XHCI.
> > > It has its own perverse restrictions.
> > > I think they are all handled within the xhci driver.
> > 
> > Yes, but that does not matter. You just cannot assume that only
> > XHCI will be used with UAS. In particular virtual drivers will
> > be used.
> 
> True, but there is no need to enforce a 2k (IIRC) alignment for XHCI.
> Perhaps you need a different property from the controller.

AFAICT controllers do not export that property.

> Even if you decide the code is 'good enough' (I don't know what the
> cost is of enforcing a 2k alignment instead of 512 bytes)
> the comment is just plain wrong.

Usually block IO will be pages. They are 4K aligned.
In terms of performance this code is unlikely to matter.
But it is needed for correctness.

What would you want for the comment?

	Regards
		Oliver

UAS: fix alignment of scatter/gather segments

[David Laight]

From: Oliver Neukum
> On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:
> > From: Oliver Neukum
> > > Sent: 29 April 2019 14:38
> > > On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> > > > From: Oliver Neukum
> > > > >
> > > > > +	 * USB has unusual scatter-gather requirements: the length of each
> > > > > +	 * scatterlist element except the last must be divisible by the
> > > > > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > > > > +	 * power of 2.  Inform the block layer about this requirement.
> > > > > +	 */
> > > >
> > > > That isn't the correct restriction for XHCI.
> > > > It has its own perverse restrictions.
> > > > I think they are all handled within the xhci driver.
> > >
> > > Yes, but that does not matter. You just cannot assume that only
> > > XHCI will be used with UAS. In particular virtual drivers will
> > > be used.
> >
> > True, but there is no need to enforce a 2k (IIRC) alignment for XHCI.
> > Perhaps you need a different property from the controller.
> 
> AFAICT controllers do not export that property.

Perhaps they need to ....

> > Even if you decide the code is 'good enough' (I don't know what the
> > cost is of enforcing a 2k alignment instead of 512 bytes)
> > the comment is just plain wrong.
> 
> Usually block IO will be pages. They are 4K aligned.
> In terms of performance this code is unlikely to matter.

Presumably there are some cases where the buffer isn't 4k aligned.
I'm guessing things like 'dd' of raw disks?

If the buffer has the wrong alignment then I presume a bounce buffer
has to be allocated?
The USB controller drivers are likely to need to be able to do that
anyway because buffers from the network stack can have almost
arbitrary alignment (I remember that code being horrid, I don't
remember a data copy in the TX path).

> But it is needed for correctness.

If you are doing it for 'correctness' then you need the correct size.
If you are doing it because you've seen too small an alignment you
should be mentioning the failing system.

> What would you want for the comment?

You need to be more specific about the alignment requirements than
the old comment, not far less specific.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] UAS: fix alignment of scatter/gather segments

[David Laight]

From: Oliver Neukum
> On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:
> > From: Oliver Neukum
> > > Sent: 29 April 2019 14:38
> > > On Mo, 2019-04-29 at 13:31 +0000, David Laight wrote:
> > > > From: Oliver Neukum
> > > > >
> > > > > +	 * USB has unusual scatter-gather requirements: the length of each
> > > > > +	 * scatterlist element except the last must be divisible by the
> > > > > +	 * Bulk maxpacket value.  Fortunately this value is always a
> > > > > +	 * power of 2.  Inform the block layer about this requirement.
> > > > > +	 */
> > > >
> > > > That isn't the correct restriction for XHCI.
> > > > It has its own perverse restrictions.
> > > > I think they are all handled within the xhci driver.
> > >
> > > Yes, but that does not matter. You just cannot assume that only
> > > XHCI will be used with UAS. In particular virtual drivers will
> > > be used.
> >
> > True, but there is no need to enforce a 2k (IIRC) alignment for XHCI.
> > Perhaps you need a different property from the controller.
> 
> AFAICT controllers do not export that property.

Perhaps they need to ....

> > Even if you decide the code is 'good enough' (I don't know what the
> > cost is of enforcing a 2k alignment instead of 512 bytes)
> > the comment is just plain wrong.
> 
> Usually block IO will be pages. They are 4K aligned.
> In terms of performance this code is unlikely to matter.

Presumably there are some cases where the buffer isn't 4k aligned.
I'm guessing things like 'dd' of raw disks?

If the buffer has the wrong alignment then I presume a bounce buffer
has to be allocated?
The USB controller drivers are likely to need to be able to do that
anyway because buffers from the network stack can have almost
arbitrary alignment (I remember that code being horrid, I don't
remember a data copy in the TX path).

> But it is needed for correctness.

If you are doing it for 'correctness' then you need the correct size.
If you are doing it because you've seen too small an alignment you
should be mentioning the failing system.

> What would you want for the comment?

You need to be more specific about the alignment requirements than
the old comment, not far less specific.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> From: Oliver Neukum
> > On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:

> > AFAICT controllers do not export that property.
> 
> Perhaps they need to ....

Feel free to make a patch.

> > > Even if you decide the code is 'good enough' (I don't know what the
> > > cost is of enforcing a 2k alignment instead of 512 bytes)
> > > the comment is just plain wrong.
> > 
> > Usually block IO will be pages. They are 4K aligned.
> > In terms of performance this code is unlikely to matter.
> 
> Presumably there are some cases where the buffer isn't 4k aligned.
> I'm guessing things like 'dd' of raw disks?

Possibly.

> If the buffer has the wrong alignment then I presume a bounce buffer
> has to be allocated?
> The USB controller drivers are likely to need to be able to do that
> anyway because buffers from the network stack can have almost
> arbitrary alignment (I remember that code being horrid, I don't
> remember a data copy in the TX path).

You can ask the network layer to undo scatter/gather.
I don't see an issue.

> > But it is needed for correctness.
> 
> If you are doing it for 'correctness' then you need the correct size.

Why? Any larger size will do.

> If you are doing it because you've seen too small an alignment you
> should be mentioning the failing system.

Why?

> > What would you want for the comment?
> 
> You need to be more specific about the alignment requirements than
> the old comment, not far less specific.

But the statement the old comment made are no longer correct.

	Regards
		Oliver

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> From: Oliver Neukum
> > On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:

> > AFAICT controllers do not export that property.
> 
> Perhaps they need to ....

Feel free to make a patch.

> > > Even if you decide the code is 'good enough' (I don't know what the
> > > cost is of enforcing a 2k alignment instead of 512 bytes)
> > > the comment is just plain wrong.
> > 
> > Usually block IO will be pages. They are 4K aligned.
> > In terms of performance this code is unlikely to matter.
> 
> Presumably there are some cases where the buffer isn't 4k aligned.
> I'm guessing things like 'dd' of raw disks?

Possibly.

> If the buffer has the wrong alignment then I presume a bounce buffer
> has to be allocated?
> The USB controller drivers are likely to need to be able to do that
> anyway because buffers from the network stack can have almost
> arbitrary alignment (I remember that code being horrid, I don't
> remember a data copy in the TX path).

You can ask the network layer to undo scatter/gather.
I don't see an issue.

> > But it is needed for correctness.
> 
> If you are doing it for 'correctness' then you need the correct size.

Why? Any larger size will do.

> If you are doing it because you've seen too small an alignment you
> should be mentioning the failing system.

Why?

> > What would you want for the comment?
> 
> You need to be more specific about the alignment requirements than
> the old comment, not far less specific.

But the statement the old comment made are no longer correct.

	Regards
		Oliver

UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Mon, 29 Apr 2019, Oliver Neukum wrote:

> On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > From: Oliver Neukum
> > > On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:
> 
> > > AFAICT controllers do not export that property.
> > 
> > Perhaps they need to ....
> 
> Feel free to make a patch.
> 
> > > > Even if you decide the code is 'good enough' (I don't know what the
> > > > cost is of enforcing a 2k alignment instead of 512 bytes)
> > > > the comment is just plain wrong.
> > > 
> > > Usually block IO will be pages. They are 4K aligned.
> > > In terms of performance this code is unlikely to matter.
> > 
> > Presumably there are some cases where the buffer isn't 4k aligned.
> > I'm guessing things like 'dd' of raw disks?
> 
> Possibly.
> 
> > If the buffer has the wrong alignment then I presume a bounce buffer
> > has to be allocated?
> > The USB controller drivers are likely to need to be able to do that
> > anyway because buffers from the network stack can have almost
> > arbitrary alignment (I remember that code being horrid, I don't
> > remember a data copy in the TX path).
> 
> You can ask the network layer to undo scatter/gather.
> I don't see an issue.
> 
> > > But it is needed for correctness.
> > 
> > If you are doing it for 'correctness' then you need the correct size.
> 
> Why? Any larger size will do.
> 
> > If you are doing it because you've seen too small an alignment you
> > should be mentioning the failing system.
> 
> Why?
> 
> > > What would you want for the comment?
> > 
> > You need to be more specific about the alignment requirements than
> > the old comment, not far less specific.
> 
> But the statement the old comment made are no longer correct.

Perhaps David would be satisfied if the comment were changed to say 
that _some_ USB controller drivers have this unusual alignment 
requirement.

Alan Stern

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Mon, 29 Apr 2019, Oliver Neukum wrote:

> On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > From: Oliver Neukum
> > > On Mo, 2019-04-29 at 14:19 +0000, David Laight wrote:
> 
> > > AFAICT controllers do not export that property.
> > 
> > Perhaps they need to ....
> 
> Feel free to make a patch.
> 
> > > > Even if you decide the code is 'good enough' (I don't know what the
> > > > cost is of enforcing a 2k alignment instead of 512 bytes)
> > > > the comment is just plain wrong.
> > > 
> > > Usually block IO will be pages. They are 4K aligned.
> > > In terms of performance this code is unlikely to matter.
> > 
> > Presumably there are some cases where the buffer isn't 4k aligned.
> > I'm guessing things like 'dd' of raw disks?
> 
> Possibly.
> 
> > If the buffer has the wrong alignment then I presume a bounce buffer
> > has to be allocated?
> > The USB controller drivers are likely to need to be able to do that
> > anyway because buffers from the network stack can have almost
> > arbitrary alignment (I remember that code being horrid, I don't
> > remember a data copy in the TX path).
> 
> You can ask the network layer to undo scatter/gather.
> I don't see an issue.
> 
> > > But it is needed for correctness.
> > 
> > If you are doing it for 'correctness' then you need the correct size.
> 
> Why? Any larger size will do.
> 
> > If you are doing it because you've seen too small an alignment you
> > should be mentioning the failing system.
> 
> Why?
> 
> > > What would you want for the comment?
> > 
> > You need to be more specific about the alignment requirements than
> > the old comment, not far less specific.
> 
> But the statement the old comment made are no longer correct.

Perhaps David would be satisfied if the comment were changed to say 
that _some_ USB controller drivers have this unusual alignment 
requirement.

Alan Stern

UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> On Mon, 29 Apr 2019, Oliver Neukum wrote:
> 
> > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> 
> > But the statement the old comment made are no longer correct.
> 
> Perhaps David would be satisfied if the comment were changed to say 
> that _some_ USB controller drivers have this unusual alignment 
> requirement.

It would seem to me that every controller that does not do
scatter/gather has this requirement. In other words, this is
the true requirement of USB. It does not come from the
controller. It comes from the protocol's need to not
send a short package.

The second, old, comment is about controllers.

	Regards
		Oliver

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> On Mon, 29 Apr 2019, Oliver Neukum wrote:
> 
> > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> 
> > But the statement the old comment made are no longer correct.
> 
> Perhaps David would be satisfied if the comment were changed to say 
> that _some_ USB controller drivers have this unusual alignment 
> requirement.

It would seem to me that every controller that does not do
scatter/gather has this requirement. In other words, this is
the true requirement of USB. It does not come from the
controller. It comes from the protocol's need to not
send a short package.

The second, old, comment is about controllers.

	Regards
		Oliver

UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Mon, 29 Apr 2019, Oliver Neukum wrote:

> On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > 
> > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > 
> > > But the statement the old comment made are no longer correct.
> > 
> > Perhaps David would be satisfied if the comment were changed to say 
> > that _some_ USB controller drivers have this unusual alignment 
> > requirement.
> 
> It would seem to me that every controller that does not do
> scatter/gather has this requirement. In other words, this is
> the true requirement of USB. It does not come from the
> controller. It comes from the protocol's need to not
> send a short package.

Are you sure that xHCI has this requirement?  I haven't checked the
spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
OHCI and EHCI do in fact have hardware support for scatter-gather).

More precisely, what matters is whether the controller is able to merge
two different DMA segments into a single packet.  UHCI can't.  OHCI and
EHCI can, but only if the first segment ends at a page boundary and the
second begins at a page boundary -- it's easier just to say that the
segments have to be maxpacket-aligned.

> The second, old, comment is about controllers.

Well, if the drivers would use bounce buffers to work around the 
controllers' issues then they wouldn't have this special requirement.  
So it really is a combination of what the hardware can do and what the 
driver can do.

Alan Stern

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Mon, 29 Apr 2019, Oliver Neukum wrote:

> On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > 
> > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > 
> > > But the statement the old comment made are no longer correct.
> > 
> > Perhaps David would be satisfied if the comment were changed to say 
> > that _some_ USB controller drivers have this unusual alignment 
> > requirement.
> 
> It would seem to me that every controller that does not do
> scatter/gather has this requirement. In other words, this is
> the true requirement of USB. It does not come from the
> controller. It comes from the protocol's need to not
> send a short package.

Are you sure that xHCI has this requirement?  I haven't checked the
spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
OHCI and EHCI do in fact have hardware support for scatter-gather).

More precisely, what matters is whether the controller is able to merge
two different DMA segments into a single packet.  UHCI can't.  OHCI and
EHCI can, but only if the first segment ends at a page boundary and the
second begins at a page boundary -- it's easier just to say that the
segments have to be maxpacket-aligned.

> The second, old, comment is about controllers.

Well, if the drivers would use bounce buffers to work around the 
controllers' issues then they wouldn't have this special requirement.  
So it really is a combination of what the hardware can do and what the 
driver can do.

Alan Stern

UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 13:55 -0400, Alan Stern wrote:
> On Mon, 29 Apr 2019, Oliver Neukum wrote:
> 
> > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > > 
> > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > > 
> > > > But the statement the old comment made are no longer correct.
> > > 
> > > Perhaps David would be satisfied if the comment were changed to say 
> > > that _some_ USB controller drivers have this unusual alignment 
> > > requirement.
> > 
> > It would seem to me that every controller that does not do
> > scatter/gather has this requirement. In other words, this is
> > the true requirement of USB. It does not come from the
> > controller. It comes from the protocol's need to not
> > send a short package.
> 
> Are you sure that xHCI has this requirement?  I haven't checked the

I am sure that it has not. UAS would never have worked.
Like in the case of storage this patch is necessary
for virtual controllers.

> spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> OHCI and EHCI do in fact have hardware support for scatter-gather).
> 
> More precisely, what matters is whether the controller is able to merge
> two different DMA segments into a single packet.  UHCI can't.  OHCI and

Correct. However, we cannot blindly assume in a class driver that
certain controllers will be used.

> EHCI can, but only if the first segment ends at a page boundary and the
> second begins at a page boundary -- it's easier just to say that the
> segments have to be maxpacket-aligned.
> 
> > The second, old, comment is about controllers.
> 
> Well, if the drivers would use bounce buffers to work around the 
> controllers' issues then they wouldn't have this special requirement.  
> So it really is a combination of what the hardware can do and what the 
> driver can do.

Yes, but the point of using an API to specify restrictions to the
upper layer is to avoid using bounce buffers. Besides, bounce
buffers in block IO is interesting in terms of VM implications.

	Regards
		Oliver

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Oliver Neukum]

On Mo, 2019-04-29 at 13:55 -0400, Alan Stern wrote:
> On Mon, 29 Apr 2019, Oliver Neukum wrote:
> 
> > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > > 
> > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > > 
> > > > But the statement the old comment made are no longer correct.
> > > 
> > > Perhaps David would be satisfied if the comment were changed to say 
> > > that _some_ USB controller drivers have this unusual alignment 
> > > requirement.
> > 
> > It would seem to me that every controller that does not do
> > scatter/gather has this requirement. In other words, this is
> > the true requirement of USB. It does not come from the
> > controller. It comes from the protocol's need to not
> > send a short package.
> 
> Are you sure that xHCI has this requirement?  I haven't checked the

I am sure that it has not. UAS would never have worked.
Like in the case of storage this patch is necessary
for virtual controllers.

> spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> OHCI and EHCI do in fact have hardware support for scatter-gather).
> 
> More precisely, what matters is whether the controller is able to merge
> two different DMA segments into a single packet.  UHCI can't.  OHCI and

Correct. However, we cannot blindly assume in a class driver that
certain controllers will be used.

> EHCI can, but only if the first segment ends at a page boundary and the
> second begins at a page boundary -- it's easier just to say that the
> segments have to be maxpacket-aligned.
> 
> > The second, old, comment is about controllers.
> 
> Well, if the drivers would use bounce buffers to work around the 
> controllers' issues then they wouldn't have this special requirement.  
> So it really is a combination of what the hardware can do and what the 
> driver can do.

Yes, but the point of using an API to specify restrictions to the
upper layer is to avoid using bounce buffers. Besides, bounce
buffers in block IO is interesting in terms of VM implications.

	Regards
		Oliver

UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Mon, 29 Apr 2019, Oliver Neukum wrote:

> On Mo, 2019-04-29 at 13:55 -0400, Alan Stern wrote:
> > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > 
> > > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > > > 
> > > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > > > 
> > > > > But the statement the old comment made are no longer correct.
> > > > 
> > > > Perhaps David would be satisfied if the comment were changed to say 
> > > > that _some_ USB controller drivers have this unusual alignment 
> > > > requirement.
> > > 
> > > It would seem to me that every controller that does not do
> > > scatter/gather has this requirement. In other words, this is
> > > the true requirement of USB. It does not come from the
> > > controller. It comes from the protocol's need to not
> > > send a short package.
> > 
> > Are you sure that xHCI has this requirement?  I haven't checked the
> 
> I am sure that it has not. UAS would never have worked.
> Like in the case of storage this patch is necessary
> for virtual controllers.

Okay, yes, I agree with what you say.  With the addition that some
controllers which _do_ support scatter-gather also have this
requirement.

In fact, xhci-hcd may be the only driver that doesn't need this special 
alignment.

Alan Stern

> > spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> > OHCI and EHCI do in fact have hardware support for scatter-gather).
> > 
> > More precisely, what matters is whether the controller is able to merge
> > two different DMA segments into a single packet.  UHCI can't.  OHCI and
> 
> Correct. However, we cannot blindly assume in a class driver that
> certain controllers will be used.
> 
> > EHCI can, but only if the first segment ends at a page boundary and the
> > second begins at a page boundary -- it's easier just to say that the
> > segments have to be maxpacket-aligned.
> > 
> > > The second, old, comment is about controllers.
> > 
> > Well, if the drivers would use bounce buffers to work around the 
> > controllers' issues then they wouldn't have this special requirement.  
> > So it really is a combination of what the hardware can do and what the 
> > driver can do.
> 
> Yes, but the point of using an API to specify restrictions to the
> upper layer is to avoid using bounce buffers. Besides, bounce
> buffers in block IO is interesting in terms of VM implications.
> 
> 	Regards
> 		Oliver
> 
> 
>

Re: [PATCH] UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Mon, 29 Apr 2019, Oliver Neukum wrote:

> On Mo, 2019-04-29 at 13:55 -0400, Alan Stern wrote:
> > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > 
> > > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > > > 
> > > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > > > 
> > > > > But the statement the old comment made are no longer correct.
> > > > 
> > > > Perhaps David would be satisfied if the comment were changed to say 
> > > > that _some_ USB controller drivers have this unusual alignment 
> > > > requirement.
> > > 
> > > It would seem to me that every controller that does not do
> > > scatter/gather has this requirement. In other words, this is
> > > the true requirement of USB. It does not come from the
> > > controller. It comes from the protocol's need to not
> > > send a short package.
> > 
> > Are you sure that xHCI has this requirement?  I haven't checked the
> 
> I am sure that it has not. UAS would never have worked.
> Like in the case of storage this patch is necessary
> for virtual controllers.

Okay, yes, I agree with what you say.  With the addition that some
controllers which _do_ support scatter-gather also have this
requirement.

In fact, xhci-hcd may be the only driver that doesn't need this special 
alignment.

Alan Stern

> > spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> > OHCI and EHCI do in fact have hardware support for scatter-gather).
> > 
> > More precisely, what matters is whether the controller is able to merge
> > two different DMA segments into a single packet.  UHCI can't.  OHCI and
> 
> Correct. However, we cannot blindly assume in a class driver that
> certain controllers will be used.
> 
> > EHCI can, but only if the first segment ends at a page boundary and the
> > second begins at a page boundary -- it's easier just to say that the
> > segments have to be maxpacket-aligned.
> > 
> > > The second, old, comment is about controllers.
> > 
> > Well, if the drivers would use bounce buffers to work around the 
> > controllers' issues then they wouldn't have this special requirement.  
> > So it really is a combination of what the hardware can do and what the 
> > driver can do.
> 
> Yes, but the point of using an API to specify restrictions to the
> upper layer is to avoid using bounce buffers. Besides, bounce
> buffers in block IO is interesting in terms of VM implications.
> 
> 	Regards
> 		Oliver
> 
> 
> 

UAS: fix alignment of scatter/gather segments

[David Laight]

From: Alan Stern
> Sent: 29 April 2019 18:55
> On Mon, 29 Apr 2019, Oliver Neukum wrote:
> 
> > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > >
> > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > >
> > > > But the statement the old comment made are no longer correct.
> > >
> > > Perhaps David would be satisfied if the comment were changed to say
> > > that _some_ USB controller drivers have this unusual alignment
> > > requirement.
> >
> > It would seem to me that every controller that does not do
> > scatter/gather has this requirement. In other words, this is
> > the true requirement of USB. It does not come from the
> > controller. It comes from the protocol's need to not
> > send a short package.

The hardware requirement is that packets that need to be joined
together to make a long request must be 'full'.
In many cases this means a zero length packet must be sent to
correctly terminate a request that is a multiple of the packet size.
Since the software has to add the zero length packet there is
no real difference between a single scatter-gather transmit and
multiple single packet trnasmits.

For USB2 bulk transfers the packet size is 512, and for USB3 1024.
The old comment suggested 2048 for some unsupported interface.

> Are you sure that xHCI has this requirement?  I haven't checked the
> spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> OHCI and EHCI do in fact have hardware support for scatter-gather).
> 
> More precisely, what matters is whether the controller is able to merge
> two different DMA segments into a single packet.  UHCI can't.  OHCI and
> EHCI can, but only if the first segment ends at a page boundary and the
> second begins at a page boundary -- it's easier just to say that the
> segments have to be maxpacket-aligned.

XHCI (for USB2 or USB3) can handle almost arbitrary fragments.
There are a couple of annoying restrictions (IIRC):
- Fragments cannot cross a 64k boundary.
  (How much would it cost the hardware to have a 32bit (or even 64bit)
  counter.)
- The 'Link TRB' between ring segments must be aligned to a packet boundary.
I believe the Linux XHCI driver now handles both these cases.
(It hadn't used to - which is why I know anything about USB3 and XHCI.)

I also recall issues with non word aligned buffers for some controllers.

> > The second, old, comment is about controllers.
> 
> Well, if the drivers would use bounce buffers to work around the
> controllers' issues then they wouldn't have this special requirement.
> So it really is a combination of what the hardware can do and what the
> driver can do.

Indeed.
So any comment should refer to what the linux usb drivers requires
of their 'user', not what happens on the USB wire.

It might me that you do end up having to request 1k aligned
buffers for XHCI, but the comment should say that you are
adding a far stronger restriction than that required by the
driver and controller.

Given that XHCI is the most common interface (at least on x86)
if the 1k alignment forces extra bounce buffers in any code
paths it could be a performance issue.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] UAS: fix alignment of scatter/gather segments

[David Laight]

From: Alan Stern
> Sent: 29 April 2019 18:55
> On Mon, 29 Apr 2019, Oliver Neukum wrote:
> 
> > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > >
> > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > >
> > > > But the statement the old comment made are no longer correct.
> > >
> > > Perhaps David would be satisfied if the comment were changed to say
> > > that _some_ USB controller drivers have this unusual alignment
> > > requirement.
> >
> > It would seem to me that every controller that does not do
> > scatter/gather has this requirement. In other words, this is
> > the true requirement of USB. It does not come from the
> > controller. It comes from the protocol's need to not
> > send a short package.

The hardware requirement is that packets that need to be joined
together to make a long request must be 'full'.
In many cases this means a zero length packet must be sent to
correctly terminate a request that is a multiple of the packet size.
Since the software has to add the zero length packet there is
no real difference between a single scatter-gather transmit and
multiple single packet trnasmits.

For USB2 bulk transfers the packet size is 512, and for USB3 1024.
The old comment suggested 2048 for some unsupported interface.

> Are you sure that xHCI has this requirement?  I haven't checked the
> spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> OHCI and EHCI do in fact have hardware support for scatter-gather).
> 
> More precisely, what matters is whether the controller is able to merge
> two different DMA segments into a single packet.  UHCI can't.  OHCI and
> EHCI can, but only if the first segment ends at a page boundary and the
> second begins at a page boundary -- it's easier just to say that the
> segments have to be maxpacket-aligned.

XHCI (for USB2 or USB3) can handle almost arbitrary fragments.
There are a couple of annoying restrictions (IIRC):
- Fragments cannot cross a 64k boundary.
  (How much would it cost the hardware to have a 32bit (or even 64bit)
  counter.)
- The 'Link TRB' between ring segments must be aligned to a packet boundary.
I believe the Linux XHCI driver now handles both these cases.
(It hadn't used to - which is why I know anything about USB3 and XHCI.)

I also recall issues with non word aligned buffers for some controllers.

> > The second, old, comment is about controllers.
> 
> Well, if the drivers would use bounce buffers to work around the
> controllers' issues then they wouldn't have this special requirement.
> So it really is a combination of what the hardware can do and what the
> driver can do.

Indeed.
So any comment should refer to what the linux usb drivers requires
of their 'user', not what happens on the USB wire.

It might me that you do end up having to request 1k aligned
buffers for XHCI, but the comment should say that you are
adding a far stronger restriction than that required by the
driver and controller.

Given that XHCI is the most common interface (at least on x86)
if the 1k alignment forces extra bounce buffers in any code
paths it could be a performance issue.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Tue, 30 Apr 2019, David Laight wrote:

> From: Alan Stern
> > Sent: 29 April 2019 18:55
> > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > 
> > > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > > >
> > > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > > >
> > > > > But the statement the old comment made are no longer correct.
> > > >
> > > > Perhaps David would be satisfied if the comment were changed to say
> > > > that _some_ USB controller drivers have this unusual alignment
> > > > requirement.
> > >
> > > It would seem to me that every controller that does not do
> > > scatter/gather has this requirement. In other words, this is
> > > the true requirement of USB. It does not come from the
> > > controller. It comes from the protocol's need to not
> > > send a short package.
> 
> The hardware requirement is that packets that need to be joined
> together to make a long request must be 'full'.
> In many cases this means a zero length packet must be sent to
> correctly terminate a request that is a multiple of the packet size.

With mass storage this happens only when there's an error.  In other
cases the sender and receiver both know beforehand how long a bulk
transfer will be, so there's no need to mark an early termination with
a zero-length packet.

> Since the software has to add the zero length packet there is
> no real difference between a single scatter-gather transmit and
> multiple single packet trnasmits.

In fact, zero-length packets have nothing to do with it.  The source of
the data doesn't matter: It could be scatter-gather, a single
contiguous buffer, or even a Ouija board.  All that matters is that the
data shows up at the receiver as a series of packets in which all
(except possibly the last) are full size.  The receiver doesn't care 
how those packets were assembled.

> For USB2 bulk transfers the packet size is 512, and for USB3 1024.
> The old comment suggested 2048 for some unsupported interface.

Wireless USB.  And to be fair, the old comment also said that 
supporting such a large value would require changes outside the USB 
stack.

> > Are you sure that xHCI has this requirement?  I haven't checked the
> > spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> > OHCI and EHCI do in fact have hardware support for scatter-gather).
> > 
> > More precisely, what matters is whether the controller is able to merge
> > two different DMA segments into a single packet.  UHCI can't.  OHCI and
> > EHCI can, but only if the first segment ends at a page boundary and the
> > second begins at a page boundary -- it's easier just to say that the
> > segments have to be maxpacket-aligned.
> 
> XHCI (for USB2 or USB3) can handle almost arbitrary fragments.
> There are a couple of annoying restrictions (IIRC):
> - Fragments cannot cross a 64k boundary.
>   (How much would it cost the hardware to have a 32bit (or even 64bit)
>   counter.)
> - The 'Link TRB' between ring segments must be aligned to a packet boundary.
> I believe the Linux XHCI driver now handles both these cases.
> (It hadn't used to - which is why I know anything about USB3 and XHCI.)
> 
> I also recall issues with non word aligned buffers for some controllers.
> 
> > > The second, old, comment is about controllers.
> > 
> > Well, if the drivers would use bounce buffers to work around the
> > controllers' issues then they wouldn't have this special requirement.
> > So it really is a combination of what the hardware can do and what the
> > driver can do.
> 
> Indeed.
> So any comment should refer to what the linux usb drivers requires
> of their 'user', not what happens on the USB wire.

The comment said nothing about the wire.  It was actually rather vague, 
just saying "USB" -- this could mean the hardware protocol or the USB 
host controller driver.

> It might me that you do end up having to request 1k aligned
> buffers for XHCI, but the comment should say that you are
> adding a far stronger restriction than that required by the
> driver and controller.

Depends on the driver.  For xHCI the restriction is stronger than 
required; for all the others (as far as I know) it is not.

> Given that XHCI is the most common interface (at least on x86)
> if the 1k alignment forces extra bounce buffers in any code
> paths it could be a performance issue.

If that happens we can ask the controller driver to export a 
"scatter-gather required alignment" parameter.  It seems premature to 
worry about it now.

Alan Stern

RE: [PATCH] UAS: fix alignment of scatter/gather segments

[Alan Stern]

On Tue, 30 Apr 2019, David Laight wrote:

> From: Alan Stern
> > Sent: 29 April 2019 18:55
> > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > 
> > > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote:
> > > > On Mon, 29 Apr 2019, Oliver Neukum wrote:
> > > >
> > > > > On Mo, 2019-04-29 at 15:06 +0000, David Laight wrote:
> > > >
> > > > > But the statement the old comment made are no longer correct.
> > > >
> > > > Perhaps David would be satisfied if the comment were changed to say
> > > > that _some_ USB controller drivers have this unusual alignment
> > > > requirement.
> > >
> > > It would seem to me that every controller that does not do
> > > scatter/gather has this requirement. In other words, this is
> > > the true requirement of USB. It does not come from the
> > > controller. It comes from the protocol's need to not
> > > send a short package.
> 
> The hardware requirement is that packets that need to be joined
> together to make a long request must be 'full'.
> In many cases this means a zero length packet must be sent to
> correctly terminate a request that is a multiple of the packet size.

With mass storage this happens only when there's an error.  In other
cases the sender and receiver both know beforehand how long a bulk
transfer will be, so there's no need to mark an early termination with
a zero-length packet.

> Since the software has to add the zero length packet there is
> no real difference between a single scatter-gather transmit and
> multiple single packet trnasmits.

In fact, zero-length packets have nothing to do with it.  The source of
the data doesn't matter: It could be scatter-gather, a single
contiguous buffer, or even a Ouija board.  All that matters is that the
data shows up at the receiver as a series of packets in which all
(except possibly the last) are full size.  The receiver doesn't care 
how those packets were assembled.

> For USB2 bulk transfers the packet size is 512, and for USB3 1024.
> The old comment suggested 2048 for some unsupported interface.

Wireless USB.  And to be fair, the old comment also said that 
supporting such a large value would require changes outside the USB 
stack.

> > Are you sure that xHCI has this requirement?  I haven't checked the
> > spec.  I know that UHCI, OHCI, and EHCI do need this alignment (and
> > OHCI and EHCI do in fact have hardware support for scatter-gather).
> > 
> > More precisely, what matters is whether the controller is able to merge
> > two different DMA segments into a single packet.  UHCI can't.  OHCI and
> > EHCI can, but only if the first segment ends at a page boundary and the
> > second begins at a page boundary -- it's easier just to say that the
> > segments have to be maxpacket-aligned.
> 
> XHCI (for USB2 or USB3) can handle almost arbitrary fragments.
> There are a couple of annoying restrictions (IIRC):
> - Fragments cannot cross a 64k boundary.
>   (How much would it cost the hardware to have a 32bit (or even 64bit)
>   counter.)
> - The 'Link TRB' between ring segments must be aligned to a packet boundary.
> I believe the Linux XHCI driver now handles both these cases.
> (It hadn't used to - which is why I know anything about USB3 and XHCI.)
> 
> I also recall issues with non word aligned buffers for some controllers.
> 
> > > The second, old, comment is about controllers.
> > 
> > Well, if the drivers would use bounce buffers to work around the
> > controllers' issues then they wouldn't have this special requirement.
> > So it really is a combination of what the hardware can do and what the
> > driver can do.
> 
> Indeed.
> So any comment should refer to what the linux usb drivers requires
> of their 'user', not what happens on the USB wire.

The comment said nothing about the wire.  It was actually rather vague, 
just saying "USB" -- this could mean the hardware protocol or the USB 
host controller driver.

> It might me that you do end up having to request 1k aligned
> buffers for XHCI, but the comment should say that you are
> adding a far stronger restriction than that required by the
> driver and controller.

Depends on the driver.  For xHCI the restriction is stronger than 
required; for all the others (as far as I know) it is not.

> Given that XHCI is the most common interface (at least on x86)
> if the 1k alignment forces extra bounce buffers in any code
> paths it could be a performance issue.

If that happens we can ask the controller driver to export a 
"scatter-gather required alignment" parameter.  It seems premature to 
worry about it now.

Alan Stern