[U-Boot] [PATCH 1/5] dm: usb: Add a new USB controller operation 'get_max_xfer_size'

[U-Boot] [PATCH 1/5] dm: usb: Add a new USB controller operation 'get_max_xfer_size'

[Bin Meng]

The HCD may have limitation on the maximum bytes to be transferred
in a USB transfer. USB class driver needs to be aware of this.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/usb-uclass.c | 11 +++++++++++
 include/usb.h                 | 22 +++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 0b8a501..bc44fc3 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -150,6 +150,17 @@ int usb_update_hub_device(struct usb_device *udev)
 	return ops->update_hub_device(bus, udev);
 }
 
+int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
+{
+	struct udevice *bus = udev->controller_dev;
+	struct dm_usb_ops *ops = usb_get_ops(bus);
+
+	if (!ops->get_max_xfer_size)
+		return -ENOSYS;
+
+	return ops->get_max_xfer_size(bus, size);
+}
+
 int usb_stop(void)
 {
 	struct udevice *bus;
diff --git a/include/usb.h b/include/usb.h
index fad0401..0ddc082 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -766,6 +766,14 @@ struct dm_usb_ops {
 	 * representation of this hub can be updated (xHCI)
 	 */
 	int (*update_hub_device)(struct udevice *bus, struct usb_device *udev);
+
+	/**
+	 * get_max_xfer_size() - Get HCD's maximum transfer bytes
+	 *
+	 * The HCD may have limitation on the maximum bytes to be transferred
+	 * in a USB transfer. USB class driver needs to be aware of this.
+	 */
+	int (*get_max_xfer_size)(struct udevice *bus, size_t *size);
 };
 
 #define usb_get_ops(dev)	((struct dm_usb_ops *)(dev)->driver->ops)
@@ -939,7 +947,7 @@ int usb_new_device(struct usb_device *dev);
 int usb_alloc_device(struct usb_device *dev);
 
 /**
- * update_hub_device() - Update HCD's internal representation of hub
+ * usb_update_hub_device() - Update HCD's internal representation of hub
  *
  * After a hub descriptor is fetched, notify HCD so that its internal
  * representation of this hub can be updated.
@@ -950,6 +958,18 @@ int usb_alloc_device(struct usb_device *dev);
 int usb_update_hub_device(struct usb_device *dev);
 
 /**
+ * usb_get_max_xfer_size() - Get HCD's maximum transfer bytes
+ *
+ * The HCD may have limitation on the maximum bytes to be transferred
+ * in a USB transfer. USB class driver needs to be aware of this.
+ *
+ * @dev:		USB device
+ * @size:		maximum transfer bytes
+ * @return 0 if OK, -ve on error
+ */
+int usb_get_max_xfer_size(struct usb_device *dev, size_t *size);
+
+/**
  * usb_emul_setup_device() - Set up a new USB device emulation
  *
  * This is normally called when a new emulation device is bound. It tells
-- 
2.9.2

[U-Boot] [PATCH 2/5] dm: usb: xhci: Implement get_max_xfer_size() operation

[Bin Meng]

xHCD allocates one segment which includes 64 TRBs for each endpoint
and the last TRB in this segment is configured as a link TRB to form
a TRB ring. Each TRB can transfer up to 64K bytes, however data
buffers referenced by transfer TRBs shall not span 64KB boundaries.
Hence the maximum number of TRBs we can use in one transfer is 62.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9b82ee5..04eb1eb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1228,6 +1228,20 @@ static int xhci_update_hub_device(struct udevice *dev, struct usb_device *udev)
 	return xhci_configure_endpoints(udev, false);
 }
 
+static int xhci_get_max_xfer_size(struct udevice *dev, size_t *size)
+{
+	/*
+	 * xHCD allocates one segment which includes 64 TRBs for each endpoint
+	 * and the last TRB in this segment is configured as a link TRB to form
+	 * a TRB ring. Each TRB can transfer up to 64K bytes, however data
+	 * buffers referenced by transfer TRBs shall not span 64KB boundaries.
+	 * Hence the maximum number of TRBs we can use in one transfer is 62.
+	 */
+	*size = (TRBS_PER_SEGMENT - 2) * TRB_MAX_BUFF_SIZE;
+
+	return 0;
+}
+
 int xhci_register(struct udevice *dev, struct xhci_hccr *hccr,
 		  struct xhci_hcor *hcor)
 {
@@ -1281,6 +1295,7 @@ struct dm_usb_ops xhci_usb_ops = {
 	.interrupt = xhci_submit_int_msg,
 	.alloc_device = xhci_alloc_device,
 	.update_hub_device = xhci_update_hub_device,
+	.get_max_xfer_size  = xhci_get_max_xfer_size,
 };
 
 #endif
-- 
2.9.2

[U-Boot] [PATCH 3/5] dm: usb: ehci: Implement get_max_xfer_size() operation

[Bin Meng]

EHCD can handle any transfer length as long as there is enough free
heap space left, hence set the theoretical max number SIZE_MAX.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/ehci-hcd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 3243c1d..be3e842 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1596,6 +1596,17 @@ static int ehci_destroy_int_queue(struct udevice *dev, struct usb_device *udev,
 	return _ehci_destroy_int_queue(udev, queue);
 }
 
+static int ehci_get_max_xfer_size(struct udevice *dev, size_t *size)
+{
+	/*
+	 * EHCD can handle any transfer length as long as there is enough
+	 * free heap space left, hence set the theoretical max number here.
+	 */
+	*size = SIZE_MAX;
+
+	return 0;
+}
+
 int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
 		  struct ehci_hcor *hcor, const struct ehci_ops *ops,
 		  uint tweaks, enum usb_init_type init)
