diff mbox

[U-Boot,06/13] fastboot: Move fastboot response functions to fastboot core

Message ID 1441032373-16992-7-git-send-email-maxime.ripard@free-electrons.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Maxime Ripard Aug. 31, 2015, 2:46 p.m. UTC 
The functions and a few define to generate a fastboot message to be sent
back to the host were so far duplicated among the users.

Move them all to a common place.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 common/aboot.c                  | 14 ++++++--------
 common/fb_mmc.c                 | 43 ++++++++++++++---------------------------
 drivers/usb/gadget/f_fastboot.c | 27 ++++++++++++++++++--------
 include/aboot.h                 |  3 ---
 include/fastboot.h              | 22 +++++++++++++++++++++
 5 files changed, 62 insertions(+), 47 deletions(-)
 create mode 100644 include/fastboot.h

Comments

Tom Rini Sept. 4, 2015, 5:20 p.m. UTC | #1 
On Mon, Aug 31, 2015 at 04:46:06PM +0200, Maxime Ripard wrote:

> The functions and a few define to generate a fastboot message to be sent
> back to the host were so far duplicated among the users.
> 
> Move them all to a common place.
[snip]
> diff --git a/common/aboot.c b/common/aboot.c
> index 18ff30ee6d11..37ad50efc50a 100644
> --- a/common/aboot.c
> +++ b/common/aboot.c
> @@ -277,7 +277,7 @@ void store_sparse_image(sparse_storage_t *storage,
>  
>  	sparse_header = sparse_parse_header(&data);
>  	if (!sparse_header) {
> -		fastboot_fail("sparse header issue\n");
> +		printf("sparse header issue\n");
>  		return;
>  	}
>  
> @@ -288,7 +288,6 @@ void store_sparse_image(sparse_storage_t *storage,
>  	if (sparse_header->blk_sz % storage->block_sz) {
>  		printf("%s: Sparse image block size issue [%u]\n",
>  		       __func__, sparse_header->blk_sz);
> -		fastboot_fail("sparse image block size issue");
>  		return;
>  	}
>  
> @@ -299,7 +298,7 @@ void store_sparse_image(sparse_storage_t *storage,
>  	for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) {
>  		chunk_header = sparse_parse_chunk(sparse_header, &data);
>  		if (!chunk_header) {
> -			fastboot_fail("Unknown chunk type");
> +			printf("Unknown chunk type");
>  			return;
>  		}
>  
> @@ -314,7 +313,6 @@ void store_sparse_image(sparse_storage_t *storage,
>  		if (blk + blkcnt > storage->start + storage->size) {
>  			printf("%s: Request would exceed partition size!\n",
>  			       __func__);
> -			fastboot_fail("Request would exceed partition size!");
>  			return;
>  		}
>  
> @@ -331,7 +329,6 @@ void store_sparse_image(sparse_storage_t *storage,
>  			if (buffer_blks != buffer_blk_cnt) {
>  				printf("%s: Write %d failed %d\n",
>  				       __func__, i, buffer_blks);
> -				fastboot_fail("flash write failure");
>  				return;
>  			}
>  
> @@ -348,9 +345,10 @@ void store_sparse_image(sparse_storage_t *storage,
>  	printf("........ wrote %u bytes to '%s'\n", bytes_written,
>  	       storage->name);
>  
> -	if (total_blocks != sparse_header->total_blks)
> -		fastboot_fail("sparse image write failure");
> +	if (total_blocks != sparse_header->total_blks) {
> +		printf("sparse image write failure");
> +		return;
> +	}
>  
> -	fastboot_okay("");
>  	return;
>  }

Why in the case of this image do we not need to do fastboot_fail/okay
now?  It's not obvious from the rest of the changes to me, thanks!
Maxime Ripard Sept. 6, 2015, 4:11 p.m. UTC | #2 
On Fri, Sep 04, 2015 at 01:20:44PM -0400, Tom Rini wrote:
> On Mon, Aug 31, 2015 at 04:46:06PM +0200, Maxime Ripard wrote:
> 
> > The functions and a few define to generate a fastboot message to be sent
> > back to the host were so far duplicated among the users.
> > 
> > Move them all to a common place.
> [snip]
> > diff --git a/common/aboot.c b/common/aboot.c
> > index 18ff30ee6d11..37ad50efc50a 100644
> > --- a/common/aboot.c
> > +++ b/common/aboot.c
> > @@ -277,7 +277,7 @@ void store_sparse_image(sparse_storage_t *storage,
> >  
> >  	sparse_header = sparse_parse_header(&data);
> >  	if (!sparse_header) {
> > -		fastboot_fail("sparse header issue\n");
> > +		printf("sparse header issue\n");
> >  		return;
> >  	}
> >  
> > @@ -288,7 +288,6 @@ void store_sparse_image(sparse_storage_t *storage,
> >  	if (sparse_header->blk_sz % storage->block_sz) {
> >  		printf("%s: Sparse image block size issue [%u]\n",
> >  		       __func__, sparse_header->blk_sz);
> > -		fastboot_fail("sparse image block size issue");
> >  		return;
> >  	}
> >  
> > @@ -299,7 +298,7 @@ void store_sparse_image(sparse_storage_t *storage,
> >  	for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) {
> >  		chunk_header = sparse_parse_chunk(sparse_header, &data);
> >  		if (!chunk_header) {
> > -			fastboot_fail("Unknown chunk type");
> > +			printf("Unknown chunk type");
> >  			return;
> >  		}
> >  
> > @@ -314,7 +313,6 @@ void store_sparse_image(sparse_storage_t *storage,
> >  		if (blk + blkcnt > storage->start + storage->size) {
> >  			printf("%s: Request would exceed partition size!\n",
> >  			       __func__);
> > -			fastboot_fail("Request would exceed partition size!");
> >  			return;
> >  		}
> >  
> > @@ -331,7 +329,6 @@ void store_sparse_image(sparse_storage_t *storage,
> >  			if (buffer_blks != buffer_blk_cnt) {
> >  				printf("%s: Write %d failed %d\n",
> >  				       __func__, i, buffer_blks);
> > -				fastboot_fail("flash write failure");
> >  				return;
> >  			}
> >  
> > @@ -348,9 +345,10 @@ void store_sparse_image(sparse_storage_t *storage,
> >  	printf("........ wrote %u bytes to '%s'\n", bytes_written,
> >  	       storage->name);
> >  
> > -	if (total_blocks != sparse_header->total_blks)
> > -		fastboot_fail("sparse image write failure");
> > +	if (total_blocks != sparse_header->total_blks) {
> > +		printf("sparse image write failure");
> > +		return;
> > +	}
> >  
> > -	fastboot_okay("");
> >  	return;
> >  }
> 
> Why in the case of this image do we not need to do fastboot_fail/okay
> now?  It's not obvious from the rest of the changes to me, thanks!

I took that shortcut while refactoring and forgot to fix it...

The issue is that we don't have access to the response buffer in those
functions. We could pass it as an argument, but that would:

  - Prevent the calling function (that "owns" the buffer pointer) to
    use fastboot_fail / fastboot_okay, which feels a bit wrong.

  - On a more theorical point, the sparse image format should be
    decoupled from the fastboot protocol, hence not really rely of
    these functions.

What we can do though, is simply return an error code, and the storage
layers could use fastboot_okay / fastboot_fail themselves. Does that
sound good?

Maxime
Tom Rini Sept. 6, 2015, 7:43 p.m. UTC | #3 
On Sun, Sep 06, 2015 at 06:11:37PM +0200, Maxime Ripard wrote:
> On Fri, Sep 04, 2015 at 01:20:44PM -0400, Tom Rini wrote:
> > On Mon, Aug 31, 2015 at 04:46:06PM +0200, Maxime Ripard wrote:
> > 
> > > The functions and a few define to generate a fastboot message to be sent
> > > back to the host were so far duplicated among the users.
> > > 
> > > Move them all to a common place.
> > [snip]
> > > diff --git a/common/aboot.c b/common/aboot.c
> > > index 18ff30ee6d11..37ad50efc50a 100644
> > > --- a/common/aboot.c
> > > +++ b/common/aboot.c
> > > @@ -277,7 +277,7 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  
> > >  	sparse_header = sparse_parse_header(&data);
> > >  	if (!sparse_header) {
> > > -		fastboot_fail("sparse header issue\n");
> > > +		printf("sparse header issue\n");
> > >  		return;
> > >  	}
> > >  
> > > @@ -288,7 +288,6 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  	if (sparse_header->blk_sz % storage->block_sz) {
> > >  		printf("%s: Sparse image block size issue [%u]\n",
> > >  		       __func__, sparse_header->blk_sz);
> > > -		fastboot_fail("sparse image block size issue");
> > >  		return;
> > >  	}
> > >  
> > > @@ -299,7 +298,7 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  	for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) {
> > >  		chunk_header = sparse_parse_chunk(sparse_header, &data);
> > >  		if (!chunk_header) {
> > > -			fastboot_fail("Unknown chunk type");
> > > +			printf("Unknown chunk type");
> > >  			return;
> > >  		}
> > >  
> > > @@ -314,7 +313,6 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  		if (blk + blkcnt > storage->start + storage->size) {
> > >  			printf("%s: Request would exceed partition size!\n",
> > >  			       __func__);
> > > -			fastboot_fail("Request would exceed partition size!");
> > >  			return;
> > >  		}
> > >  
> > > @@ -331,7 +329,6 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  			if (buffer_blks != buffer_blk_cnt) {
> > >  				printf("%s: Write %d failed %d\n",
> > >  				       __func__, i, buffer_blks);
> > > -				fastboot_fail("flash write failure");
> > >  				return;
> > >  			}
> > >  
> > > @@ -348,9 +345,10 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  	printf("........ wrote %u bytes to '%s'\n", bytes_written,
> > >  	       storage->name);
> > >  
> > > -	if (total_blocks != sparse_header->total_blks)
> > > -		fastboot_fail("sparse image write failure");
> > > +	if (total_blocks != sparse_header->total_blks) {
> > > +		printf("sparse image write failure");
> > > +		return;
> > > +	}
> > >  
> > > -	fastboot_okay("");
> > >  	return;
> > >  }
> > 
> > Why in the case of this image do we not need to do fastboot_fail/okay
> > now?  It's not obvious from the rest of the changes to me, thanks!
> 
> I took that shortcut while refactoring and forgot to fix it...
> 
> The issue is that we don't have access to the response buffer in those
> functions. We could pass it as an argument, but that would:
> 
>   - Prevent the calling function (that "owns" the buffer pointer) to
>     use fastboot_fail / fastboot_okay, which feels a bit wrong.
> 
>   - On a more theorical point, the sparse image format should be
>     decoupled from the fastboot protocol, hence not really rely of
>     these functions.
> 
> What we can do though, is simply return an error code, and the storage
> layers could use fastboot_okay / fastboot_fail themselves. Does that
> sound good?

Error code and letting the higher level deal with it sounds good to me!
diff mbox

Patch

diff --git a/common/aboot.c b/common/aboot.c
index 18ff30ee6d11..37ad50efc50a 100644
--- a/common/aboot.c
+++ b/common/aboot.c
@@ -277,7 +277,7 @@  void store_sparse_image(sparse_storage_t *storage,
 
 	sparse_header = sparse_parse_header(&data);
 	if (!sparse_header) {
-		fastboot_fail("sparse header issue\n");
+		printf("sparse header issue\n");
 		return;
 	}
 
@@ -288,7 +288,6 @@  void store_sparse_image(sparse_storage_t *storage,
 	if (sparse_header->blk_sz % storage->block_sz) {
 		printf("%s: Sparse image block size issue [%u]\n",
 		       __func__, sparse_header->blk_sz);
-		fastboot_fail("sparse image block size issue");
 		return;
 	}
 
@@ -299,7 +298,7 @@  void store_sparse_image(sparse_storage_t *storage,
 	for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) {
 		chunk_header = sparse_parse_chunk(sparse_header, &data);
 		if (!chunk_header) {
-			fastboot_fail("Unknown chunk type");
+			printf("Unknown chunk type");
 			return;
 		}
 
@@ -314,7 +313,6 @@  void store_sparse_image(sparse_storage_t *storage,
 		if (blk + blkcnt > storage->start + storage->size) {
 			printf("%s: Request would exceed partition size!\n",
 			       __func__);
-			fastboot_fail("Request would exceed partition size!");
 			return;
 		}
 
@@ -331,7 +329,6 @@  void store_sparse_image(sparse_storage_t *storage,
 			if (buffer_blks != buffer_blk_cnt) {
 				printf("%s: Write %d failed %d\n",
 				       __func__, i, buffer_blks);
-				fastboot_fail("flash write failure");
 				return;
 			}
 
@@ -348,9 +345,10 @@  void store_sparse_image(sparse_storage_t *storage,
 	printf("........ wrote %u bytes to '%s'\n", bytes_written,
 	       storage->name);
 
-	if (total_blocks != sparse_header->total_blks)
-		fastboot_fail("sparse image write failure");
+	if (total_blocks != sparse_header->total_blks) {
+		printf("sparse image write failure");
+		return;
+	}
 
-	fastboot_okay("");
 	return;
 }
diff --git a/common/fb_mmc.c b/common/fb_mmc.c
index a44627c00060..0d6bcbf1f78b 100644
--- a/common/fb_mmc.c
+++ b/common/fb_mmc.c
@@ -6,6 +6,7 @@ 
 
 #include <config.h>
 #include <common.h>
+#include <fastboot.h>
 #include <fb_mmc.h>
 #include <part.h>
 #include <aboot.h>
@@ -16,23 +17,8 @@ 
 #define CONFIG_FASTBOOT_GPT_NAME GPT_ENTRY_NAME
 #endif
 
-/* The 64 defined bytes plus the '\0' */
-#define RESPONSE_LEN	(64 + 1)
-
 static char *response_str;
 
-void fastboot_fail(const char *s)
-{
-	strncpy(response_str, "FAIL\0", 5);
-	strncat(response_str, s, RESPONSE_LEN - 4 - 1);
-}
-
-void fastboot_okay(const char *s)
-{
-	strncpy(response_str, "OKAY\0", 5);
-	strncat(response_str, s, RESPONSE_LEN - 4 - 1);
-}
-
 struct fb_mmc_sparse {
 	block_dev_desc_t	*dev_desc;
 };
@@ -85,7 +71,7 @@  static void write_raw_image(block_dev_desc_t *dev_desc, disk_partition_t *info,
 
 	if (blkcnt > info->size) {
 		error("too large for partition: '%s'\n", part_name);
-		fastboot_fail("too large for partition");
+		fastboot_fail(response_str, "too large for partition");
 		return;
 	}
 
@@ -95,13 +81,13 @@  static void write_raw_image(block_dev_desc_t *dev_desc, disk_partition_t *info,
 				     buffer);
 	if (blks != blkcnt) {
 		error("failed writing to device %d\n", dev_desc->dev);
-		fastboot_fail("failed writing to device");
+		fastboot_fail(response_str, "failed writing to device");
 		return;
 	}
 
 	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
 	       part_name);
-	fastboot_okay("");
+	fastboot_okay(response_str, "");
 }
 
 void fb_mmc_flash_write(const char *cmd, void *download_buffer,
@@ -116,7 +102,7 @@  void fb_mmc_flash_write(const char *cmd, void *download_buffer,
 	dev_desc = get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
 	if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) {
 		error("invalid mmc device\n");
-		fastboot_fail("invalid mmc device");
+		fastboot_fail(response_str, "invalid mmc device");
 		return;
 	}
 
@@ -126,20 +112,21 @@  void fb_mmc_flash_write(const char *cmd, void *download_buffer,
 		if (is_valid_gpt_buf(dev_desc, download_buffer)) {
 			printf("%s: invalid GPT - refusing to write to flash\n",
 			       __func__);
-			fastboot_fail("invalid GPT partition");
+			fastboot_fail(response_str, "invalid GPT partition");
 			return;
 		}
 		if (write_mbr_and_gpt_partitions(dev_desc, download_buffer)) {
 			printf("%s: writing GPT partitions failed\n", __func__);
-			fastboot_fail("writing GPT partitions failed");
+			fastboot_fail(response_str,
+				      "writing GPT partitions failed");
 			return;
 		}
 		printf("........ success\n");
-		fastboot_okay("");
+		fastboot_okay(response_str, "");
 		return;
 	} else if (get_partition_info_efi_by_name_or_alias(dev_desc, cmd, &info)) {
 		error("cannot find partition: '%s'\n", cmd);
-		fastboot_fail("cannot find partition");
+		fastboot_fail(response_str, "cannot find partition");
 		return;
 	}
 
@@ -175,7 +162,7 @@  void fb_mmc_erase(const char *cmd, char *response)
 
 	if (mmc == NULL) {
 		error("invalid mmc device");
-		fastboot_fail("invalid mmc device");
+		fastboot_fail(response_str, "invalid mmc device");
 		return;
 	}
 
@@ -185,14 +172,14 @@  void fb_mmc_erase(const char *cmd, char *response)
 	dev_desc = get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
 	if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) {
 		error("invalid mmc device");
-		fastboot_fail("invalid mmc device");
+		fastboot_fail(response_str, "invalid mmc device");
 		return;
 	}
 
 	ret = get_partition_info_efi_by_name_or_alias(dev_desc, cmd, &info);
 	if (ret) {
 		error("cannot find partition: '%s'", cmd);
-		fastboot_fail("cannot find partition");
+		fastboot_fail(response_str, "cannot find partition");
 		return;
 	}
 
@@ -211,11 +198,11 @@  void fb_mmc_erase(const char *cmd, char *response)
 	blks = dev_desc->block_erase(dev_desc->dev, blks_start, blks_size);
 	if (blks != blks_size) {
 		error("failed erasing from device %d", dev_desc->dev);
-		fastboot_fail("failed erasing from device");
+		fastboot_fail(response_str, "failed erasing from device");
 		return;
 	}
 
 	printf("........ erased " LBAFU " bytes from '%s'\n",
 	       blks_size * info.blksz, cmd);
-	fastboot_okay("");
+	fastboot_okay(response_str, "");
 }
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index ca01a018b5d1..5703decfd360 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -13,6 +13,7 @@ 
 #include <config.h>
 #include <common.h>
 #include <errno.h>
+#include <fastboot.h>
 #include <malloc.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
@@ -34,9 +35,6 @@ 
 #define RX_ENDPOINT_MAXIMUM_PACKET_SIZE_1_1  (0x0040)
 #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
 
-/* The 64 defined bytes plus \0 */
-#define RESPONSE_LEN	(64 + 1)
-
 #define EP_BUFFER_SIZE			4096
 
 struct f_fastboot {
@@ -125,6 +123,19 @@  static struct usb_gadget_strings *fastboot_strings[] = {
 static void rx_handler_command(struct usb_ep *ep, struct usb_request *req);
 static int strcmp_l1(const char *s1, const char *s2);
 
+
+void fastboot_fail(char *response, const char *reason)
+{
+	strncpy(response, "FAIL\0", 5);
+	strncat(response, reason, FASTBOOT_RESPONSE_LEN - 4 - 1);
+}
+
+void fastboot_okay(char *response, const char *reason)
+{
+	strncpy(response, "OKAY\0", 5);
+	strncat(response, reason, FASTBOOT_RESPONSE_LEN - 4 - 1);
+}
+
 static void fastboot_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	int status = req->status;
@@ -358,7 +369,7 @@  static int strcmp_l1(const char *s1, const char *s2)
 static void cb_getvar(struct usb_ep *ep, struct usb_request *req)
 {
 	char *cmd = req->buf;
-	char response[RESPONSE_LEN];
+	char response[FASTBOOT_RESPONSE_LEN];
 	const char *s;
 	size_t chars_left;
 
@@ -415,7 +426,7 @@  static unsigned int rx_bytes_expected(unsigned int maxpacket)
 #define BYTES_PER_DOT	0x20000
 static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
 {
-	char response[RESPONSE_LEN];
+	char response[FASTBOOT_RESPONSE_LEN];
 	unsigned int transfer_size = download_size - download_bytes;
 	const unsigned char *buffer = req->buf;
 	unsigned int buffer_size = req->actual;
@@ -472,7 +483,7 @@  static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
 static void cb_download(struct usb_ep *ep, struct usb_request *req)
 {
 	char *cmd = req->buf;
-	char response[RESPONSE_LEN];
+	char response[FASTBOOT_RESPONSE_LEN];
 	unsigned int max;
 
 	strsep(&cmd, ":");
@@ -533,7 +544,7 @@  static void cb_continue(struct usb_ep *ep, struct usb_request *req)
 static void cb_flash(struct usb_ep *ep, struct usb_request *req)
 {
 	char *cmd = req->buf;
-	char response[RESPONSE_LEN];
+	char response[FASTBOOT_RESPONSE_LEN];
 
 	strsep(&cmd, ":");
 	if (!cmd) {
@@ -577,7 +588,7 @@  static void cb_oem(struct usb_ep *ep, struct usb_request *req)
 static void cb_erase(struct usb_ep *ep, struct usb_request *req)
 {
 	char *cmd = req->buf;
-	char response[RESPONSE_LEN];
+	char response[FASTBOOT_RESPONSE_LEN];
 
 	strsep(&cmd, ":");
 	if (!cmd) {
diff --git a/include/aboot.h b/include/aboot.h
index 162d81da6d9d..55da8336f20d 100644
--- a/include/aboot.h
+++ b/include/aboot.h
@@ -20,9 +20,6 @@  typedef struct sparse_storage {
 				 char *data);
 } sparse_storage_t;
 
-void fastboot_fail(const char *s);
-void fastboot_okay(const char *s);
-
 static inline int is_sparse_image(void *buf)
 {
 	sparse_header_t *s_header = (sparse_header_t *)buf;
diff --git a/include/fastboot.h b/include/fastboot.h
new file mode 100644
index 000000000000..db826d20bfef
--- /dev/null
+++ b/include/fastboot.h
@@ -0,0 +1,22 @@ 
+/*
+ * (C) Copyright 2008 - 2009
+ * Windriver, <www.windriver.com>
+ * Tom Rix <Tom.Rix@windriver.com>
+ *
+ * Copyright 2011 Sebastian Andrzej Siewior <bigeasy@linutronix.de>
+ *
+ * Copyright 2014 Linaro, Ltd.
+ * Rob Herring <robh@kernel.org>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+#ifndef _FASTBOOT_H_
+#define _FASTBOOT_H_
+
+/* The 64 defined bytes plus \0 */
+#define FASTBOOT_RESPONSE_LEN	(64 + 1)
+
+void fastboot_fail(char *response, const char *reason);
+void fastboot_okay(char *response, const char *reason);
+
+#endif /* _FASTBOOT_H_ */