common: usb_storage : Implement logic to calculate optimal

Performs code cleanup by making common function for usb_stor_read/ usb_stor_write. Currently only difference in these fucntions is call to usb_read_10/usb_write_10 scsi commands.

Signed-off-by Rajesh Bhagat <[email protected]>

---
Changes in v5:
- Converts USB_STOR_OPERATION_FUNC macro to a function
- Corrects the multi line comment accroding to coding style
- Renames variable to dev to remove code duplication

Changes in v4:
- Adds code to make common function for read/write

 common/usb_storage.c |  129 +++++++++++++++----------------------------------
 1 files changed, 40 insertions(+), 89 deletions(-)

common/usb_storage.c +0 -0

index 7e6e52d..f060637 100644

common/usb_storage.c +8 -4

+}
+
 #ifdef CONFIG_BLK
-static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
-				   lbaint_t blkcnt, void *buffer)
+static unsigned long usb_stor_read_write(struct udevice *dev, lbaint_t blknr,
+					 lbaint_t blkcnt, const void *buffer,
+					 bool is_write)
 #else
-static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
-				   lbaint_t blkcnt, void *buffer)
+static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
+					 lbaint_t blknr, lbaint_t blkcnt,
+					 const void *buffer, bool is_write)
 #endif
 {
 	lbaint_t start, blks;

common/usb_storage.c +8 -5

 	do {
-		/* XXX need some comment here */
+		/*
+		 * If read/write fails retry for max retry count else
+		 * return with number of blocks written successfully.
+		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
 		if (blks > USB_MAX_XFER_BLK)

@@ -1162,8 +1176,8 @@ retry_it:
 			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");
+		if (usb_stor_operation(srb, ss, start, smallblks, is_write)) {
+			debug("%s ERROR\n", USB_STOR_OP_TYPE);
 			usb_request_sense(srb, ss);
 			if (retry--)
 				goto retry_it;

@@ -1176,9 +1190,9 @@ retry_it:
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
-	debug("usb_read: end startblk " LBAF
+	debug("usb_%s: end startblk " LBAF
 	      ", blccnt %x buffer %" PRIxPTR "\n",
-	      start, smallblks, buf_addr);
+	      USB_STOR_OP_TYPE, start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
 	if (blkcnt >= USB_MAX_XFER_BLK)

@@ -1186,90 +1200,27 @@ retry_it:
 	return blkcnt;
 }

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Monday, June 13, 2016 7:00 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 v5 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

Quoted 26 lines · Rajesh Bhagat

Performs code cleanup by making common function for usb_stor_read/ usb_stor_write. Currently only difference in these fucntions is call to usb_read_10/usb_write_10 scsi commands.

Signed-off-by Rajesh Bhagat <[email protected]>

---
Changes in v5:
- Converts USB_STOR_OPERATION_FUNC macro to a function
- Corrects the multi line comment accroding to coding style
- Renames variable to dev to remove code duplication

Changes in v4:
- Adds code to make common function for read/write

 common/usb_storage.c |  129
+++++++++++++++----------------------------------
 1 files changed, 40 insertions(+), 89 deletions(-)

common/usb_storage.c index renamed from common/usb_storage.c +0 -0

7e6e52d..f060637 100644

common/usb_storage.c +0 -0

@@ -1104,12 +1104,22 @@ static void usb_bin_fixup(struct

usb_device_descriptor descriptor, } #endif /* CONFIG_USB_BIN_FIXUP */

Quoted 1 line

+#define USB_STOR_OP_TYPE (is_write ? "write" : "read")

I just noticed this gem here. It's a macro which uses a variable, but the variable is not passed in as it's argument. Just inline this, it's not worth having it as a macro here.

common/usb_storage.c static void usb_bin_fixup(struct usb_device_descriptor descriptor, · line 1112

Quoted 4 lines

common/usb_storage.c +4 -0

+static int usb_stor_operation(ccb *srb, struct us_data *ss, unsigned long start,
+			      unsigned short blocks, bool is_write) {
+	return is_write ? usb_write_10(srb, ss, start, blocks) :
+		usb_read_10(srb, ss, start, blocks);

usb_read_10() and usb_write_10() look exactly the same for all but the command . You should do this unification at that level instead.

Quoted 21 lines

common/usb_storage.c +8 -4

+}
+
 #ifdef CONFIG_BLK
-static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
-				   lbaint_t blkcnt, void *buffer)
+static unsigned long usb_stor_read_write(struct udevice *dev, lbaint_t blknr,
+					 lbaint_t blkcnt, const void *buffer,
+					 bool is_write)
 #else
-static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
-				   lbaint_t blkcnt, void *buffer)
+static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
+					 lbaint_t blknr, lbaint_t blkcnt,
+					 const void *buffer, bool is_write)
 #endif
 {
 	lbaint_t start, blks;

@@ -1129,9 +1139,9 @@ static unsigned long usb_stor_read(struct

blk_desc *block_dev, lbaint_t blknr, #ifdef CONFIG_BLK

common/usb_storage.c +0 -0

 	block_dev = dev_get_uclass_platdata(dev);
 	udev = dev_get_parent_priv(dev_get_parent(dev));

common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1144

Quoted 5 lines

common/usb_storage.c +2 -2

-	debug("\nusb_read: udev %d\n", block_dev->devnum);
+	debug("\nusb_%s: udev %d\n", USB_STOR_OP_TYPE, block_dev->devnum);
 #else
-	debug("\nusb_read: udev %d\n", block_dev->devnum);
+	debug("\nusb_%s: udev %d\n", USB_STOR_OP_TYPE, block_dev->devnum);

You can just use "\n%s: ..." and use __func__ instead of USB_STOR_OP_TYPE here, that'd be less cryptic than "usb_read:" and it's used all over the place already anyway.

Quoted 7 lines

common/usb_storage.c +0 -0

 	udev = usb_dev_desc[block_dev->devnum].priv;
 	if (!udev) {
 		debug("%s: No device\n", __func__); @@ -1146,11 +1156,15 @@ static

unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,

common/usb_storage.c +0 -0

 	start = blknr;
 	blks = blkcnt;

common/usb_storage.c static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, · line 1161

Quoted 5 lines

common/usb_storage.c +3 -2

-	debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
-	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
+	debug("\nusb_%s: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
+	      PRIxPTR "\n", USB_STOR_OP_TYPE, block_dev->devnum, start, blks,
+	      buf_addr);

DTTO here.

Quoted 34 lines

common/usb_storage.c +8 -5

 	do {
-		/* XXX need some comment here */
+		/*
+		 * If read/write fails retry for max retry count else
+		 * return with number of blocks written successfully.
+		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
 		if (blks > USB_MAX_XFER_BLK)

@@ -1162,8 +1176,8 @@ retry_it:
 			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");
+		if (usb_stor_operation(srb, ss, start, smallblks, is_write)) {
+			debug("%s ERROR\n", USB_STOR_OP_TYPE);
 			usb_request_sense(srb, ss);
 			if (retry--)
 				goto retry_it;

@@ -1176,9 +1190,9 @@ retry_it:
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
-	debug("usb_read: end startblk " LBAF
+	debug("usb_%s: end startblk " LBAF
 	      ", blccnt %x buffer %" PRIxPTR "\n",
-	      start, smallblks, buf_addr);
+	      USB_STOR_OP_TYPE, start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
 	if (blkcnt >= USB_MAX_XFER_BLK)

common/usb_storage.c retry_it: · line 1201

Quoted 2 lines

common/usb_storage.c +0 -0

@@ -1186,90 +1200,27 @@ retry_it:
 	return blkcnt;
 }

[...]

--
Best regards,
Marek Vasut

From: Rajesh Bhagat <[email protected]>

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

Signed-off-by Rajesh Bhagat <[email protected]>

---
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 |   67 ++++++++++++++++++++++++++++++++++++++++++++++---
 include/usb.h        |    1 +
 2 files changed, 63 insertions(+), 5 deletions(-)

common/usb_storage.c +0 -0

index f060637..9b09412 100644
@@ -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.

common/usb_storage.c +1 -0

 #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)

common/usb_storage.c +11 -0

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

common/usb_storage.c +23 -5

+	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

@@ -1128,6 +1171,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;

@@ -1167,26 +1211,36 @@ 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("usb_%s: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",
+		      USB_STOR_OP_TYPE, 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_stor_operation(srb, ss, start, smallblks, is_write)) {
 			debug("%s ERROR\n", USB_STOR_OP_TYPE);
 			usb_request_sense(srb, ss);
-			if (retry--)
+			if (retry--) {
+				if (!dec_cur_xfer_blks(udev))
+					smallblks = udev->cur_xfer_blks;
+				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))
+			smallblks = udev->cur_xfer_blks;
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 

@@ -1195,7 +1249,7 @@ retry_it:
 	      USB_STOR_OP_TYPE, 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;
 }

@@ -1282,6 +1336,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

index 02a0ccd..b815816 100644
@@ -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 at denx.de]
Sent: Monday, June 13, 2016 7:07 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]>; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH v5 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks

Quoted 39 lines · Rajesh Bhagat

From: Rajesh Bhagat <[email protected]>

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

Signed-off-by Rajesh Bhagat <[email protected]>

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

Quoted 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/write

 common/usb_storage.c |   67

++++++++++++++++++++++++++++++++++++++++++++++---

common/usb_storage.c struct us_data { · line 106

Quoted 9 lines

 include/usb.h        |    1 +
 2 files changed, 63 insertions(+), 5 deletions(-)

common/usb_storage.c index renamed from common/usb_storage.c +0 -0

f060637..9b09412 100644

common/usb_storage.c +0 -0

@@ -106,11 +106,16 @@ 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
 

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 + 1) /

(USB_MIN_XFER_BLK + 1)))

Quoted 1 line

common/usb_storage.c +1 -0

+#define CALC_CUR_XFER_BLKS(pos)	((1 << (12 + pos)) - 1)

Parenthesis around variables are missing.

Quoted 6 lines

common/usb_storage.c +1 -0

 #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(".");

}

common/usb_storage.c static void usb_show_progress(void) · line 159

Quoted 10 lines

common/usb_storage.c +10 -0

+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  = pos ? CALC_CUR_XFER_BLKS(pos - 1) : 0;

If pos == 0 , the condition below will be true (size will be 2047), so this extra ternary is unnecessary.

Quoted 15 lines

common/usb_storage.c +15 -0

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

common/usb_storage.c static void usb_show_progress(void) · line 181

Quoted 6 lines

common/usb_storage.c +6 -0

+
+	pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
+	size = CALC_CUR_XFER_BLKS(pos + 1);
+
+	if (size > blks || size > USB_MAX_XFER_BLK)
+		return -EINVAL;

Why don't you clamp the size to min(blks, USB_MAX_XFER_BLK) instead ?

common/usb_storage.c static void usb_show_progress(void) · line 167

Quoted 5 lines

common/usb_storage.c +4 -0

+	udev->cur_xfer_blks = size;
+	return 0;
+}
+

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

Quoted 3 lines

* show info on storage devices; 'usb start/init' must be invoked earlier
* as we only retrieve structures populated during devices
initialization @@ -1128,6 +1171,7 @@ static unsigned long

usb_stor_read_write(struct blk_desc *block_dev,

common/usb_storage.c static unsigned long usb_stor_read_write(struct

Quoted 8 lines

common/usb_storage.c +1 -0

 	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;

@@ -1167,26 +1211,36 @@ static unsigned long usb_stor_read_write(struct

blk_desc *block_dev,

common/usb_storage.c int usb_storage_probe(struct usb_device *dev, unsigned

Quoted 50 lines

common/usb_storage.c +15 -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_%s: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",
+		      USB_STOR_OP_TYPE, 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_stor_operation(srb, ss, start, smallblks, is_write)) {
 			debug("%s ERROR\n", USB_STOR_OP_TYPE);
 			usb_request_sense(srb, ss);
-			if (retry--)
+			if (retry--) {
+				if (!dec_cur_xfer_blks(udev))
+					smallblks = udev->cur_xfer_blks;
+				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))
+			smallblks = udev->cur_xfer_blks;
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 

@@ -1195,7 +1249,7 @@ retry_it:
 	      USB_STOR_OP_TYPE, 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;
 }

@@ -1282,6 +1336,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned

int ifnum,

Quoted 20 lines

common/usb_storage.c +3 -0

 		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 index 02a0ccd..b815816 renamed from include/usb.h +0 -0

100644

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 */
 };
 

include/usb.h struct usb_device { · line 159

Quoted 2 lines

include/usb.h +0 -0

 struct int_queue;

--
Best regards,
Marek Vasut

Quoted 76 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Monday, June 13, 2016 7:07 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]>; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH v5 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks

Quoted 39 lines · Rajesh Bhagat

From: Rajesh Bhagat <[email protected]>

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

Signed-off-by Rajesh Bhagat <[email protected]>

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

Quoted 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/write

 common/usb_storage.c |   67

++++++++++++++++++++++++++++++++++++++++++++++---

common/usb_storage.c struct us_data { · line 106

Quoted 9 lines

 include/usb.h        |    1 +
 2 files changed, 63 insertions(+), 5 deletions(-)

common/usb_storage.c index renamed from common/usb_storage.c +0 -0

f060637..9b09412 100644

common/usb_storage.c +0 -0

@@ -106,11 +106,16 @@ 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 3 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 + 1) /

(USB_MIN_XFER_BLK + 1)))

Quoted 1 line

common/usb_storage.c +1 -0

+#define CALC_CUR_XFER_BLKS(pos)	((1 << (12 + pos)) - 1)

Hello Marek,

Quoted 3 lines

Parenthesis around variables are missing.

Will take care in v6.

Quoted 6 lines

Quoted 6 lines

common/usb_storage.c +1 -0

 #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(".");

}