@@ -1658,6 +1669,7 @@ struct dm_usb_ops ehci_usb_ops = {
 	.create_int_queue = ehci_create_int_queue,
 	.poll_int_queue = ehci_poll_int_queue,
 	.destroy_int_queue = ehci_destroy_int_queue,
+	.get_max_xfer_size  = ehci_get_max_xfer_size,
 };
 
 #endif
-- 
2.9.2

[U-Boot] [PATCH 4/5] usb: storage: Refactor to use max_xfer_blk from struct us_data

[Bin Meng]

This adds a new memeber max_xfer_blk in struct us_data to record
the maximum number of transfer blocks for the storage device.

It is set per HCD setting, and so far is to 65535 for EHCD and 20
for everything else.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_storage.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index df0b057..957ccdb 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -98,19 +98,9 @@ struct us_data {
 	struct scsi_cmd	*srb;			/* current srb */
 	trans_reset	transport_reset;	/* reset routine */
 	trans_cmnd	transport;		/* transport routine */
+	unsigned short	max_xfer_blk;		/* maximum transfer blocks */
 };
 
-#ifdef CONFIG_USB_EHCI_HCD
-/*
- * The U-Boot EHCI driver can handle any transfer length as long as there is
- * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- * limited to 65535 blocks.
- */
-#define USB_MAX_XFER_BLK	65535
-#else
-#define USB_MAX_XFER_BLK	20
-#endif
-
 #ifndef CONFIG_BLK
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
 #endif
@@ -949,6 +939,23 @@ do_retry:
 	return USB_STOR_TRANSPORT_FAILED;
 }
 
+static void usb_stor_set_max_xfer_blk(struct us_data *us)
+{
+	unsigned short blk;
+
+#ifdef CONFIG_USB_EHCI_HCD
+	/*
+	 * The U-Boot EHCI driver can handle any transfer length as long as
+	 * there is enough free heap space left, but the SCSI READ(10) and
+	 * WRITE(10) commands are limited to 65535 blocks.
+	 */
+	blk = USHRT_MAX;
+#else
+	blk = 20;
+#endif
+
+	us->max_xfer_blk = blk;
+}
 
 static int usb_inquiry(struct scsi_cmd *srb, struct us_data *ss)
 {
@@ -1150,12 +1157,12 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 		/* XXX need some comment here */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > USB_MAX_XFER_BLK)
-			smallblks = USB_MAX_XFER_BLK;
+		if (blks > ss->max_xfer_blk)
+			smallblks = ss->max_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == USB_MAX_XFER_BLK)
+		if (smallblks == ss->max_xfer_blk)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1178,7 +1185,7 @@ retry_it:
 	      start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= USB_MAX_XFER_BLK)
