[U-Boot] comments: [PATCH v7 3/3] common: usb_storage: Implement logic to calculate optimal usb maximum trasfer blocks

Hi Rajesh & Marek

I have spend the last couple of days testing this patch and have a few review comments:

On 06/16/2016 12:35 PM, Rajesh Bhagat wrote:

Implements the logic to calculate the optimal usb maximum trasfer blocks instead of sending USB_MAX_XFER_BLK blocks which is 65535 and 20 in case of EHCI and other USB protocols respectively.

It defines USB_MIN_XFER_BLK/USB_MAX_XFER_BLK trasfer blocks that should be checked for success starting from minimum to maximum, and rest of the read/write are performed with that optimal value. It tries to increase/ decrease the blocks in follwing scenarios:

1.decrease blocks: when read/write for a particular number of blocks fails. 2. increase blocks: when read/write for a particular number of blocks pass and amount left to trasfer is greater than current number of blocks.

Currently changes are done for EHCI where min = 4096 and max = 65535 is taken. And for other cases code is left unchanged by keeping min = max = 20.

Signed-off-by: Sriram Dash <sriram.dash at nxp.com> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at nxp.com> Reviewed-by: Simon Glass <sjg at chromium.org>

---

Changes in v7:

• None

Changes in v6:

• Adds paranthesis around macro variables
• Removes extra ternary operator from dec_cur_xfer_blks
• Clamps the size to min(blks, USB_MAX_XFER_BLK) in inc_cur_xfer_blks

Changes in v5:

• None

Changes in v4:

• Adds udev paramater in dec/inc_cur_xfer_blks function and adds sanity check on it.
• Changes type of pos varible to unsigned int in dec/inc_cur_xfer_blks
• Removes usage of pos varible from usb_stor_read/write

Changes in v3:

• Adds cur_xfer_blks in struct usb_device to retain values
• Adds functions dec/inc_cur_xfer_blks to remove code duplication
• Moves check from macro to calling functions

Changes in v2:

• Removes table to store blocks and use formula (1 << (12 + n)) - 1
• Adds logic to start from minimum, go to maximum in each read/write

