dm: usb: Add a new USB controller operation 'get_max_xfer_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.

Signed-off-by Bin Meng <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Tested-by Stefan Roese <[email protected]>

common/usb_storage.c +1 -0

@@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
 #else
 	blk = 20;
 #endif
+#else

Quoted 15 lines · 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 <[email protected]>

Tested-by Stefan Roese <[email protected]>

[snip]

Quoted 5 lines

common/usb_storage.c +1 -0

@@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
 #else
      blk = 20;
 #endif
+#else

common/usb_storage.c static void usb_stor_set_max_xfer_blk(struct us_data *us) · line 970

Quoted 10 lines

common/usb_storage.c +10 -0

+     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!

Hi Tom,

Quoted 34 lines · Tom Rini

Quoted 15 lines · 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 <[email protected]>

Tested-by Stefan Roese <[email protected]>

[snip]

Quoted 5 lines

common/usb_storage.c +1 -0

@@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
 #else
      blk = 20;
 #endif
+#else

common/usb_storage.c static void usb_stor_set_max_xfer_blk(struct us_data *us) · line 970

Quoted 10 lines

common/usb_storage.c +10 -0

+     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

Quoted 7 lines

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;

Quoted 48 lines · Bin Meng

Hi Tom,

Quoted 34 lines · Tom Rini

Quoted 15 lines · 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 <[email protected]>

Tested-by Stefan Roese <[email protected]>

[snip]

Quoted 5 lines

common/usb_storage.c +1 -0

@@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
 #else
      blk = 20;
 #endif
+#else

common/usb_storage.c static void usb_stor_set_max_xfer_blk(struct us_data *us) · line 970

Quoted 10 lines

common/usb_storage.c +10 -0

+     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

Quoted 7 lines

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.

Quoted 4 lines

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!

Quoted 54 lines · Tom Rini

Quoted 52 lines · Bin Meng

Hi Tom,

Quoted 34 lines · Tom Rini

Quoted 15 lines · 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 <[email protected]>

Tested-by Stefan Roese <[email protected]>

[snip]

Quoted 5 lines

common/usb_storage.c +1 -0

@@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
 #else
      blk = 20;
 #endif
+#else

common/usb_storage.c static void usb_stor_set_max_xfer_blk(struct us_data *us) · line 970

Quoted 10 lines

common/usb_storage.c +10 -0

+     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

Quoted 7 lines

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;

Quoted 3 lines

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 .

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 <[email protected]>

---

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

drivers/usb/host/usb-uclass.c +11 -0

index 0b8a501..bc44fc3 100644
@@ -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;

include/usb.h +21 -1

index fad0401..0ddc082 100644
@@ -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