+	if (blkcnt >= ss->max_xfer_blk)
 		debug("\n");
 	return blkcnt;
 }
@@ -1236,12 +1243,12 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > USB_MAX_XFER_BLK)
-			smallblks = USB_MAX_XFER_BLK;
+		if (blks > ss->max_xfer_blk)
+			smallblks = ss->max_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == USB_MAX_XFER_BLK)
+		if (smallblks == ss->max_xfer_blk)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1263,7 +1270,7 @@ retry_it:
 	      PRIxPTR "\n", start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= USB_MAX_XFER_BLK)
+	if (blkcnt >= ss->max_xfer_blk)
 		debug("\n");
 	return blkcnt;
 
@@ -1384,6 +1391,10 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 		ss->irqmaxp = usb_maxpacket(dev, ss->irqpipe);
 		dev->irq_handle = usb_stor_irq;
 	}
+
+	/* Set the maximum transfer size per host controller setting */
+	usb_stor_set_max_xfer_blk(ss);
+
 	dev->privptr = (void *)ss;
 	return 1;
 }
-- 
2.9.2

[U-Boot] [PATCH 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Bin Meng]

When EHCD and xHCD are enabled at the same time, USB storage device
driver will fail to read/write from/to the storage device attached
to the xHCI interface, due to its transfer blocks exceeds the xHCD
driver limitation.

With driver model, we have an API to get the controller's maximum
transfer size and we can use that to determine the storage driver's
capability of read/write.

Note: the non-DM version driver is still broken with xHCD and the
intent here is not to fix the non-DM one, since the xHCD itself is
already broken in places like 3.0 hub support, etc.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_storage.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 957ccdb..a57570b 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -939,10 +939,14 @@ do_retry:
 	return USB_STOR_TRANSPORT_FAILED;
 }
 
-static void usb_stor_set_max_xfer_blk(struct us_data *us)
+static void usb_stor_set_max_xfer_blk(struct usb_device *udev,
+				      struct us_data *us)
 {
 	unsigned short blk;
+	size_t __maybe_unused size;
+	int __maybe_unused ret;
 
+#ifndef CONFIG_DM_USB
 #ifdef CONFIG_USB_EHCI_HCD
 	/*
 	 * The U-Boot EHCI driver can handle any transfer length as long as
@@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
 #else
 	blk = 20;
 #endif
+#else
+	ret = usb_get_max_xfer_size(udev, (size_t *)&size);
+	if (ret < 0) {
+		/* unimplemented, let's use default 20 */
+		blk = 20;
+	} else {
+		if (size > USHRT_MAX * 512)
+			blk = USHRT_MAX;
+		blk = size / 512;
+	}
+#endif
 
 	us->max_xfer_blk = blk;
 }
@@ -1393,7 +1408,7 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 	}
 
 	/* Set the maximum transfer size per host controller setting */
-	usb_stor_set_max_xfer_blk(ss);
+	usb_stor_set_max_xfer_blk(dev, ss);
 
 	dev->privptr = (void *)ss;
 	return 1;
-- 
2.9.2

[U-Boot] [PATCH 1/5] dm: usb: Add a new USB controller operation 'get_max_xfer_size'

[Stefan Roese]

Hi Bin,

thanks for working on this. Without these 5 patches applied, loading
files via "load" command from USB sticks results in U-Boot crashes.
With all these 5 patches applied, loading via FS now works as
expected. :)

So:

Tested-by: Stefan Roese <sr@denx.de>

Thank,
Stefan