common/usb_storage.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++---- include/usb.h | 1 + 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c index a7d84bf..93b901c 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -106,11 +106,16 @@ struct us_data {

• enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
• limited to 65535 blocks.

*/

Should this also include CONFIG_USB_XHCI? It appears that it is currently limited to the fixed 20 blocks.

+#define USB_MIN_XFER_BLK 4095

Where did this number 4095 come from; is 4095 blocks low enough? It surprises me that USB_MIN_XFER_BLK has been set so high, when the linux default is 240 blocks and the windows default is 128 blocks. Further, there appears to be some devices that will only support up to 64 blocks.

http://lists.denx.de/pipermail/u-boot/2016-February/246250.html

Maybe we could define a mid value for the transfer to start at; giving room to move in both directions dependent on the outcome of the transfer:

#define USB_MIN_XFER_BLK 64 #define USB_MID_XFER_BLK 4095 #define USB_MAX_XFER_BLK 65535

However the 5 second delay incurred for each ehci timeout would make working all the way down to 64 blocks painfully slow. Maybe if the first transfer fails (at 4095 blocks) then it should jump straight to 64 blocks and work its way up on each success.

#define USB_MAX_XFER_BLK 65535 #else +#define USB_MIN_XFER_BLK 20 #define USB_MAX_XFER_BLK 20 #endif

+#define GET_CUR_XFER_BLKS(blks) (LOG2(((blks) + 1) / (USB_MIN_XFER_BLK + 1))) +#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + (pos))) - 1)

#ifndef CONFIG_BLK static struct us_data usb_stor[USB_MAX_STOR_DEV]; #endif @@ -141,6 +146,44 @@ static void usb_show_progress(void) debug("."); }

+static int dec_cur_xfer_blks(struct usb_device *udev) +{

• /* decrease the cur_xfer_blks */
  

• unsigned int pos;
  

• unsigned short size;
  

• if (!udev)
  

•         return -EINVAL;
  

• pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
  

• size  = CALC_CUR_XFER_BLKS(pos - 1);
  

• if (size < USB_MIN_XFER_BLK)
  

•         return -EINVAL;
  

• udev->cur_xfer_blks = size;
  

• return 0;
  

+}

+static int inc_cur_xfer_blks(struct usb_device *udev, lbaint_t blks) +{

• /* try to increase the cur_xfer_blks */
  

• unsigned int pos;
  

• unsigned short size;
  

• if (!udev)
  

•         return -EINVAL;
  

• pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
  

• size = CALC_CUR_XFER_BLKS(pos + 1);
  

• if (size > min(blks, (lbaint_t)USB_MAX_XFER_BLK))
  

•         return -EINVAL;
  

• udev->cur_xfer_blks = size;
  

• return 0;
  

+}

/*******************************************************************************

• show info on storage devices; 'usb start/init' must be invoked earlier
• as we only retrieve structures populated during devices initialization

@@ -1102,6 +1145,7 @@ static unsigned long usb_stor_read_write(struct blk_desc *block_dev, struct usb_device *udev; struct us_data *ss; int retry;

• bool retry_flag = false;
  ccb *srb = &usb_ccb;
  

#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1141,26 +1185,35 @@ static unsigned long usb_stor_read_write(struct blk_desc *block_dev, */ retry = 2; srb->pdata = (unsigned char *)buf_addr;

•         if (blks > USB_MAX_XFER_BLK)
  

•                 smallblks = USB_MAX_XFER_BLK;
  

•         if (blks > udev->cur_xfer_blks)
  

•                 smallblks = udev->cur_xfer_blks;
          else
                  smallblks = (unsigned short) blks;
  

retry_it:

•         if (smallblks == USB_MAX_XFER_BLK)
  

•         debug("%s: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",
  

•               __func__, retry, udev->cur_xfer_blks, smallblks);
  

•         if (smallblks == udev->cur_xfer_blks)
                  usb_show_progress();
          srb->datalen = block_dev->blksz * smallblks;
          srb->pdata = (unsigned char *)buf_addr;
          if (usb_read_write_10(srb, ss, start, smallblks, is_write)) {
                  debug("%s ERROR\n", __func__);
                  usb_request_sense(srb, ss);
  

•                 if (retry--)
  

•                 if (retry--) {
  

•                         if (!dec_cur_xfer_blks(udev))
  

•                                 smallblks = udev->cur_xfer_blks;
  

There is a potential error here:

1. A previous transfer has set udev->cur_xfer_blks=8191. 2. A current transfer with smallblks=10 fails (for whatever reason). 3. Both udev->cur_xfer_blks and smallblks are set to 4095 blocks. 4. The retry transfer will now occur with 4085 too many blocks.

The smallblks should not be assigned here; instead the retry_it label should be moved to include the the four lines of code above it, which take into account the current blks value.

•                         retry_flag = true;
                          goto retry_it;
  

•                 }
                  blkcnt -= blks;
                  break;
          }
          start += smallblks;
          blks -= smallblks;
          buf_addr += srb->datalen;
  

•         if (!retry_flag && !inc_cur_xfer_blks(udev, blks))
  

There is a potential error here:

1. A previous transfer has set udev->cur_xfer_blks=8191, after a transfer with 16383 blocks caused an ehci time out error. 2. As retry_flag has been defined locally, all new transfers will again attempt to use 16383, which will cause another ehci time our error (each new transfer incurring another 5 second delay).

I am not sure if this is the intended behavior. Maybe we do want to reattempt larger values on new transfers to weed out erroneous errors, however a 5 second delay on every new transfer is a steep price to pay for this. I suspect that once we have fallen back to a lower value, that we will want to stick to that lower value for all future transfers (on that specific device).

A solution could be to replace the retry_flag with something like dev->xfer_throttled or dev->xfer_rate_limited.

•                 smallblks = udev->cur_xfer_blks;
  } while (blks != 0);
  ss->flags &= ~USB_READY;
  
  

@@ -1169,7 +1222,7 @@ retry_it: __func__, start, smallblks, buf_addr);

  usb_disable_asynch(0); /* asynch transfer allowed */

• if (blkcnt >= USB_MAX_XFER_BLK)
  

• if (blkcnt >= udev->cur_xfer_blks)
          debug("\n");
  return blkcnt;
  

} @@ -1256,6 +1309,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum, break; }

• /* Initialize the current transfer blocks to minimum value */
  

• dev->cur_xfer_blks = USB_MIN_XFER_BLK;
  

• /*
   * We are expecting a minimum of 2 endpoints - in and out (bulk).
   * An optional interrupt is OK (necessary for CBI protocol).
  

diff --git a/include/usb.h b/include/usb.h index 02a0ccd..b815816 100644 --- a/include/usb.h +++ b/include/usb.h @@ -153,6 +153,7 @@ struct usb_device { struct udevice *dev; /* Pointer to associated device */ struct udevice *controller_dev; /* Pointer to associated controller */ #endif

• unsigned short cur_xfer_blks;   /* Current maximum transfer blocks */
  

};

struct int_queue;

2.6.2.198.g614a2ac

Cheers. - Matt Bright