common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
PATCH v3 1/1 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.
---
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(-)
common/usb_storage.c +66 -12
@@ -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).
include/usb.h +1 -0
@@ -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 ;
Signature · 1 line
2.6.2.198.g614a2ac
PATCH v3 1/1 common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
Quoted 47 lines · Rajesh Bhagat
Implements the logic to calculate the optimal usb maximum trasfer blocks instead of sending
USB_MAX_XFER_BLKblocks 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 <[email protected]>Signed-off-by Rajesh Bhagat <[email protected]>---
Changes in v3:
- Addscur_xfer_blksin structusb_deviceto retain values
- Adds functions dec/inc_cur_xfer_blks to remove code duplication
- Moves check from macro to calling functionsChanges 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/writecommon/usb_storage.c | 78 ++++++++++++++++++++++++++++++++++++++++++++-------- include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)common/usb_storage.c +2
-0@@ -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
common/usb_storage.c struct us_data { · line 122
Quoted 10 lines
common/usb_storage.c +7
-0#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.
Quoted 34 lines
common/usb_storage.c +22
-1+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;
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1178
Quoted 4 lines
common/usb_storage.c +1
-0@@ -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 ?
Quoted 21 lines
common/usb_storage.c +5
-3debug ("\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)) {
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1207
Quoted 10 lines
common/usb_storage.c +5
-1debug ("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;
The same piece of code is below. I would rather see you factor out the common code and then make any changes.
Quoted 110 lines
common/usb_storage.c +24
-7break ; } 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). include/usb.h +1
-0@@ -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 ;
Signature · 2 lines
Best regards, Marek Vasut
PATCH v3 1/1 common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
Quoted 64 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 08, 2016 8:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: sjg at chromium.org; york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH v3] common:usb_storage: Implement logic to calculate optimal usb maximum trasfer blocksQuoted 27 lines · Rajesh Bhagat
Implements the logic to calculate the optimal usb maximum trasfer blocks instead of sending
USB_MAX_XFER_BLKblocks 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 <[email protected]>Signed-off-by Rajesh Bhagat <[email protected]>---
Changes in v3:
- Addscur_xfer_blksin structusb_deviceto retain values
- Adds functions dec/inc_cur_xfer_blks to remove code duplication
- Moves check from macro to calling functionsQuoted 5 lines
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/writecommon/usb_storage.c | 78++++++++++++++++++++++++++++++++++++++++++++--------
common/usb_storage.c struct us_data { · line 106
Quoted 9 lines
include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)common/usb_storage.c index renamed from common/usb_storage.c +0
-07e6e52d..e08dcd4 100644
common/usb_storage.c +0
-0@@ -106,11 +106,40 @@ struct us_data {* enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
Quoted 9 lines
common/usb_storage.c +2
-0* 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
Quoted 8 lines
common/usb_storage.c struct us_data { · line 116
Quoted 1 line
common/usb_storage.c +1
-0+#define GET_CUR_XFER_BLKS (blks) (LOG2(blks / (USB_MIN_XFER_BLK +1)))
common/usb_storage.c struct us_data { · line 122
Quoted 5 lines
common/usb_storage.c +5
-0+#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,
Quoted 9 lines
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.
Quoted 2 lines
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".
Quoted 35 lines
Quoted 13 lines
common/usb_storage.c +13
-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 ) {common/usb_storage.c struct us_data {
Quoted 10 lines
common/usb_storage.c +8
-0+ *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_tblknr,common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc
Quoted 10 lines
common/usb_storage.c +0
-0unsigned short smallblks;common/usb_storage.c +2
-1struct 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_tblknr,
Quoted 6 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1178
Quoted 4 lines
common/usb_storage.c +1
-0buf_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.
Quoted 28 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc · line 1150
Quoted 4 lines
common/usb_storage.c +0
-1debug ("\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_tblknr,Quoted 23 lines
common/usb_storage.c +7
-4/* 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))
Quoted 9 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1207
Quoted 5 lines
common/usb_storage.c +3
-0+ 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
Quoted 119 lines
common/usb_storage.c static unsigned long usb_stor_write(struct blk_desc
Quoted 21 lines
common/usb_storage.c +4
-1break ; } 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_tblknr,common/usb_storage.c static unsigned long usb_stor_write(struct blk_desc
Quoted 10 lines
common/usb_storage.c +2
-1unsigned 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_tblknr,common/usb_storage.c static unsigned long usb_stor_write(struct blk_desc
Quoted 8 lines
common/usb_storage.c +1
-0}#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_tblknr,common/usb_storage.c int usb_storage_probe(struct usb_device *dev, unsigned
Quoted 49 lines
common/usb_storage.c +14
-5*/ 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, unsignedint ifnum,
Quoted 15 lines
common/usb_storage.c +3
-0break ; } +/* 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). include/usb.h index 02a0ccd..bd49014 renamed from include/usb.h +0
-0100644
include/usb.h struct usb_device { · line 159
Quoted 7 lines
include/usb.h +1
-0@@ -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
PATCH v3 1/1 common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
Quoted 85 lines · Rajesh Bhagat
Quoted 64 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 08, 2016 8:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: sjg at chromium.org; york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH v3] common:usb_storage: Implement logic to calculate optimal usb maximum trasfer blocksQuoted 27 lines · Rajesh Bhagat
Implements the logic to calculate the optimal usb maximum trasfer blocks instead of sending
USB_MAX_XFER_BLKblocks 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 <[email protected]>Signed-off-by Rajesh Bhagat <[email protected]>---
Changes in v3:
- Addscur_xfer_blksin structusb_deviceto retain values
- Adds functions dec/inc_cur_xfer_blks to remove code duplication
- Moves check from macro to calling functionsQuoted 5 lines
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/writecommon/usb_storage.c | 78++++++++++++++++++++++++++++++++++++++++++++--------
common/usb_storage.c struct us_data { · line 106
Quoted 9 lines
include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)common/usb_storage.c index renamed from common/usb_storage.c +0
-07e6e52d..e08dcd4 100644
common/usb_storage.c +0
-0@@ -106,11 +106,40 @@ struct us_data {* enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
Quoted 9 lines
common/usb_storage.c +2
-0* 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 Quoted 8 lines
common/usb_storage.c struct us_data { · line 116
Quoted 1 line
common/usb_storage.c +1
-0+#define GET_CUR_XFER_BLKS (blks) (LOG2(blks / (USB_MIN_XFER_BLK +1)))
common/usb_storage.c struct us_data { · line 122
Quoted 5 lines
common/usb_storage.c +5
-0+#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,
Quoted 9 lines
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 ...
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
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 ?
Quoted 42 lines
Quoted 35 lines
Quoted 13 lines
common/usb_storage.c +13
-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 ) {common/usb_storage.c struct us_data {
Quoted 10 lines
common/usb_storage.c +8
-0+ *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_tblknr,common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc
Quoted 10 lines
common/usb_storage.c +0
-0unsigned short smallblks;common/usb_storage.c +2
-1struct 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_tblknr,Quoted 6 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1178
Quoted 4 lines
common/usb_storage.c +1
-0buf_addr = (uintptr_t )buffer; start = blknr; blks = blkcnt; + pos =GET_CUR_XFER_BLKS (udev->cur_xfer_blks);Is this $pos even needed ? Why ?
Quoted 2 lines
$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 ?
Quoted 38 lines
Quoted 28 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc · line 1150
Quoted 4 lines
common/usb_storage.c +0
-1debug ("\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_tblknr,Quoted 23 lines
common/usb_storage.c +7
-4/* 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))Quoted 9 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1207
Quoted 5 lines
common/usb_storage.c +3
-0+ 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.
Quoted 3 lines
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.
Quoted 122 lines
Best Regards,
Rajesh BhagatQuoted 119 lines
common/usb_storage.c static unsigned long usb_stor_write(struct blk_desc
Quoted 21 lines
common/usb_storage.c +4
-1break ; } 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_tblknr,common/usb_storage.c static unsigned long usb_stor_write(struct blk_desc
Quoted 10 lines
common/usb_storage.c +2
-1unsigned 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_tblknr,common/usb_storage.c static unsigned long usb_stor_write(struct blk_desc
Quoted 8 lines
common/usb_storage.c +1
-0}#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_tblknr,common/usb_storage.c int usb_storage_probe(struct usb_device *dev, unsigned
Quoted 49 lines
common/usb_storage.c +14
-5*/ 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, unsignedint ifnum,
Quoted 15 lines
common/usb_storage.c +3
-0break ; } +/* 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). include/usb.h index 02a0ccd..bd49014 renamed from include/usb.h +0
-0100644
include/usb.h struct usb_device { · line 159
Quoted 7 lines
include/usb.h +1
-0@@ -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
Signature · 2 lines
Best regards, Marek Vasut
PATCH v3 1/1 common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
Quoted 11 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 08, 2016 7:25 PM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: sjg at chromium.org; york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH v3] common:usb_storage: Implement logic to calculate optimal usb maximum trasfer blocks
Quoted 92 lines
Quoted 90 lines · Rajesh Bhagat
Quoted 66 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, June 08, 2016 8:38 AM
To: Rajesh Bhagat <[email protected]>; u-boot at lists.denx.de
Cc: sjg at chromium.org; york sun <[email protected]>; Sriram Dash <[email protected]>
Subject: Re: [PATCH v3] common:usb_storage: Implement logic to calculate optimal usb maximum trasfer blocksQuoted 27 lines · Rajesh Bhagat
Implements the logic to calculate the optimal usb maximum trasfer blocks instead of sending
USB_MAX_XFER_BLKblocks 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 <[email protected]>Signed-off-by Rajesh Bhagat <[email protected]>---
Changes in v3:
- Addscur_xfer_blksin structusb_deviceto retain values
- Adds functions dec/inc_cur_xfer_blks to remove code duplication
- Moves check from macro to calling functionsQuoted 6 lines
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/writecommon/usb_storage.c | 78++++++++++++++++++++++++++++++++++++++++++++--------
Quoted 10 lines
include/usb.h | 1 + 2 files changed, 67 insertions(+), 12 deletions(-)common/usb_storage.c index renamed from common/usb_storage.c +0
-07e6e52d..e08dcd4 100644
common/usb_storage.c +0
-0@@ -106,11 +106,40 @@ struct us_data {* enough free heap space left, but the SCSI READ(10) and WRITE(10) commands
are
Quoted 9 lines
* limited to 65535 blocks.
*/common/usb_storage.c +2
-0+#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 Quoted 9 lines
Quoted 1 line
+#define
GET_CUR_XFER_BLKS(blks)(LOG2(blks / (USB_MIN_XFER_BLK+1)))
common/usb_storage.c struct us_data {
Quoted 6 lines
common/usb_storage.c +6
-0+#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,
Quoted 9 lines
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,
Quoted 2 lines
... until an idiot comes around and does it.
Ok, I will add the mentioned sanity checks in v4.
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 47 lines
Quoted 13 lines
Quoted 13 lines
common/usb_storage.c struct us_data {
Quoted 13 lines
common/usb_storage.c +12
-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;
Quoted 33 lines
Quoted 32 lines
common/usb_storage.c struct us_data {
Quoted 13 lines
common/usb_storage.c +10
-1+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_tblknr,Quoted 11 lines
common/usb_storage.c +2
-1unsigned 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_tblknr,common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1178
Quoted 4 lines
common/usb_storage.c +1
-0buf_addr = (uintptr_t )buffer; start = blknr; blks = blkcnt; + pos =GET_CUR_XFER_BLKS (udev->cur_xfer_blks);Is this $pos even needed ? Why ?
Quoted 6 lines
Quoted 2 lines
$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_blksdirectly ?
Let me try to check if we can work without this in v4.
Quoted 40 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
common/usb_storage.c +0
-1debug ("\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_tblknr,Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
common/usb_storage.c +0
-1/* XXX need some comment here */ retry =2 ; srb->pdata = (unsigned char *)buf_addr; -if (blks >USB_MAX_XFER_BLK )Quoted 8 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
Quoted 8 lines
common/usb_storage.c +3
-2- 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",
Quoted 21 lines
Quoted 20 lines
Quoted 6 lines
common/usb_storage.c +2
-0+ 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)) {common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1207
Quoted 10 lines
common/usb_storage.c +5
-1debug ("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;The same piece of code is below. I would rather see you factor out the common code and then make any changes.
Quoted 9 lines
Quoted 1 line
IMHO, All the common code is moved in functions dec/inc_cur_xfer_blks as pointed
earlier.
Quoted 2 lines
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 nextpatch 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
Quoted 132 lines
Quoted 48 lines
Best Regards,
Rajesh BhagatQuoted 45 lines
Quoted 21 lines
common/usb_storage.c +4
-1break ; } 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(structQuoted 1 line
blk_desc
*block_dev,lbaint_tblknr,Quoted 11 lines
common/usb_storage.c +2
-1unsigned 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_tblknr,Quoted 9 lines
common/usb_storage.c +1
-0}#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_tblknr,Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
common/usb_storage.c +0
-1*/ retry =2 ; srb->pdata = (unsigned char *)buf_addr; -if (blks >USB_MAX_XFER_BLK )Quoted 8 lines
common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
Quoted 8 lines
common/usb_storage.c +3
-2- 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",
Quoted 59 lines
Quoted 59 lines
Quoted 37 lines
common/usb_storage.c +11
-2+ 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,Quoted 1 line
unsigned
int ifnum,
Quoted 20 lines
common/usb_storage.c +3
-0break ; } +/* 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). include/usb.h index 02a0ccd..bd49014 renamed from include/usb.h +0
-0100644
include/usb.h +1
-0@@ -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 */ };Quoted 7 lines
Quoted 7 lines
include/usb.h struct usb_device { · line 159
Quoted 2 lines
include/usb.h +0
-0struct int_queue ;--
Best regards,
Marek Vasut--
Best regards,
Marek Vasut