Re: [U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Thursday, June 09, 2016 8:21 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 v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

On 06/09/2016 08:32 AM, Rajesh Bhagat wrote:

...

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 rajesh.bhagat@nxp.com

Changes in v4:

• Adds code to make common function for read/write

common/usb_storage.c | 132 ++++++++++++++++++--------------------------------- 1 file changed, 46 insertions(+), 86 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..5efdc90 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1104,12 +1104,20 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor, } #endif /* CONFIG_USB_BIN_FIXUP */

+#define USB_STOR_OPERATION is_write ? "write" : "read" +#define USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks) \

• 		is_write ?				 \
  

• 		usb_write_10(srb, ss, start, smallblks) :\
  

• 		usb_read_10(srb, ss, start, smallblks)
  

Turn this into a function. The macro is obviously missing even the most basic parenthesis, but that's beyond the point.

Ok, I will change it into a function in v5.

...

#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 +1137,9 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, #ifdef CONFIG_BLK block_dev = dev_get_uclass_platdata(dev); udev = dev_get_parent_priv(dev_get_parent(dev));

• debug("\nusb_read: udev %d\n", block_dev->devnum);

• debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);

#else

• debug("\nusb_read: udev %d\n", block_dev->devnum);

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

unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr, start = blknr; blks = blkcnt;

• 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_OPERATION, block_dev->devnum, start, blks,
  

•      buf_addr);
  
  

  do {

• /* XXX need some comment here */
  

Multi-line comment goes this way: /*

• foo
• bar

*/ It's in the coding style document too, please read it.

My apologies, I took it from usb_stor_write function and din't noticed the coding style issue in the existing code. Will correct it in v5.

...

• /* 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 +1173,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_FUNC(srb, ss, start, smallblks)) {
  

• 	debug("%s ERROR\n", USB_STOR_OPERATION);
  usb_request_sense(srb, ss);
  if (retry--)
  	goto retry_it;
  

@@ -1176,9 +1187,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_OPERATION, start, smallblks, buf_addr);
  
  

  usb_disable_asynch(0); /* asynch transfer allowed */ if (blkcnt >= USB_MAX_XFER_BLK)

@@ -1186,6 +1197,24 @@ retry_it: return blkcnt; }

+#ifdef CONFIG_BLK +static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,

• 		   lbaint_t blkcnt, void *buffer) #else static unsigned long
  

+usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,

• 		   lbaint_t blkcnt, void *buffer) #endif {
  

• return usb_stor_read_write(

+#ifdef CONFIG_BLK

Rename the variable to be always dev and drop this ifdef ... please, review the patches before submitting to remove such stupid omissions from them, it's wasting both my time and yours.

I understand your concern, but my intent was to keep the exiting prototypes unchanged, may be difference of thought process. Will take care in v5.

Best Regards, Rajesh Bhagat

...

• 		   dev,
  

+#else

• 		   block_dev,
  

+#endif

• 		   blknr, blkcnt, buffer, false); }
  

#ifdef CONFIG_BLK static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, const void *buffer) @@ -1194,82

+1223,13 @@

...

static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, lbaint_t blkcnt, const void *buffer) #endif {

• lbaint_t start, blks;
• uintptr_t buf_addr;
• unsigned short smallblks;
• struct usb_device *udev;
• struct us_data *ss;
• int retry;
• ccb *srb = &usb_ccb;

• return usb_stor_read_write(

#ifdef CONFIG_BLK

• struct blk_desc *block_dev;

-#endif

• if (blkcnt == 0)

• return 0;
  

• /* Setup device */

-#ifdef CONFIG_BLK

• block_dev = dev_get_uclass_platdata(dev);
• udev = dev_get_parent_priv(dev_get_parent(dev));
• debug("\nusb_read: udev %d\n", block_dev->devnum);

• 		   dev,
  

#else

• debug("\nusb_read: udev %d\n", block_dev->devnum);
• udev = usb_dev_desc[block_dev->devnum].priv;
• if (!udev) {

• debug("%s: No device\n", __func__);
  

• return 0;
  

• }

• 		   block_dev,
  

#endif

• ss = (struct us_data *)udev->privptr;
• usb_disable_asynch(1); /* asynch transfer not allowed */
• srb->lun = block_dev->lun;
• buf_addr = (uintptr_t)buffer;
• start = blknr;
• blks = blkcnt;
• debug("\nusb_write: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"

•      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
  

• do {

• /* If 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)
  

• 	smallblks = USB_MAX_XFER_BLK;
  

• else
  

• 	smallblks = (unsigned short) blks;
  

-retry_it:

• if (smallblks == USB_MAX_XFER_BLK)
  

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

• 		goto retry_it;
  

• 	blkcnt -= blks;
  

• 	break;
  

• }
  

• start += smallblks;
  

• blks -= smallblks;
  

• buf_addr += srb->datalen;
  

• } while (blks != 0);
• ss->flags &= ~USB_READY;
• debug("usb_write: end startblk " LBAF ", blccnt %x buffer %"

•      PRIxPTR "\n", start, smallblks, buf_addr);
  

• usb_disable_asynch(0); /* asynch transfer allowed */
• if (blkcnt >= USB_MAX_XFER_BLK)

• debug("\n");
  

• return blkcnt;

• 		   blknr, blkcnt, buffer, true);
  

}

/* Probe to see if a new device is actually a Storage device */

-- Best regards, Marek Vasut