On 07.09.2017 15:13, Bin Meng wrote:
> The HCD may have limitation on the maximum bytes to be transferred
> in a USB transfer. USB class driver needs to be aware of this.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>   drivers/usb/host/usb-uclass.c | 11 +++++++++++
>   include/usb.h                 | 22 +++++++++++++++++++++-
>   2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 0b8a501..bc44fc3 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -150,6 +150,17 @@ int usb_update_hub_device(struct usb_device *udev)
>   	return ops->update_hub_device(bus, udev);
>   }
>   
> +int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
> +{
> +	struct udevice *bus = udev->controller_dev;
> +	struct dm_usb_ops *ops = usb_get_ops(bus);
> +
> +	if (!ops->get_max_xfer_size)
> +		return -ENOSYS;
> +
> +	return ops->get_max_xfer_size(bus, size);
> +}
> +
>   int usb_stop(void)
>   {
>   	struct udevice *bus;
> diff --git a/include/usb.h b/include/usb.h
> index fad0401..0ddc082 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -766,6 +766,14 @@ struct dm_usb_ops {
>   	 * representation of this hub can be updated (xHCI)
>   	 */
>   	int (*update_hub_device)(struct udevice *bus, struct usb_device *udev);
> +
> +	/**
> +	 * get_max_xfer_size() - Get HCD's maximum transfer bytes
> +	 *
> +	 * The HCD may have limitation on the maximum bytes to be transferred
> +	 * in a USB transfer. USB class driver needs to be aware of this.
> +	 */
> +	int (*get_max_xfer_size)(struct udevice *bus, size_t *size);
>   };
>   
>   #define usb_get_ops(dev)	((struct dm_usb_ops *)(dev)->driver->ops)
> @@ -939,7 +947,7 @@ int usb_new_device(struct usb_device *dev);
>   int usb_alloc_device(struct usb_device *dev);
>   
>   /**
> - * update_hub_device() - Update HCD's internal representation of hub
> + * usb_update_hub_device() - Update HCD's internal representation of hub
>    *
>    * After a hub descriptor is fetched, notify HCD so that its internal
>    * representation of this hub can be updated.
> @@ -950,6 +958,18 @@ int usb_alloc_device(struct usb_device *dev);
>   int usb_update_hub_device(struct usb_device *dev);
>   
>   /**
> + * usb_get_max_xfer_size() - Get HCD's maximum transfer bytes
> + *
> + * The HCD may have limitation on the maximum bytes to be transferred
> + * in a USB transfer. USB class driver needs to be aware of this.
> + *
> + * @dev:		USB device
> + * @size:		maximum transfer bytes
> + * @return 0 if OK, -ve on error
> + */
> +int usb_get_max_xfer_size(struct usb_device *dev, size_t *size);
> +
> +/**
>    * usb_emul_setup_device() - Set up a new USB device emulation
>    *
>    * This is normally called when a new emulation device is bound. It tells
> 

[U-Boot] [PATCH 2/5] dm: usb: xhci: Implement get_max_xfer_size() operation

[Stefan Roese]

On 07.09.2017 15:13, Bin Meng wrote:
> xHCD allocates one segment which includes 64 TRBs for each endpoint
> and the last TRB in this segment is configured as a link TRB to form
> a TRB ring. Each TRB can transfer up to 64K bytes, however data
> buffers referenced by transfer TRBs shall not span 64KB boundaries.
> Hence the maximum number of TRBs we can use in one transfer is 62.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Tested-by: Stefan Roese <sr@denx.de>

Thank,
Stefan

[U-Boot] [PATCH 3/5] dm: usb: ehci: Implement get_max_xfer_size() operation

[Stefan Roese]

On 07.09.2017 15:13, Bin Meng wrote:
> EHCD can handle any transfer length as long as there is enough free
> heap space left, hence set the theoretical max number SIZE_MAX.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 4/5] usb: storage: Refactor to use max_xfer_blk from struct us_data

[Stefan Roese]

On 07.09.2017 15:13, Bin Meng wrote:
> This adds a new memeber max_xfer_blk in struct us_data to record
> the maximum number of transfer blocks for the storage device.
> 
> It is set per HCD setting, and so far is to 65535 for EHCD and 20
> for everything else.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Stefan Roese]

On 07.09.2017 15:13, Bin Meng wrote:
> When EHCD and xHCD are enabled at the same time, USB storage device
> driver will fail to read/write from/to the storage device attached
> to the xHCI interface, due to its transfer blocks exceeds the xHCD
> driver limitation.
> 
> With driver model, we have an API to get the controller's maximum
> transfer size and we can use that to determine the storage driver's
> capability of read/write.
> 
> Note: the non-DM version driver is still broken with xHCD and the
> intent here is not to fix the non-DM one, since the xHCD itself is
> already broken in places like 3.0 hub support, etc.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Tom Rini]

On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote:

> When EHCD and xHCD are enabled at the same time, USB storage device
> driver will fail to read/write from/to the storage device attached
> to the xHCI interface, due to its transfer blocks exceeds the xHCD
> driver limitation.
> 
> With driver model, we have an API to get the controller's maximum
> transfer size and we can use that to determine the storage driver's
> capability of read/write.
> 
> Note: the non-DM version driver is still broken with xHCD and the
> intent here is not to fix the non-DM one, since the xHCD itself is
> already broken in places like 3.0 hub support, etc.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Stefan Roese <sr@denx.de>
[snip]
> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
>  #else
>  	blk = 20;
>  #endif
> +#else
> +	ret = usb_get_max_xfer_size(udev, (size_t *)&size);
> +	if (ret < 0) {
> +		/* unimplemented, let's use default 20 */
> +		blk = 20;
> +	} else {
> +		if (size > USHRT_MAX * 512)
> +			blk = USHRT_MAX;
> +		blk = size / 512;
> +	}
> +#endif

