[U-Boot] [PATCH v3] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
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@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com --- 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 | 78 ++++++++++++++++++++++++++++++++++++++++++++-------- include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..e08dcd4 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -106,11 +106,40 @@ struct us_data { * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are * limited to 65535 blocks. */ +#define USB_MIN_XFER_BLK 4095 #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 / (USB_MIN_XFER_BLK + 1))) +#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + pos)) - 1) + +static int dec_cur_xfer_blks(int *pos, unsigned short *smallblks) +{ + /* decrease the cur_xfer_blks */ + unsigned short size = (*pos > 0) ? CALC_CUR_XFER_BLKS(*pos - 1) : 0; + if (size >= USB_MIN_XFER_BLK) { + *smallblks = size; + (*pos)--; + return 0; + } + return -EINVAL; +} + +static int inc_cur_xfer_blks(int *pos, unsigned short *smallblks, lbaint_t blks) +{ + /* try to increase the cur_xfer_blks */ + unsigned short size = (*pos >= 0) ? CALC_CUR_XFER_BLKS(*pos + 1) : 0; + if (size <= blks && size <= USB_MAX_XFER_BLK) { + *smallblks = size; + (*pos)++; + return 0; + } + return -EINVAL; +} + #ifndef CONFIG_BLK static struct us_data usb_stor[USB_MAX_STOR_DEV]; #endif @@ -1117,7 +1146,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, unsigned short smallblks; struct usb_device *udev; struct us_data *ss; - int retry; + int retry, pos; + bool retry_flag = false; ccb *srb = &usb_ccb; #ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1145,6 +1175,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, buf_addr = (uintptr_t)buffer; start = blknr; blks = blkcnt; + pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %" PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr); @@ -1153,26 +1184,35 @@ 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 > udev->cur_xfer_blks) + smallblks = udev->cur_xfer_blks; else smallblks = (unsigned short) blks; retry_it: - if (smallblks == USB_MAX_XFER_BLK) + debug("usb_read: retry #%d, cur_xfer_blks %hu, smallblks %hu\n", + 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_10(srb, ss, start, smallblks)) { debug("Read ERROR\n"); usb_request_sense(srb, ss); - if (retry--) + if (retry--) { + if (!dec_cur_xfer_blks(&pos, &smallblks)) + udev->cur_xfer_blks = smallblks; + retry_flag = true; goto retry_it; + } blkcnt -= blks; break; } start += smallblks; blks -= smallblks; buf_addr += srb->datalen; + + if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks)) + udev->cur_xfer_blks = smallblks; } while (blks != 0); ss->flags &= ~USB_READY;
@@ -1181,7 +1221,7 @@ retry_it: 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; } @@ -1199,7 +1239,8 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, unsigned short smallblks; struct usb_device *udev; struct us_data *ss; - int retry; + int retry, pos; + bool retry_flag = false; ccb *srb = &usb_ccb; #ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1222,6 +1263,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, } #endif ss = (struct us_data *)udev->privptr; + pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1239,26 +1281,35 @@ 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 > udev->cur_xfer_blks) + smallblks = udev->cur_xfer_blks; else smallblks = (unsigned short) blks; retry_it: - if (smallblks == USB_MAX_XFER_BLK) + debug("usb_write: retry #%d, cur_xfer_blks %hu, smallblks %hu\n", + 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_write_10(srb, ss, start, smallblks)) { debug("Write ERROR\n"); usb_request_sense(srb, ss); - if (retry--) + if (retry--) { + if (!dec_cur_xfer_blks(&pos, &smallblks)) + udev->cur_xfer_blks = smallblks; + retry_flag = true; goto retry_it; + } blkcnt -= blks; break; } start += smallblks; blks -= smallblks; buf_addr += srb->datalen; + + if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks)) + udev->cur_xfer_blks = smallblks; } while (blks != 0); ss->flags &= ~USB_READY;
@@ -1266,7 +1317,7 @@ retry_it: PRIxPTR "\n", 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;
@@ -1331,6 +1382,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..bd49014 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;
On 06/07/2016 11:29 AM, 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@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
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 | 78 ++++++++++++++++++++++++++++++++++++++++++++-------- include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..e08dcd4 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -106,11 +106,40 @@ struct us_data {
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- limited to 65535 blocks.
*/ +#define USB_MIN_XFER_BLK 4095 #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 / (USB_MIN_XFER_BLK + 1))) +#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + pos)) - 1)
+static int dec_cur_xfer_blks(int *pos, unsigned short *smallblks) +{
- /* decrease the cur_xfer_blks */
- unsigned short size = (*pos > 0) ? CALC_CUR_XFER_BLKS(*pos - 1) : 0;
If someone passes null pointer into the function, it will fail. Also, you can refactor it to make it more readable:
if (!pos) return -EINVAL; if (size < USB_MIN_XFER_BLK) return -EINVAL; ... do stuff ...
I still don't understand why $pos isn't unsigned int, it should be.
- if (size >= USB_MIN_XFER_BLK) {
*smallblks = size;(*pos)--;return 0;- }
- return -EINVAL;
+}
+static int inc_cur_xfer_blks(int *pos, unsigned short *smallblks, lbaint_t blks) +{
- /* try to increase the cur_xfer_blks */
- unsigned short size = (*pos >= 0) ? CALC_CUR_XFER_BLKS(*pos + 1) : 0;
- if (size <= blks && size <= USB_MAX_XFER_BLK) {
*smallblks = size;(*pos)++;return 0;- }
- return -EINVAL;
+}
#ifndef CONFIG_BLK static struct us_data usb_stor[USB_MAX_STOR_DEV]; #endif @@ -1117,7 +1146,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1145,6 +1175,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, buf_addr = (uintptr_t)buffer; start = blknr; blks = blkcnt;
- pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
Is this $pos even needed ? Why ?
debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %" PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr); @@ -1153,26 +1184,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_read: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks;
The same piece of code is below. I would rather see you factor out the common code and then make any changes.
break; } start += smallblks; blks -= smallblks; buf_addr += srb->datalen;
if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1181,7 +1221,7 @@ retry_it: 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;
} @@ -1199,7 +1239,8 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1222,6 +1263,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, } #endif ss = (struct us_data *)udev->privptr;
pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1239,26 +1281,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_write: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks; break;if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1266,7 +1317,7 @@ retry_it: PRIxPTR "\n", 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;
@@ -1331,6 +1382,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..bd49014 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;
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, June 08, 2016 8:38 AM To: Rajesh Bhagat rajesh.bhagat@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; york sun york.sun@nxp.com; Sriram Dash sriram.dash@nxp.com Subject: Re: [PATCH v3] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
On 06/07/2016 11:29 AM, 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@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
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 | 78
++++++++++++++++++++++++++++++++++++++++++++--------
include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..e08dcd4 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -106,11 +106,40 @@ struct us_data {
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands
are
- limited to 65535 blocks.
*/ +#define USB_MIN_XFER_BLK 4095 #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 / (USB_MIN_XFER_BLK +
1)))
+#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + pos)) - 1)
+static int dec_cur_xfer_blks(int *pos, unsigned short *smallblks) {
- /* decrease the cur_xfer_blks */
- unsigned short size = (*pos > 0) ? CALC_CUR_XFER_BLKS(*pos - 1) : 0;
Hello Marek,
If someone passes null pointer into the function, it will fail. Also, you can refactor it to make it more readable:
if (!pos) return -EINVAL; if (size < USB_MIN_XFER_BLK) return -EINVAL; ... do stuff ...
Refactoring the code to return early in case of error conditions is fine. But pos and smallblks can never be passed as NULL in these functions.
I still don't understand why $pos isn't unsigned int, it should be.
I believe keeping a variable as "int" provides a natural way of checking for overflow conditions. We check can check if number has become negative and take actions, which is not possible when number is taken as "unsigned int".
- if (size >= USB_MIN_XFER_BLK) {
*smallblks = size;(*pos)--;return 0;- }
- return -EINVAL;
+}
+static int inc_cur_xfer_blks(int *pos, unsigned short *smallblks, +lbaint_t blks) {
- /* try to increase the cur_xfer_blks */
- unsigned short size = (*pos >= 0) ? CALC_CUR_XFER_BLKS(*pos + 1) : 0;
- if (size <= blks && size <= USB_MAX_XFER_BLK) {
*smallblks = size;(*pos)++;return 0;- }
- return -EINVAL;
+}
#ifndef CONFIG_BLK static struct us_data usb_stor[USB_MAX_STOR_DEV]; #endif @@ -1117,7 +1146,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev,
lbaint_t blknr,
unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1145,6 +1175,7 @@ static unsigned long usb_stor_read(struct blk_desc
*block_dev, lbaint_t blknr,
buf_addr = (uintptr_t)buffer; start = blknr; blks = blkcnt;
- pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
Is this $pos even needed ? Why ?
$pos variable is updated in dec/inc_cur_xfer_blks functions which calculates the next transfer blocks, and finally when all conditions are met is stored in udev->cur_xfer_blks.
debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %" PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr); @@ -1153,26 +1184,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_read: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks;The same piece of code is below. I would rather see you factor out the common code and then make any changes.
IMHO, All the common code is moved in functions dec/inc_cur_xfer_blks as pointed earlier. Are you are referring to other common code of usb_stor_read/write functions which is not part of this patchset. If yes, I would prefer to take it as a next patch set.
Best Regards, Rajesh Bhagat
break; } start += smallblks; blks -= smallblks; buf_addr += srb->datalen;
if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1181,7 +1221,7 @@ retry_it: 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;
} @@ -1199,7 +1239,8 @@ static unsigned long usb_stor_write(struct blk_desc
*block_dev, lbaint_t blknr,
unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1222,6 +1263,7 @@ static unsigned long usb_stor_write(struct blk_desc
*block_dev, lbaint_t blknr,
} #endif ss = (struct us_data *)udev->privptr;
pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1239,26 +1281,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_write: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks; break;if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1266,7 +1317,7 @@ retry_it: PRIxPTR "\n", 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;
@@ -1331,6 +1382,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..bd49014 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;
-- Best regards, Marek Vasut
On 06/08/2016 06:09 AM, Rajesh Bhagat wrote:
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, June 08, 2016 8:38 AM To: Rajesh Bhagat rajesh.bhagat@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; york sun york.sun@nxp.com; Sriram Dash sriram.dash@nxp.com Subject: Re: [PATCH v3] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
On 06/07/2016 11:29 AM, 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@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
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 | 78
++++++++++++++++++++++++++++++++++++++++++++--------
include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..e08dcd4 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -106,11 +106,40 @@ struct us_data {
- enough free heap space left, but the SCSI READ(10) and WRITE(10) commands
are
- limited to 65535 blocks.
*/ +#define USB_MIN_XFER_BLK 4095 #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 / (USB_MIN_XFER_BLK +
1)))
+#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + pos)) - 1)
+static int dec_cur_xfer_blks(int *pos, unsigned short *smallblks) {
- /* decrease the cur_xfer_blks */
- unsigned short size = (*pos > 0) ? CALC_CUR_XFER_BLKS(*pos - 1) : 0;
Hello Marek,
If someone passes null pointer into the function, it will fail. Also, you can refactor it to make it more readable:
if (!pos) return -EINVAL; if (size < USB_MIN_XFER_BLK) return -EINVAL; ... do stuff ...
Refactoring the code to return early in case of error conditions is fine. But pos and smallblks can never be passed as NULL in these functions.
... until an idiot comes around and does it.
I still don't understand why $pos isn't unsigned int, it should be.
I believe keeping a variable as "int" provides a natural way of checking for overflow conditions. We check can check if number has become negative and take actions, which is not possible when number is taken as "unsigned int".
But the number here can never be negative, can it ?
- if (size >= USB_MIN_XFER_BLK) {
*smallblks = size;(*pos)--;return 0;- }
- return -EINVAL;
+}
+static int inc_cur_xfer_blks(int *pos, unsigned short *smallblks, +lbaint_t blks) {
- /* try to increase the cur_xfer_blks */
- unsigned short size = (*pos >= 0) ? CALC_CUR_XFER_BLKS(*pos + 1) : 0;
- if (size <= blks && size <= USB_MAX_XFER_BLK) {
*smallblks = size;(*pos)++;return 0;- }
- return -EINVAL;
+}
#ifndef CONFIG_BLK static struct us_data usb_stor[USB_MAX_STOR_DEV]; #endif @@ -1117,7 +1146,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev,
lbaint_t blknr,
unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1145,6 +1175,7 @@ static unsigned long usb_stor_read(struct blk_desc
*block_dev, lbaint_t blknr,
buf_addr = (uintptr_t)buffer; start = blknr; blks = blkcnt;
- pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
Is this $pos even needed ? Why ?
$pos variable is updated in dec/inc_cur_xfer_blks functions which calculates the next transfer blocks, and finally when all conditions are met is stored in udev->cur_xfer_blks.
And why doesn't this work with udev->cur_xfer_blks directly ?
debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %" PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr); @@ -1153,26 +1184,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_read: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks;The same piece of code is below. I would rather see you factor out the common code and then make any changes.
IMHO, All the common code is moved in functions dec/inc_cur_xfer_blks as pointed earlier. Are you are referring to other common code of usb_stor_read/write functions which is not part of this patchset. If yes, I would prefer to take it as a next patch set.
Please don't add new stuff to code which needs fixing first, that's not a good practice. Prepare the ground first and then add new stuff.
Best Regards, Rajesh Bhagat
break; } start += smallblks; blks -= smallblks; buf_addr += srb->datalen;
if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1181,7 +1221,7 @@ retry_it: 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;
} @@ -1199,7 +1239,8 @@ static unsigned long usb_stor_write(struct blk_desc
*block_dev, lbaint_t blknr,
unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1222,6 +1263,7 @@ static unsigned long usb_stor_write(struct blk_desc
*block_dev, lbaint_t blknr,
} #endif ss = (struct us_data *)udev->privptr;
pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1239,26 +1281,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_write: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks; break;if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1266,7 +1317,7 @@ retry_it: PRIxPTR "\n", 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;
@@ -1331,6 +1382,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..bd49014 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;
-- Best regards, Marek Vasut
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, June 08, 2016 7:25 PM To: Rajesh Bhagat rajesh.bhagat@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; york sun york.sun@nxp.com; Sriram Dash sriram.dash@nxp.com Subject: Re: [PATCH v3] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
On 06/08/2016 06:09 AM, Rajesh Bhagat wrote:
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, June 08, 2016 8:38 AM To: Rajesh Bhagat rajesh.bhagat@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; york sun york.sun@nxp.com; Sriram Dash sriram.dash@nxp.com Subject: Re: [PATCH v3] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
On 06/07/2016 11:29 AM, 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@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
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 | 78
++++++++++++++++++++++++++++++++++++++++++++--------
include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..e08dcd4 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -106,11 +106,40 @@ struct us_data {
- enough free heap space left, but the SCSI READ(10) and WRITE(10)
commands
are
- limited to 65535 blocks.
*/ +#define USB_MIN_XFER_BLK 4095 #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 / (USB_MIN_XFER_BLK +
1)))
+#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + pos)) - 1)
+static int dec_cur_xfer_blks(int *pos, unsigned short *smallblks) {
- /* decrease the cur_xfer_blks */
- unsigned short size = (*pos > 0) ? CALC_CUR_XFER_BLKS(*pos - 1) :
+0;
Hello Marek,
If someone passes null pointer into the function, it will fail. Also, you can refactor it to make it more readable:
if (!pos) return -EINVAL; if (size < USB_MIN_XFER_BLK) return -EINVAL; ... do stuff ...
Refactoring the code to return early in case of error conditions is fine. But pos and smallblks can never be passed as NULL in these functions.
Hello Marek,
... until an idiot comes around and does it.
Ok, I will add the mentioned sanity checks in v4.
I still don't understand why $pos isn't unsigned int, it should be.
I believe keeping a variable as "int" provides a natural way of checking for overflow conditions. We check can check if number has become negative and take actions, which is not possible when number is taken as
"unsigned int".
But the number here can never be negative, can it ?
Ok, I will change the variable to unsigned int in v4.
- if (size >= USB_MIN_XFER_BLK) {
*smallblks = size;(*pos)--;return 0;- }
- return -EINVAL;
+}
+static int inc_cur_xfer_blks(int *pos, unsigned short *smallblks, +lbaint_t blks) {
- /* try to increase the cur_xfer_blks */
- unsigned short size = (*pos >= 0) ? CALC_CUR_XFER_BLKS(*pos + 1) :
0;
- if (size <= blks && size <= USB_MAX_XFER_BLK) {
*smallblks = size;(*pos)++;return 0;- }
- return -EINVAL;
+}
#ifndef CONFIG_BLK static struct us_data usb_stor[USB_MAX_STOR_DEV]; #endif @@ -1117,7 +1146,8 @@ static unsigned long usb_stor_read(struct blk_desc +*block_dev,
lbaint_t blknr,
unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1145,6 +1175,7 @@ static unsigned long usb_stor_read(struct blk_desc
*block_dev, lbaint_t blknr,
buf_addr = (uintptr_t)buffer; start = blknr; blks = blkcnt;
- pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
Is this $pos even needed ? Why ?
$pos variable is updated in dec/inc_cur_xfer_blks functions which calculates the next transfer blocks, and finally when all conditions are met is stored
in udev->cur_xfer_blks.
And why doesn't this work with udev->cur_xfer_blks directly ?
Let me try to check if we can work without this in v4.
debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %" PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr); @@ -1153,26 +1184,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_read: retry #%d, cur_xfer_blks %hu, smallblks%hu\n",
retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks;The same piece of code is below. I would rather see you factor out the common code and then make any changes.
IMHO, All the common code is moved in functions dec/inc_cur_xfer_blks as pointed
earlier.
Are you are referring to other common code of usb_stor_read/write functions which is not part of this patchset. If yes, I would prefer to take it as a next
patch set.
Please don't add new stuff to code which needs fixing first, that's not a good practice. Prepare the ground first and then add new stuff.
Let me give it a try to make common function for usb_stor_read/write in v4.
Best Regards, Rajesh Bhagat
Best Regards, Rajesh Bhagat
break; } start += smallblks; blks -= smallblks; buf_addr += srb->datalen;
if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1181,7 +1221,7 @@ retry_it: 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;
} @@ -1199,7 +1239,8 @@ static unsigned long usb_stor_write(struct blk_desc
*block_dev, lbaint_t blknr,
unsigned short smallblks; struct usb_device *udev; struct us_data *ss;
- int retry;
- int retry, pos;
- bool retry_flag = false; ccb *srb = &usb_ccb;
#ifdef CONFIG_BLK struct blk_desc *block_dev; @@ -1222,6 +1263,7 @@ static unsigned long usb_stor_write(struct blk_desc
*block_dev, lbaint_t blknr,
} #endif ss = (struct us_data *)udev->privptr;
pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
usb_disable_asynch(1); /* asynch transfer not allowed */
@@ -1239,26 +1281,35 @@ 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 > udev->cur_xfer_blks)smallblks = udev->cur_xfer_blks;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_write: retry #%d, cur_xfer_blks %hu, smallblks%hu\n",
retry, udev->cur_xfer_blks, smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(&pos, &smallblks))udev->cur_xfer_blks = smallblks;retry_flag = true; goto retry_it;} blkcnt -= blks; break;if (!retry_flag && !inc_cur_xfer_blks(&pos, &smallblks, blks))udev->cur_xfer_blks = smallblks;@@ -1266,7 +1317,7 @@ retry_it: PRIxPTR "\n", 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;
@@ -1331,6 +1382,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..bd49014 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;
-- Best regards, Marek Vasut
-- Best regards, Marek Vasut
participants (2)
-
Marek Vasut -
Rajesh Bhagat