Quoted 14 lines

common/usb_storage.c static void usb_show_progress(void) · line 159

Quoted 10 lines

common/usb_storage.c +10 -0

+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  = pos ? CALC_CUR_XFER_BLKS(pos - 1) : 0;

If pos == 0 , the condition below will be true (size will be 2047), so this extra ternary is unnecessary.

Will take care in v6. Will remove the extra ternary operator used, as 2047 will follow the below check (size < USB_MIN_XFER_BLK) and code will work fine.

Quoted 15 lines

Quoted 15 lines

common/usb_storage.c +15 -0

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

Quoted 9 lines

common/usb_storage.c static void usb_show_progress(void) · line 181

Quoted 6 lines

common/usb_storage.c +6 -0

+
+	pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
+	size = CALC_CUR_XFER_BLKS(pos + 1);
+
+	if (size > blks || size > USB_MAX_XFER_BLK)
+		return -EINVAL;

Why don't you clamp the size to min(blks, USB_MAX_XFER_BLK) instead ?

Do you mean below statement?

if (size > min(blks, USB_MAX_XFER_BLK))

	return -EINVAL;

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, June 14, 2016 5:41 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]>; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH v5 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks

Quoted 111 lines · Rajesh Bhagat

Quoted 78 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Monday, June 13, 2016 7:07 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]>; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH v5 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks

Quoted 39 lines · Rajesh Bhagat

From: Rajesh Bhagat <[email protected]>

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

Signed-off-by Rajesh Bhagat <[email protected]>

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

Quoted 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/write

 common/usb_storage.c |   67

++++++++++++++++++++++++++++++++++++++++++++++---

Quoted 10 lines

 include/usb.h        |    1 +
 2 files changed, 63 insertions(+), 5 deletions(-)

common/usb_storage.c index renamed from common/usb_storage.c +0 -0

f060637..9b09412 100644

common/usb_storage.c +0 -0

@@ -106,11 +106,16 @@ 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 3 lines

Quoted 1 line

+#define GET_CUR_XFER_BLKS(blks) (LOG2((blks + 1) /

(USB_MIN_XFER_BLK + 1)))

Quoted 1 line

+#define CALC_CUR_XFER_BLKS(pos) ((1 << (12 + pos)) - 1)

Hello Marek,

Quoted 3 lines

Parenthesis around variables are missing.

Will take care in v6.

Quoted 6 lines

Quoted 6 lines

#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(".");
}

Quoted 14 lines

common/usb_storage.c static void usb_show_progress(void) · line 159

Quoted 10 lines

common/usb_storage.c +10 -0

+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  = pos ? CALC_CUR_XFER_BLKS(pos - 1) : 0;

If pos == 0 , the condition below will be true (size will be 2047), so this extra ternary is unnecessary.

Quoted 2 lines

Will take care in v6. Will remove the extra ternary operator used, as 2047 will follow the below check (size < USB_MIN_XFER_BLK) and code will work

fine.

Quoted 26 lines

Quoted 25 lines

Quoted 12 lines

common/usb_storage.c +11 -0

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

common/usb_storage.c static void usb_show_progress(void) · line 181

Quoted 10 lines

common/usb_storage.c +10 -0

+	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 > blks || size > USB_MAX_XFER_BLK)
+		return -EINVAL;

Why don't you clamp the size to min(blks, USB_MAX_XFER_BLK) instead ?

Quoted 4 lines

Do you mean below statement?

if (size > min(blks, USB_MAX_XFER_BLK))

	return -EINVAL;

Yes, if that makes sense.

--
Best regards,
Marek Vasut