So, Coverity saw this and found an issue (CID 167250), and I was going
to just fix it, but I'm not sure.  The problem is that we check size >
(USHRT_MAX * 512), and then assign blk.  Then, we always assign blk, so
the test above isn't used.  But my background recollection is that
there's a real issue that's being addressed here.  Can we really just
always say blk = size / 512 in this case, or did we want to be shifting
size not blk under the if?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170927/c965f96d/attachment.sig>

[U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Bin Meng]

Hi Tom,

On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini <trini@konsulko.com> wrote:
> On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote:
>
>> When EHCD and xHCD are enabled at the same time, USB storage device
>> driver will fail to read/write from/to the storage device attached
>> to the xHCI interface, due to its transfer blocks exceeds the xHCD
>> driver limitation.
>>
>> With driver model, we have an API to get the controller's maximum
>> transfer size and we can use that to determine the storage driver's
>> capability of read/write.
>>
>> Note: the non-DM version driver is still broken with xHCD and the
>> intent here is not to fix the non-DM one, since the xHCD itself is
>> already broken in places like 3.0 hub support, etc.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> Tested-by: Stefan Roese <sr@denx.de>
> [snip]
>> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
>>  #else
>>       blk = 20;
>>  #endif
>> +#else
>> +     ret = usb_get_max_xfer_size(udev, (size_t *)&size);
>> +     if (ret < 0) {
>> +             /* unimplemented, let's use default 20 */
>> +             blk = 20;
>> +     } else {
>> +             if (size > USHRT_MAX * 512)
>> +                     blk = USHRT_MAX;
>> +             blk = size / 512;
>> +     }
>> +#endif
>
> So, Coverity saw this and found an issue (CID 167250), and I was going
> to just fix it, but I'm not sure.  The problem is that we check size >
> (USHRT_MAX * 512), and then assign blk.  Then, we always assign blk, so
> the test above isn't used.  But my background recollection is that
> there's a real issue that's being addressed here.  Can we really just
> always say blk = size / 512 in this case, or did we want to be shifting
> size not blk under the if?  Thanks!

Did this patch applied to anywhere? I see it is in the usb tree, but
not in the u-boot/master.

The fix should be:

             if (size > USHRT_MAX * 512)
                     size = USHRT_MAX * 512;

Regards,
Bin

[U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Tom Rini]

On Thu, Sep 28, 2017 at 09:34:23AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini <trini@konsulko.com> wrote:
> > On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote:
> >
> >> When EHCD and xHCD are enabled at the same time, USB storage device
> >> driver will fail to read/write from/to the storage device attached
> >> to the xHCI interface, due to its transfer blocks exceeds the xHCD
> >> driver limitation.
> >>
> >> With driver model, we have an API to get the controller's maximum
> >> transfer size and we can use that to determine the storage driver's
> >> capability of read/write.
> >>
> >> Note: the non-DM version driver is still broken with xHCD and the
> >> intent here is not to fix the non-DM one, since the xHCD itself is
> >> already broken in places like 3.0 hub support, etc.
> >>
> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >> Tested-by: Stefan Roese <sr@denx.de>
> > [snip]
> >> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
> >>  #else
> >>       blk = 20;
> >>  #endif
> >> +#else
> >> +     ret = usb_get_max_xfer_size(udev, (size_t *)&size);
> >> +     if (ret < 0) {
> >> +             /* unimplemented, let's use default 20 */
> >> +             blk = 20;
> >> +     } else {
> >> +             if (size > USHRT_MAX * 512)
> >> +                     blk = USHRT_MAX;
> >> +             blk = size / 512;
> >> +     }
> >> +#endif
> >
> > So, Coverity saw this and found an issue (CID 167250), and I was going
> > to just fix it, but I'm not sure.  The problem is that we check size >
> > (USHRT_MAX * 512), and then assign blk.  Then, we always assign blk, so
> > the test above isn't used.  But my background recollection is that
> > there's a real issue that's being addressed here.  Can we really just
> > always say blk = size / 512 in this case, or did we want to be shifting
> > size not blk under the if?  Thanks!
> 
> Did this patch applied to anywhere? I see it is in the usb tree, but
> not in the u-boot/master.
> 
> The fix should be:
> 
>              if (size > USHRT_MAX * 512)
>                      size = USHRT_MAX * 512;

It's in usb/master, and will be in master itself shortly.  Please submit
a patch that corrects this and has the Reported-by for Coverity.  Marek,
do you want to take it via your tree or should I just grab it?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170927/8d89853d/attachment.sig>

[U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Marek Vasut]

On 09/28/2017 03:39 AM, Tom Rini wrote:
> On Thu, Sep 28, 2017 at 09:34:23AM +0800, Bin Meng wrote:
>> Hi Tom,
>>
>> On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini <trini@konsulko.com> wrote:
>>> On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote:
>>>
>>>> When EHCD and xHCD are enabled at the same time, USB storage device
>>>> driver will fail to read/write from/to the storage device attached
>>>> to the xHCI interface, due to its transfer blocks exceeds the xHCD
>>>> driver limitation.
>>>>
>>>> With driver model, we have an API to get the controller's maximum
>>>> transfer size and we can use that to determine the storage driver's
>>>> capability of read/write.
>>>>
>>>> Note: the non-DM version driver is still broken with xHCD and the
>>>> intent here is not to fix the non-DM one, since the xHCD itself is
>>>> already broken in places like 3.0 hub support, etc.
>>>>
>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>> Tested-by: Stefan Roese <sr@denx.de>
>>> [snip]
>>>> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
>>>>  #else
>>>>       blk = 20;
>>>>  #endif
>>>> +#else
>>>> +     ret = usb_get_max_xfer_size(udev, (size_t *)&size);
>>>> +     if (ret < 0) {
>>>> +             /* unimplemented, let's use default 20 */
>>>> +             blk = 20;
>>>> +     } else {
>>>> +             if (size > USHRT_MAX * 512)
>>>> +                     blk = USHRT_MAX;
>>>> +             blk = size / 512;
>>>> +     }
>>>> +#endif
>>>
>>> So, Coverity saw this and found an issue (CID 167250), and I was going
>>> to just fix it, but I'm not sure.  The problem is that we check size >
>>> (USHRT_MAX * 512), and then assign blk.  Then, we always assign blk, so
>>> the test above isn't used.  But my background recollection is that
>>> there's a real issue that's being addressed here.  Can we really just
>>> always say blk = size / 512 in this case, or did we want to be shifting
>>> size not blk under the if?  Thanks!
>>
>> Did this patch applied to anywhere? I see it is in the usb tree, but
>> not in the u-boot/master.
>>
>> The fix should be:
>>
>>              if (size > USHRT_MAX * 512)
>>                      size = USHRT_MAX * 512;
> 
> It's in usb/master, and will be in master itself shortly.  Please submit
> a patch that corrects this and has the Reported-by for Coverity.  Marek,
> do you want to take it via your tree or should I just grab it?  Thanks!

A USB patch should always go through -usb , I believe that's an
established practice .

-- 
Best regards,
Marek Vasut

[U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Tom Rini]

On Thu, Sep 28, 2017 at 03:55:24AM +0200, Marek Vasut wrote:
> On 09/28/2017 03:39 AM, Tom Rini wrote:
> > On Thu, Sep 28, 2017 at 09:34:23AM +0800, Bin Meng wrote:
> >> Hi Tom,
> >>
> >> On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini <trini@konsulko.com> wrote:
> >>> On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote:
> >>>
> >>>> When EHCD and xHCD are enabled at the same time, USB storage device
> >>>> driver will fail to read/write from/to the storage device attached
> >>>> to the xHCI interface, due to its transfer blocks exceeds the xHCD
> >>>> driver limitation.
> >>>>
> >>>> With driver model, we have an API to get the controller's maximum
> >>>> transfer size and we can use that to determine the storage driver's
> >>>> capability of read/write.
> >>>>
> >>>> Note: the non-DM version driver is still broken with xHCD and the
> >>>> intent here is not to fix the non-DM one, since the xHCD itself is
> >>>> already broken in places like 3.0 hub support, etc.
> >>>>
> >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >>>> Tested-by: Stefan Roese <sr@denx.de>
> >>> [snip]
> >>>> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
> >>>>  #else
> >>>>       blk = 20;
> >>>>  #endif
> >>>> +#else
> >>>> +     ret = usb_get_max_xfer_size(udev, (size_t *)&size);
> >>>> +     if (ret < 0) {
> >>>> +             /* unimplemented, let's use default 20 */
> >>>> +             blk = 20;
> >>>> +     } else {
> >>>> +             if (size > USHRT_MAX * 512)
> >>>> +                     blk = USHRT_MAX;
> >>>> +             blk = size / 512;
> >>>> +     }
> >>>> +#endif
> >>>
> >>> So, Coverity saw this and found an issue (CID 167250), and I was going
> >>> to just fix it, but I'm not sure.  The problem is that we check size >
> >>> (USHRT_MAX * 512), and then assign blk.  Then, we always assign blk, so
> >>> the test above isn't used.  But my background recollection is that
> >>> there's a real issue that's being addressed here.  Can we really just
> >>> always say blk = size / 512 in this case, or did we want to be shifting
> >>> size not blk under the if?  Thanks!
> >>
> >> Did this patch applied to anywhere? I see it is in the usb tree, but
> >> not in the u-boot/master.
> >>
> >> The fix should be:
> >>
> >>              if (size > USHRT_MAX * 512)
> >>                      size = USHRT_MAX * 512;
> > 
> > It's in usb/master, and will be in master itself shortly.  Please submit
> > a patch that corrects this and has the Reported-by for Coverity.  Marek,
> > do you want to take it via your tree or should I just grab it?  Thanks!
> 
> A USB patch should always go through -usb , I believe that's an
> established practice .

Aside from when you tell me to just pick up a fix directly, for whatever
appropriate reason, yes.  Which is why I asked.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170927/1a8f07c6/attachment.sig>