Implement "fastboot flash" for eMMC
v3 cover Implement "fastboot flash" for eMMC
This series implements the "fastboot flash" command for eMMC devices. It supports both raw and sparse images.
NOTES:
- the support for the "fastboot flash" command is enabled with CONFIG_FASTBOOT_FLASH
- the support for eMMC is enabled with CONFIG_FASTBOOT_FLASH_MMC_DEV
- (future) the support for NAND would be enabled with CONFIG_FASTBOOT_FLASH_NAND(???)
This has been tested on ARMv7.
Changes in v3:
- remove most references to 'mmc',
which leaves only one mmc specific function: mmc_get_dev()
Changes in v2:
- split large function into three
- improved handling of response messages
- additional partition size checking when writing sparse image
- update README.android-fastboot file
- new in v2
Steve Rae (4):
usb/gadget: fastboot: add sparse image definitions usb/gadget: fastboot: add eMMC support for flash command usb/gadget: fastboot: add support for flash command usb/gadget: fastboot: minor cleanup
README | 10 +++ common/Makefile | 5 ++ common/fb_mmc.c | 190 ++++++++++++++++++++++++++++++++++++++++ doc/README.android-fastboot | 5 +- drivers/usb/gadget/f_fastboot.c | 41 ++++++++- include/fb_mmc.h | 8 ++ include/sparse_format.h | 58 ++++++++++++ 7 files changed, 312 insertions(+), 5 deletions(-) create mode 100644 common/fb_mmc.c create mode 100644 include/fb_mmc.h create mode 100644 include/sparse_format.h
Signature · 1 line
1.8.5
PATCH v3 1/4 usb/gadget: fastboot: add sparse image definitions
- to prepare for the support of fastboot sparse images
---
This file is ASIS from:
https://raw.githubusercontent.com/AOSB/android_system_core/master/libsparse/sparse_format.h
(commit 28fa5bc347390480fe190294c6c385b6a9f0d68b)
except for the __UBOOT__ conditional include.
Changes in v3: None
Changes in v2: None
include/sparse_format.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 include/sparse_format.h
include/sparse_format.h new file +58 -0
@@ -0,0 +1,58 @@ +/* +* Copyright (C) 2010 The Android Open Source Project +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +#ifndef _LIBSPARSE_SPARSE_FORMAT_H_ +#define _LIBSPARSE_SPARSE_FORMAT_H_ +#define __UBOOT__ +#ifndef __UBOOT__ +#include "sparse_defs.h" +#endif + +typedef struct sparse_header { +__le32 magic;/* 0xed26ff3a */ +__le16 major_version;/* (0x1) - reject images with higher major versions */ +__le16 minor_version;/* (0x0) - allow images with higer minor versions */ +__le16 file_hdr_sz;/* 28 bytes for first revision of the file format */ +__le16 chunk_hdr_sz;/* 12 bytes for first revision of the file format */ +__le32 blk_sz;/* block size in bytes, must be a multiple of 4 (4096) */ +__le32 total_blks;/* total blocks in the non-sparse output image */ +__le32 total_chunks;/* total chunks in the sparse input image */ +__le32 image_checksum;/* CRC32 checksum of the original data, counting "don't care" */ +/* as 0. Standard 802.3 polynomial, use a Public Domain */ +/* table implementation */ +}sparse_header_t ; + +#define SPARSE_HEADER_MAGIC 0xed26ff3a + +#define CHUNK_TYPE_RAW 0xCAC1 +#define CHUNK_TYPE_FILL 0xCAC2 +#define CHUNK_TYPE_DONT_CARE 0xCAC3 +#define CHUNK_TYPE_CRC32 0xCAC4 + +typedef struct chunk_header { +__le16 chunk_type;/* 0xCAC1 -> raw; 0xCAC2 -> fill; 0xCAC3 -> don't care */ +__le16 reserved1; +__le32 chunk_sz;/* in blocks in output image */ +__le32 total_sz;/* in bytes of chunk input file including chunk header and data */ +}chunk_header_t ; + +/* Following a Raw or Fill or CRC32 chunk is data. +* For a Raw chunk, it's the data in chunk_sz * blk_sz. +* For a Fill chunk, it's 4 bytes of the fill data. +* For a CRC32 chunk, it's 4 bytes of CRC32 +*/ + +#endif
Signature · 1 line
1.8.5
PATCH v3 1/4 usb/gadget: fastboot: add sparse image definitions
Quoted 36 lines · Steve Rae
- to prepare for the support of fastboot sparse images
Signed-off-by Steve Rae <[email protected]>---
This file is ASIS from:https://raw.githubusercontent.com/AOSB/android_system_core/master/libspars
e/sparse_format.h(commit28fa5bc347390480fe190294c6c385b6a9f0d68b) except for the__UBOOT__conditional include.Changes in v3: None
Changes in v2: Noneinclude/sparse_format.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 include/sparse_format.hinclude/sparse_format.h new file +12
-0@@ -0,0 +1,58 @@ +/* +* Copyright (C) 2010 The Android Open Source Project +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
Quoted 2 lines
implied. + * See the License for the specific language governing permissions and + * limitations under the License.
Please use the SPDX licence identifiers (pelase see Licenses/README)?
Quoted 12 lines
+ */
include/sparse_format.h new file +11
-0+ +#ifndef _LIBSPARSE_SPARSE_FORMAT_H_ +#define _LIBSPARSE_SPARSE_FORMAT_H_ +#define __UBOOT__ +#ifndef __UBOOT__ +#include "sparse_defs.h" +#endif + +typedef struct sparse_header { + __le32 magic; /* 0xed26ff3a */ + __le16 major_version; /* (0x1) - reject images with higher major
Quoted 4 lines
versions */ +
__le16minor_version; /* (0x0) - allow images with higer
minor versions */ +__le16file_hdr_sz; /* 28 bytes for first revision of
the file format */ +__le16chunk_hdr_sz; /* 12 bytes for first revision
of the file format */ +__le32blk_sz; /* block size in bytes,
must be a
Quoted 4 lines
multiple of 4 (4096) */ +
__le32total_blks; /* total blocks in the
non-sparse output image */ +__le32total_chunks; /* total chunks in the
sparse input image */ +__le32image_checksum; /* CRC32 checksum of the
original data, counting "don't care" */ + /* as 0.
Standard 802.3
include/sparse_format.h
Quoted 13 lines
polynomial, use a Public Domain */
include/sparse_format.h new file +12
-0+/* table implementation */ +} sparse_header_t; + +#define SPARSE_HEADER_MAGIC 0xed26ff3a + +#define CHUNK_TYPE_RAW 0xCAC1 +#define CHUNK_TYPE_FILL 0xCAC2 +#define CHUNK_TYPE_DONT_CARE 0xCAC3 +#define CHUNK_TYPE_CRC32 0xCAC4 + +typedef struct chunk_header { + __le16 chunk_type; /* 0xCAC1 -> raw; 0xCAC2 -> fill; 0xCAC3 ->
don't
include/sparse_format.h
Quoted 3 lines
care */ +
__le16reserved1;include/sparse_format.h new file +2
-0+__le32 chunk_sz;/* in blocks in output image */ +__le32 total_sz;/* in bytes of chunk input file including chunk
header
include/sparse_format.h
Quoted 3 lines
and data */ +}
chunk_header_t;include/sparse_format.h new file +2
-0+ +/* Following a Raw or Fill or CRC32 chunk is data.
The comment here is not aligned with coding style, I'll leave fixing it up to you, since the license header text needs revisiting.
Quoted 6 lines
+ * For a Raw chunk, it's the data in chunk_sz * blk_sz. + * For a Fill chunk, it's 4 bytes of the fill data. + * For a CRC32 chunk, it's 4 bytes of CRC32+ */
include/sparse_format.h new file +2
-0+ +#endif
PATCH v3 1/4 usb/gadget: fastboot: add sparse image definitions
Quoted 89 lines · Marek Vasut
Quoted 36 lines · Steve Rae
- to prepare for the support of fastboot sparse images
Signed-off-by Steve Rae <[email protected]>---
This file is ASIS from:https://raw.githubusercontent.com/AOSB/android_system_core/master/libspars
e/sparse_format.h(commit28fa5bc347390480fe190294c6c385b6a9f0d68b) except for the__UBOOT__conditional include.Changes in v3: None
Changes in v2: Noneinclude/sparse_format.h | 58+++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
create mode 100644include/sparse_format.hinclude/sparse_format.h new file +12
-0@@ -0,0 +1,58 @@ +/* +* Copyright (C) 2010 The Android Open Source Project +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or Quoted 2 lines
implied. + * See the License for the specific language governing permissions and + * limitations under the License.
Please use the SPDX licence identifiers (pelase see Licenses/README)?
Quoted 12 lines
+ */
include/sparse_format.h new file +11
-0+ +#ifndef _LIBSPARSE_SPARSE_FORMAT_H_ +#define _LIBSPARSE_SPARSE_FORMAT_H_ +#define __UBOOT__ +#ifndef __UBOOT__ +#include "sparse_defs.h" +#endif + +typedef struct sparse_header { + __le32 magic; /* 0xed26ff3a */ + __le16 major_version; /* (0x1) - reject images with higher majorQuoted 4 lines
versions */ +
__le16minor_version; /* (0x0) - allow images with higer
minor versions */ +__le16file_hdr_sz; /* 28 bytes for first revision of
the file format */ +__le16chunk_hdr_sz; /* 12 bytes for first revision
of the file format */ +__le32blk_sz; /* block size in bytes,must be a
Quoted 4 lines
multiple of 4 (4096) */ +
__le32total_blks; /* total blocks in the
non-sparse output image */ +__le32total_chunks; /* total chunks in the
sparse input image */ +__le32image_checksum; /* CRC32 checksum of the
original data, counting "don't care" */ + /* as 0.Standard 802.3
include/sparse_format.h
Quoted 13 lines
polynomial, use a Public Domain */
include/sparse_format.h new file +12
-0+/* table implementation */ +} sparse_header_t; + +#define SPARSE_HEADER_MAGIC 0xed26ff3a + +#define CHUNK_TYPE_RAW 0xCAC1 +#define CHUNK_TYPE_FILL 0xCAC2 +#define CHUNK_TYPE_DONT_CARE 0xCAC3 +#define CHUNK_TYPE_CRC32 0xCAC4 + +typedef struct chunk_header { + __le16 chunk_type; /* 0xCAC1 -> raw; 0xCAC2 -> fill; 0xCAC3 ->don't
include/sparse_format.h
Quoted 3 lines
care */ +
__le16reserved1;include/sparse_format.h new file +2
-0+__le32 chunk_sz;/* in blocks in output image */ +__le32 total_sz;/* in bytes of chunk input file including chunk header
include/sparse_format.h
Quoted 3 lines
and data */ +}
chunk_header_t;include/sparse_format.h new file +2
-0+ +/* Following a Raw or Fill or CRC32 chunk is data. The comment here is not aligned with coding style, I'll leave fixing it up to you, since the license header text needs revisiting.
Quoted 6 lines
include/sparse_format.h
Quoted 6 lines
+ * For a Raw chunk, it's the data in chunk_sz * blk_sz. + * For a Fill chunk, it's 4 bytes of the fill data. + * For a CRC32 chunk, it's 4 bytes of CRC32+ */
include/sparse_format.h new file +2
-0+ +#endif
To clarify:
I am taking this file ASIS from the location stated in the commit message....
Do we _really_ want to modify _anything_ in this file (especially when the content is not changing); or do we want to leaving it pristine?
PATCH v3 1/4 usb/gadget: fastboot: add sparse image definitions
Quoted 45 lines · Steve Rae
Quoted 44 lines · Marek Vasut
Quoted 38 lines · Steve Rae
- to prepare for the support of fastboot sparse images
Signed-off-by Steve Rae <[email protected]>---
This file is ASIS from:https://raw.githubusercontent.com/AOSB/android_system_core/master/libspa
rse/sparse_format.h(commit28fa5bc347390480fe190294c6c385b6a9f0d68b) except for the__UBOOT__conditional include.Changes in v3: None
Changes in v2: Noneinclude/sparse_format.h | 58+++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
create mode 100644
include/sparse_format.hinclude/sparse_format.h new file +12
-0@@ -0,0 +1,58 @@ +/* +* Copyright (C) 2010 The Android Open Source Project +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or Quoted 2 lines
implied. + * See the License for the specific language governing permissions and + * limitations under the License.
Please use the SPDX licence identifiers (pelase see Licenses/README)?
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
+ */
include/sparse_format.h new file +4
-0+ +#ifndef _LIBSPARSE_SPARSE_FORMAT_H_ +#define _LIBSPARSE_SPARSE_FORMAT_H_ +#define __UBOOT__Quoted 8 lines
Quoted 8 lines
include/sparse_format.h new file +7
-0+#ifndef __UBOOT__ +#include "sparse_defs.h" +#endif + +typedef struct sparse_header { + __le32 magic; /* 0xed26ff3a */ + __le16 major_version; /* (0x1) - reject images with higher majorversions */ +
__le16minor_version; /* (0x0) - allow images with
higer
Quoted 1 line
Quoted 1 line
Quoted 1 line
minor versions */ +
__le16file_hdr_sz; /* 28 bytes for first
revision
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
of the file format */ +
__le16chunk_hdr_sz; /* 12 bytes for first
revision of the file format */ +__le32blk_sz; /* block size in
bytes,must be a
Quoted 2 lines
multiple of 4 (4096) */ +
__le32total_blks; /* total blocks in the
non-sparse output image */ +__le32total_chunks; /* total chunks
in
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
the sparse input image */ +
__le32image_checksum; /* CRC32 checksum
of the original data, counting "don't care" */ + /*
as 0.
Quoted 32 lines
Quoted 32 lines
Standard 802.3
include/sparse_format.h
Quoted 13 lines
polynomial, use a Public Domain */
include/sparse_format.h new file +12
-0+/* table implementation */ +} sparse_header_t; + +#define SPARSE_HEADER_MAGIC 0xed26ff3a + +#define CHUNK_TYPE_RAW 0xCAC1 +#define CHUNK_TYPE_FILL 0xCAC2 +#define CHUNK_TYPE_DONT_CARE 0xCAC3 +#define CHUNK_TYPE_CRC32 0xCAC4 + +typedef struct chunk_header { + __le16 chunk_type; /* 0xCAC1 -> raw; 0xCAC2 -> fill; 0xCAC3 ->don't
include/sparse_format.h
Quoted 3 lines
care */ +
__le16reserved1;include/sparse_format.h new file +2
-0+__le32 chunk_sz;/* in blocks in output image */ +__le32 total_sz;/* in bytes of chunk input file including chunk header
include/sparse_format.h
Quoted 3 lines
and data */ +}
chunk_header_t;include/sparse_format.h new file +2
-0+ +/* Following a Raw or Fill or CRC32 chunk is data. The comment here is not aligned with coding style, I'll leave fixing it up to you, since the license header text needs revisiting.
Quoted 12 lines
Quoted 6 lines
include/sparse_format.h
Quoted 6 lines
+ * For a Raw chunk, it's the data in chunk_sz * blk_sz. + * For a Fill chunk, it's 4 bytes of the fill data. + * For a CRC32 chunk, it's 4 bytes of CRC32+ */
include/sparse_format.h new file +2
-0+ +#endif To clarify:
I am taking this file ASIS from the location stated in the commit message....
Do we _really_ want to modify _anything_ in this file (especially when the content is not changing); or do we want to leaving it pristine?
I'd prefer consistency with the rest of the codebase. Let's wait what others think.
PATCH v3 1/4 usb/gadget: fastboot: add sparse image definitions
In reply to Steve RaeQuoted 36 lines · Steve Rae
Quoted 33 lines · Marek Vasut
Quoted 32 lines · Steve Rae
- to prepare for the support of fastboot sparse images
Signed-off-by Steve Rae <[email protected]>---
This file is ASIS from:https://raw.githubusercontent.com/AOSB/android_system_core/master/libspars
e/sparse_format.h(commit28fa5bc347390480fe190294c6c385b6a9f0d68b) except for the__UBOOT__conditional include.Changes in v3: None
Changes in v2: Noneinclude/sparse_format.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 include/sparse_format.hinclude/sparse_format.h new file +8
-0@@ -0,0 +1,58 @@ +/* +* Copyright (C) 2010 The Android Open Source Project +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
include/sparse_format.h new file +4
-0+* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License.
Please use the SPDX licence identifiers (pelase see Licenses/README)?
And again, the license itself _is_ a problem.
[snip]
Quoted 6 lines
To clarify:
I am taking this file ASIS from the location stated in the commit message....
Do we _really_ want to modify _anything_ in this file (especially when the content is not changing); or do we want to leaving it pristine?
I'd be fine with it being pristine if it was a GPL-compatible license.
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140805/6f9b4bde/attachment.pgp>
PATCH v3 1/4 usb/gadget: fastboot: add sparse image definitions
Quoted 49 lines · Tom Rini
Quoted 36 lines · Steve Rae
Quoted 33 lines · Marek Vasut
Quoted 32 lines · Steve Rae
- to prepare for the support of fastboot sparse images
Signed-off-by Steve Rae <[email protected]>---
This file is ASIS from:https://raw.githubusercontent.com/AOSB/android_system_core/master/libspars
e/sparse_format.h(commit28fa5bc347390480fe190294c6c385b6a9f0d68b) except for the__UBOOT__conditional include.Changes in v3: None
Changes in v2: Noneinclude/sparse_format.h | 58+++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
create mode 100644include/sparse_format.hinclude/sparse_format.h new file +8
-0@@ -0,0 +1,58 @@ +/* +* Copyright (C) 2010 The Android Open Source Project +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
include/sparse_format.h new file +4
-0+* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License.
Please use the SPDX licence identifiers (pelase see Licenses/README)?
And again, the license itself _is_ a problem.
[snip]
Quoted 9 lines
Quoted 6 lines
To clarify:
I am taking this file ASIS from the location stated in the commit message....
Do we _really_ want to modify _anything_ in this file (especially when the content is not changing); or do we want to leaving it pristine?I'd be fine with it being pristine if it was a GPL-compatible license.
Any ideas on how can we proceed? How can we get past this license issue? IMO, the "fastboot flash" command is severely hampered if we cannot support the "sparse" format.
Thanks in advance, Steve
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
In reply to Steve Rae- add support for 'fastboot flash' command for eMMC devices
---
I suspect that the "sparse image" handling (ie. the "while (remaining_chunks)" loop) has been implemented elsewhere -- I need help finding the original code to determine any licensing issues....
Thanks, Steve
Changes in v3:
- remove most references to 'mmc',
which leaves only one mmc specific function: mmc_get_dev()
Changes in v2:
- split large function into three
- improved handling of response messages
- additional partition size checking when writing sparse image
common/Makefile | 5 ++ common/fb_mmc.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/fb_mmc.h | 8 +++ 3 files changed, 203 insertions(+) create mode 100644 common/fb_mmc.c create mode 100644 include/fb_mmc.h
common/Makefile +5 -0
@@ -266,4 +266,9 @@ obj-$(CONFIG_IO_TRACE) += iotrace.o obj-y += memsize.o obj-y += stdio.o +# This option is not just y/n - it can have a numeric value +ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV +obj-y += fb_mmc.o +endif + CFLAGS_env_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)
common/fb_mmc.c new file +190 -0
190 lines not on this page · raw message
include/fb_mmc.h new file +8 -0
@@ -0,0 +1,8 @@ +/* +* Copyright 2014 Broadcom Corporation. +* +* SPDX-License-Identifier: GPL-2.0+ +*/ + +void fb_mmc_flash_write (const char *cmd,void *download_buffer, +unsigned int download_bytes,char *response);
Signature · 1 line
1.8.5
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
common/fb_mmc.c line 15
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ +#include <common.h> +#include <fb_mmc.h> +#include <part.h> +#include <sparse_format.h> + +/* The 64 defined bytes plus \0 */ +#define RESPONSE_LEN (64 + 1) + +static char *response_str;
I'd suggest to pass this "response_str" around instead of making it global.
common/fb_mmc.c line 20
Quoted 4 lines
common/fb_mmc.c new file +4
-0+static void fastboot_resp (const char *s) +{ +strncpy (response_str, s,RESPONSE_LEN ); + response_str[RESPONSE_LEN -1 ] ='\0' ;
This could be shrunk to a single snprintf(response_str, RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't grow.
Quoted 24 lines
common/fb_mmc.c new file +24
-0+} + +static int is_sparse_image (void *buf) +{ +sparse_header_t *s_header = (sparse_header_t *)buf; + +if ((le32_to_cpu (s_header->magic) ==SPARSE_HEADER_MAGIC ) && + (le16_to_cpu (s_header->major_version) ==1 )) +return 1 ; + +return 0 ; +} + +static void write_sparse_image (block_dev_desc_t *dev_desc, +disk_partition_t *info,const char *part_name, +void *buffer,unsigned int download_bytes) +{ +lbaint_t blk; +lbaint_t blkcnt; +lbaint_t blks; +sparse_header_t *s_header = (sparse_header_t *)buffer; +chunk_header_t *c_header; +void *buf; +uint32_t blk_sz;
common/fb_mmc.c line 54
Quoted 10 lines
common/fb_mmc.c new file +10
-0+uint32_t remaining_chunks; +uint32_t bytes_written =0 ; + + blk_sz =le32_to_cpu (s_header->blk_sz); + +/* verify s_header->blk_sz is exact multiple of info->blksz */ +if (blk_sz != (blk_sz & ~(info->blksz -1 ))) { +printf ("%s: Sparse image block size issue [%u]\n" , + __func__, blk_sz); +fastboot_resp ("FAILsparse image block size issue" );
Can't you just make the fastboot_resp() function a variadic one AND move the printf() into the fastboot_resp() function? You could then even get consistent output on both the device and in the response if you snprintf() into the response_str first and then printf() the response_str .
Quoted 2 lines
(quoted diff) +2
-0+ return; + }
[...]
Quoted 9 lines
+static void write_raw_image(block_dev_desc_t
*dev_desc,disk_partition_t
*info, + const char*part_name, void *buffer,common/fb_mmc.c new file +7
-0+ unsigned int download_bytes) +{ +lbaint_t blkcnt; +lbaint_t blks; + +/* determine number of blocks to write */ + blkcnt = ((download_bytes + (info->blksz -1 )) & ~(info->blksz -1 ));
common/fb_mmc.c line 143
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ blkcnt = blkcnt / info->blksz; + +if (blkcnt > info->size) { +printf ("%s: too large for partition: '%s'\n" , __func__, + part_name); +fastboot_resp ("FAILtoo large for partition" ); +return ; + } + +printf ("Flashing Raw Image\n" );
Use puts() here and everywhere where printf() is not taking any args please.
Quoted 13 lines
common/fb_mmc.c new file +13
-0+ blks = dev_desc->block_write (dev_desc->dev, info->start, blkcnt, + buffer); +if (blks != blkcnt) { +printf ("%s: failed writing to device %d\n" , __func__, + dev_desc->dev); +fastboot_resp ("FAILfailed writing to device" ); +return ; + } + +printf ("........ wrote " LBAFU " bytes to '%s'\n" , blkcnt * info->blksz, + part_name); +fastboot_resp ("OKAY" ); +}
[...]
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
Quoted 15 lines · Marek Vasut
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]common/fb_mmc.c line 15
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ +#include <common.h> +#include <fb_mmc.h> +#include <part.h> +#include <sparse_format.h> + +/* The 64 defined bytes plus \0 */ +#define RESPONSE_LEN (64 + 1) + +static char *response_str;I'd suggest to pass this "
response_str" around instead of making it global.
That would involve adding it to fastboot_resp(), which is called 11 times in this code, from 3 different functions (so would need to add this to two of the functions...). And as these evolve, there will likely be more nested functions, which would all require "passing it around".... I think that this "static global pointer" is a cleaner implementation.
Quoted 8 lines
common/fb_mmc.c line 20
Quoted 4 lines
common/fb_mmc.c new file +4
-0+static void fastboot_resp (const char *s) +{ +strncpy (response_str, s,RESPONSE_LEN ); + response_str[RESPONSE_LEN -1 ] ='\0' ;This could be shrunk to a single
snprintf(response_str, RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't grow.
snprintf() is used very sparingling in U-Boot, and with the cautionary statements in README (line 852) and the fact that CONFIG_SYS_VSNPRINTF is not defined for armv7 builds, I am not going to use it....
Quoted 31 lines
Quoted 31 lines
common/fb_mmc.c new file +31
-0+} + +static int is_sparse_image (void *buf) +{ +sparse_header_t *s_header = (sparse_header_t *)buf; + +if ((le32_to_cpu (s_header->magic) ==SPARSE_HEADER_MAGIC ) && + (le16_to_cpu (s_header->major_version) ==1 )) +return 1 ; + +return 0 ; +} + +static void write_sparse_image (block_dev_desc_t *dev_desc, +disk_partition_t *info,const char *part_name, +void *buffer,unsigned int download_bytes) +{ +lbaint_t blk; +lbaint_t blkcnt; +lbaint_t blks; +sparse_header_t *s_header = (sparse_header_t *)buffer; +chunk_header_t *c_header; +void *buf; +uint32_t blk_sz; +uint32_t remaining_chunks; +uint32_t bytes_written =0 ; + + blk_sz =le32_to_cpu (s_header->blk_sz); + +/* verify s_header->blk_sz is exact multiple of info->blksz */ +if (blk_sz != (blk_sz & ~(info->blksz -1 ))) {
Quoted 9 lines
common/fb_mmc.c line 54
Quoted 3 lines
common/fb_mmc.c new file +3
-0+printf ("%s: Sparse image block size issue [%u]\n" , + __func__, blk_sz); +fastboot_resp ("FAILsparse image block size issue" );Can't you just make the
fastboot_resp()function a variadic one AND move theprintf()into thefastboot_resp()function? You could then even get consistent output on both the device and in the response if yousnprintf()into theresponse_strfirst and thenprintf()theresponse_str.
Generally, the printf() statements which are sent to the console, and the fastboot_resp() statements which are sent to the host running the "fastboot" application are not the same....
Quoted 18 lines
Quoted 2 lines
(quoted diff) +2
-0+ return; + }[...]
Quoted 13 lines
+static void write_raw_image(block_dev_desc_t
*dev_desc,disk_partition_t
*info, + const char*part_name, void *buffer,common/fb_mmc.c new file +11
-0+ unsigned int download_bytes) +{ +lbaint_t blkcnt; +lbaint_t blks; + +/* determine number of blocks to write */ + blkcnt = ((download_bytes + (info->blksz -1 )) & ~(info->blksz -1 )); + blkcnt = blkcnt / info->blksz; + +if (blkcnt > info->size) { + printf("%s: too large for partition: '%s'\n", __func__,
Quoted 8 lines
common/fb_mmc.c line 143
Quoted 6 lines
common/fb_mmc.c new file +6
-0+ part_name); +fastboot_resp ("FAILtoo large for partition" ); +return ; + } + +printf ("Flashing Raw Image\n" );Use
puts()here and everywhere whereprintf()is not taking any args please.
done in v4 - Thanks!
Quoted 16 lines
Quoted 14 lines
common/fb_mmc.c new file +13
-0+ blks = dev_desc->block_write (dev_desc->dev, info->start, blkcnt, + buffer); +if (blks != blkcnt) { +printf ("%s: failed writing to device %d\n" , __func__, + dev_desc->dev); +fastboot_resp ("FAILfailed writing to device" ); +return ; + } + +printf ("........ wrote " LBAFU " bytes to '%s'\n" , blkcnt * info->blksz, + part_name); +fastboot_resp ("OKAY" ); +}[...]
Thanks, Steve
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
Quoted 18 lines · Steve Rae
Quoted 16 lines · Marek Vasut
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]common/fb_mmc.c line 15
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ +#include <common.h> +#include <fb_mmc.h> +#include <part.h> +#include <sparse_format.h> + +/* The 64 defined bytes plus \0 */ +#define RESPONSE_LEN (64 + 1) + +static char *response_str;I'd suggest to pass this "
response_str" around instead of making it global.
Quoted 5 lines
That would involve adding it to
fastboot_resp(), which is called 11 times in this code, from 3 different functions (so would need to add this to two of the functions...). And as these evolve, there will likely be more nested functions, which would all require "passing it around".... I think that this "static global pointer" is a cleaner implementation.
Eventually, the amount of these static variables in the code will grow and it will become increasingly difficult to weed them out. I believe it would be even better to pass around some kind of a structure with "private data" of the fastboot, which would cater for all possible variables which might come in the future. What do you think ?
Quoted 9 lines
Quoted 7 lines
common/fb_mmc.c line 20
Quoted 4 lines
common/fb_mmc.c new file +4
-0+static void fastboot_resp (const char *s) +{ +strncpy (response_str, s,RESPONSE_LEN ); + response_str[RESPONSE_LEN -1 ] ='\0' ;This could be shrunk to a single
snprintf(response_str, RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't grow.
snprintf()is used very sparingling in U-Boot
This is not a reason to avoid it.
Quoted 1 line
, and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
Quoted 1 line
and the fact that
CONFIG_SYS_VSNPRINTFis not defined for armv7 builds, I am
not going to use it....
Is it a problem to define it? Also, even without CONFIG_SYS_VSNPRINTF , the functions are still available, see the README:
857 If this option is not given then these functions will 858 silently discard their buffer size argument - this means 859 you are not getting any overflow checking in this case.
I have yet to see some hard-evidence against using safe printing functions here.
Quoted 41 lines
Quoted 32 lines
Quoted 32 lines
common/fb_mmc.c new file +32
-0+} + +static int is_sparse_image (void *buf) +{ +sparse_header_t *s_header = (sparse_header_t *)buf; + +if ((le32_to_cpu (s_header->magic) ==SPARSE_HEADER_MAGIC ) && + (le16_to_cpu (s_header->major_version) ==1 )) +return 1 ; + +return 0 ; +} + +static void write_sparse_image (block_dev_desc_t *dev_desc, +disk_partition_t *info,const char *part_name, +void *buffer,unsigned int download_bytes) +{ +lbaint_t blk; +lbaint_t blkcnt; +lbaint_t blks; +sparse_header_t *s_header = (sparse_header_t *)buffer; +chunk_header_t *c_header; +void *buf; +uint32_t blk_sz; +uint32_t remaining_chunks; +uint32_t bytes_written =0 ; + + blk_sz =le32_to_cpu (s_header->blk_sz); + +/* verify s_header->blk_sz is exact multiple of info->blksz */ +if (blk_sz != (blk_sz & ~(info->blksz -1 ))) { + printf("%s: Sparse image block size issue [%u]\n",Quoted 8 lines
common/fb_mmc.c line 54
Quoted 2 lines
common/fb_mmc.c new file +2
-0+ __func__, blk_sz); +fastboot_resp ("FAILsparse image block size issue" );Can't you just make the
fastboot_resp()function a variadic one AND move theprintf()into thefastboot_resp()function? You could then even get consistent output on both the device and in the response if yousnprintf()into theresponse_strfirst and thenprintf()theresponse_str.
Quoted 3 lines
Generally, the
printf()statements which are sent to the console, and thefastboot_resp()statements which are sent to the host running the "fastboot" application are not the same....
OK, thanks!
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
Quoted 25 lines · Marek Vasut
Quoted 18 lines · Steve Rae
Quoted 16 lines · Marek Vasut
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]common/fb_mmc.c line 15
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ +#include <common.h> +#include <fb_mmc.h> +#include <part.h> +#include <sparse_format.h> + +/* The 64 defined bytes plus \0 */ +#define RESPONSE_LEN (64 + 1) + +static char *response_str;I'd suggest to pass this "
response_str" around instead of making it global.Quoted 5 lines
That would involve adding it to
fastboot_resp(), which is called 11 times in this code, from 3 different functions (so would need to add this to two of the functions...). And as these evolve, there will likely be more nested functions, which would all require "passing it around".... I think that this "static global pointer" is a cleaner implementation.
Quoted 6 lines
Eventually, the amount of these static variables in the code will grow and it will become increasingly difficult to weed them out. I believe it would be even better to pass around some kind of a structure with "private data" of the fastboot, which would cater for all possible variables which might come in the future. What do you think ?
Yes -- if there is private data that the fastboot implementation requires, then a data structure is the way to go. However, I still think that this "fastboot response string" would even be an exception to that private data....
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
common/fb_mmc.c line 20
Quoted 4 lines
common/fb_mmc.c new file +4
-0+static void fastboot_resp (const char *s) +{ +strncpy (response_str, s,RESPONSE_LEN ); + response_str[RESPONSE_LEN -1 ] ='\0' ;This could be shrunk to a single
snprintf(response_str, RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't grow.
snprintf()is used very sparingling in U-BootThis is not a reason to avoid it.
true....
Quoted 4 lines
Quoted 2 lines
, and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
I was referring to what you mention below...
852 - Safe printf() functions
853 Define CONFIG_SYS_VSNPRINTF to compile in safe versions of 854 the printf() functions. These are defined in 855 include/vsprintf.h and include snprintf(), vsnprintf() and 856 so on. Code size increase is approximately 300-500 bytes. 857 If this option is not given then these functions will 858 silently discard their buffer size argument - this means 859 you are not getting any overflow checking in this case.
Quoted 4 lines
Quoted 2 lines
and the fact that
CONFIG_SYS_VSNPRINTFis not defined for armv7 builds, I amnot going to use it....
Quoted 8 lines
Is it a problem to define it? Also, even without
CONFIG_SYS_VSNPRINTF, the functions are still available, see the README:857 If this option is not given then these functions will 858 silently discard their buffer size argument - this means 859 you are not getting any overflow checking in this case.I have yet to see some hard-evidence against using safe printing functions here.
I don't want to be the first to defined it for all of armv7.... And I really don't want to define it only only my boards running so that they can run 'fastboot'
What do you suggest?
Quoted 41 lines
Quoted 41 lines
Quoted 40 lines
Quoted 24 lines
common/fb_mmc.c new file +24
-0+} + +static int is_sparse_image (void *buf) +{ +sparse_header_t *s_header = (sparse_header_t *)buf; + +if ((le32_to_cpu (s_header->magic) ==SPARSE_HEADER_MAGIC ) && + (le16_to_cpu (s_header->major_version) ==1 )) +return 1 ; + +return 0 ; +} + +static void write_sparse_image (block_dev_desc_t *dev_desc, +disk_partition_t *info,const char *part_name, +void *buffer,unsigned int download_bytes) +{ +lbaint_t blk; +lbaint_t blkcnt; +lbaint_t blks; +sparse_header_t *s_header = (sparse_header_t *)buffer; +chunk_header_t *c_header; +void *buf; +uint32_t blk_sz;common/fb_mmc.c line 54
Quoted 10 lines
common/fb_mmc.c new file +10
-0+uint32_t remaining_chunks; +uint32_t bytes_written =0 ; + + blk_sz =le32_to_cpu (s_header->blk_sz); + +/* verify s_header->blk_sz is exact multiple of info->blksz */ +if (blk_sz != (blk_sz & ~(info->blksz -1 ))) { +printf ("%s: Sparse image block size issue [%u]\n" , + __func__, blk_sz); +fastboot_resp ("FAILsparse image block size issue" );Can't you just make the
fastboot_resp()function a variadic one AND move theprintf()into thefastboot_resp()function? You could then even get consistent output on both the device and in the response if yousnprintf()into theresponse_strfirst and thenprintf()theresponse_str.
Quoted 6 lines
Quoted 3 lines
Generally, the
printf()statements which are sent to the console, and thefastboot_resp()statements which are sent to the host running the "fastboot" application are not the same....OK, thanks!
Thanks, Steve
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
Quoted 33 lines · Steve Rae
Quoted 26 lines · Marek Vasut
Quoted 18 lines · Steve Rae
Quoted 16 lines · Marek Vasut
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]common/fb_mmc.c line 15
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ +#include <common.h> +#include <fb_mmc.h> +#include <part.h> +#include <sparse_format.h> + +/* The 64 defined bytes plus \0 */ +#define RESPONSE_LEN (64 + 1) + +static char *response_str;I'd suggest to pass this "
response_str" around instead of making it global.Quoted 6 lines
That would involve adding it to
fastboot_resp(), which is called 11 times in this code, from 3 different functions (so would need to add this to two of the functions...). And as these evolve, there will likely be more nested functions, which would all require "passing it around".... I think that this "static global pointer" is a cleaner implementation.Quoted 5 lines
Eventually, the amount of these static variables in the code will grow and it will become increasingly difficult to weed them out. I believe it would be even better to pass around some kind of a structure with "private data" of the fastboot, which would cater for all possible variables which might come in the future. What do you think ?
Quoted 4 lines
Yes -- if there is private data that the fastboot implementation requires, then a data structure is the way to go. However, I still think that this "fastboot response string" would even be an exception to that private data....
OK, let's leave it this way for now.
Quoted 22 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
common/fb_mmc.c line 20
Quoted 4 lines
common/fb_mmc.c new file +4
-0+static void fastboot_resp (const char *s) +{ +strncpy (response_str, s,RESPONSE_LEN ); + response_str[RESPONSE_LEN -1 ] ='\0' ;This could be shrunk to a single
snprintf(response_str, RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't grow.Quoted 3 lines
Quoted 1 line
snprintf()is used very sparingling in U-BootThis is not a reason to avoid it.
true....
Quoted 4 lines
Quoted 1 line
, and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
I was referring to what you mention below...
852 - Safeprintf()functions
Quoted 7 lines
853 Define CONFIG_SYS_VSNPRINTF to compile in safe versions of 854 the printf() functions. These are defined in 855 include/vsprintf.h and include snprintf(), vsnprintf() and 856 so on. Code size increase is approximately 300-500 bytes. 857 If this option is not given then these functions will 858 silently discard their buffer size argument - this means 859 you are not getting any overflow checking in this case.
I really don't see the "cautionary statements" here , no . I see that it discards the size checking if this CONFIG_SYS_VSNPRINTF is not enabled, but that does not obstruct the operation of those functions.
Quoted 14 lines
Quoted 14 lines
Quoted 2 lines
and the fact that
CONFIG_SYS_VSNPRINTFis not defined for armv7 builds, I amnot going to use it....
Is it a problem to define it? Also, even without
CONFIG_SYS_VSNPRINTF, thefunctions are still available, see the README:
857 If this option is not given then these functions
will 858 silently discard their buffer size argument -
this means 859 you are not getting any overflow
checking in this case.
Quoted 4 lines
Quoted 2 lines
I have yet to see some hard-evidence against using safe printing functions here.
I don't want to be the first to defined it for all of armv7....
Honestly, we should just enable this CONFIG_SYS_VSNPRINTF by default for the good of humanity and all the things, since this unbounded string handling is just evil (see how OpenSSL ended up, partly because of that ... and I am just starting to see the pattern in all the security code). I don't want to go down that road with U-Boot.
So, would you please cook a separate patch to enable this by default, so it would spur the right kind of discussion on this matter ?
Quoted 3 lines
And I really don't want to define it only only my boards running so that they can run 'fastboot'
What do you suggest?
See above, thanks !
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
Hi Marek,
[snip]
Quoted 11 lines
Quoted 1 line
I don't want to be the first to defined it for all of armv7....
Honestly, we should just enable this
CONFIG_SYS_VSNPRINTFby default for the good of humanity and all the things, since this unbounded string handling is just evil (see how OpenSSL ended up, partly because of that ... and I am just starting to see the pattern in all the security code). I don't want to go down that road with U-Boot.So, would you please cook a separate patch to enable this by default, so it would spur the right kind of discussion on this matter ?
We should enable this by default. Unbounded string handling scares me.
If we have problems with blowing over SPL size restrictions, perhaps have it disabled only on those cases (that are known to have a problem).
Quoted 5 lines
Quoted 3 lines
And I really don't want to define it only only my boards running so that they can run 'fastboot'
What do you suggest?See above, thanks !
Regards
-- Pantelis
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
Quoted 12 lines · Pantelis Antoniou
Hi Marek,
[snip]
Quoted 8 lines
Quoted 1 line
I don't want to be the first to defined it for all of armv7....
Honestly, we should just enable this
CONFIG_SYS_VSNPRINTFby default for the good of humanity and all the things, since this unbounded string handling is just evil (see how OpenSSL ended up, partly because of that ... and I am just starting to see the pattern in all the security code). I don't want to go down that road with U-Boot.
Quoted 7 lines
Quoted 2 lines
So, would you please cook a separate patch to enable this by default, so it would spur the right kind of discussion on this matter ?
We should enable this by default. Unbounded string handling scares me.
If we have problems with blowing over SPL size restrictions, perhaps have it disabled only on those cases (that are known to have a problem).
Right, I fully agree with what you said. The SPL and TPL might have issues with this being enabled, but then this can be enabled for full-blown U-Boot only.
But this discussion should happen in a thread associated with patch enabling this. ;-)
Best regards,
Marek Vasut
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
In reply to Marek VasutQuoted 71 lines · Marek Vasut
Quoted 33 lines · Steve Rae
Quoted 26 lines · Marek Vasut
Quoted 18 lines · Steve Rae
Quoted 16 lines · Marek Vasut
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]common/fb_mmc.c line 15
Quoted 10 lines
common/fb_mmc.c new file +10
-0+ +#include <common.h> +#include <fb_mmc.h> +#include <part.h> +#include <sparse_format.h> + +/* The 64 defined bytes plus \0 */ +#define RESPONSE_LEN (64 + 1) + +static char *response_str;I'd suggest to pass this "
response_str" around instead of making it global.Quoted 6 lines
That would involve adding it to
fastboot_resp(), which is called 11 times in this code, from 3 different functions (so would need to add this to two of the functions...). And as these evolve, there will likely be more nested functions, which would all require "passing it around".... I think that this "static global pointer" is a cleaner implementation.Quoted 5 lines
Eventually, the amount of these static variables in the code will grow and it will become increasingly difficult to weed them out. I believe it would be even better to pass around some kind of a structure with "private data" of the fastboot, which would cater for all possible variables which might come in the future. What do you think ?
Quoted 4 lines
Yes -- if there is private data that the fastboot implementation requires, then a data structure is the way to go. However, I still think that this "fastboot response string" would even be an exception to that private data....
OK, let's leave it this way for now.
Quoted 22 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
common/fb_mmc.c line 20
Quoted 4 lines
common/fb_mmc.c new file +4
-0+static void fastboot_resp (const char *s) +{ +strncpy (response_str, s,RESPONSE_LEN ); + response_str[RESPONSE_LEN -1 ] ='\0' ;This could be shrunk to a single
snprintf(response_str, RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't grow.Quoted 3 lines
Quoted 1 line
snprintf()is used very sparingling in U-BootThis is not a reason to avoid it.
true....
Quoted 4 lines
Quoted 1 line
, and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
I was referring to what you mention below...
852 - Safeprintf()functionsQuoted 7 lines
853 Define CONFIG_SYS_VSNPRINTF to compile in safe versions of 854 the printf() functions. These are defined in 855 include/vsprintf.h and include snprintf(), vsnprintf() and 856 so on. Code size increase is approximately 300-500 bytes. 857 If this option is not given then these functions will 858 silently discard their buffer size argument - this means 859 you are not getting any overflow checking in this case.
Quoted 4 lines
I really don't see the "cautionary statements" here , no . I see that it discards the size checking if this
CONFIG_SYS_VSNPRINTFis not enabled, but that does not obstruct the operation of those functions.
I'm really confused: my code ensures that the buffer is not overflowed and that it is terminated properly. If snprintf() (without CONFIG_SYS_VSNPRINTF defined) doesn't provide "any overflow checking", then why would I use it?
Quoted 19 lines
Quoted 14 lines
Quoted 14 lines
Quoted 2 lines
and the fact that
CONFIG_SYS_VSNPRINTFis not defined for armv7 builds, I amnot going to use it....
Is it a problem to define it? Also, even without
CONFIG_SYS_VSNPRINTF, thefunctions are still available, see the README:
857 If this option is not given then these functions
will 858 silently discard their buffer size argument -
this means 859 you are not getting any overflow
checking in this case.Quoted 4 lines
Quoted 2 lines
I have yet to see some hard-evidence against using safe printing functions here.
I don't want to be the first to defined it for all of armv7....
Quoted 8 lines
Honestly, we should just enable this
CONFIG_SYS_VSNPRINTFby default for the good of humanity and all the things, since this unbounded string handling is just evil (see how OpenSSL ended up, partly because of that ... and I am just starting to see the pattern in all the security code). I don't want to go down that road with U-Boot.So, would you please cook a separate patch to enable this by default, so it would spur the right kind of discussion on this matter ?
I will apologize in advance, but I just don't know anything about SPL or TPL or any other boards (outside of my very limited armv7 and armv8 scope)....
I would be happy to review and test this suggested patch (on our boards), but would be uncomfortable with proposing this patch. Please go ahead and submit a patch, and I'll check it! Thanks, Steve
Quoted 7 lines
Quoted 4 lines
And I really don't want to define it only only my boards running so that they can run 'fastboot'
What do you suggest?See above, thanks !
PATCH v3 2/4 usb/gadget: fastboot: add eMMC support for flash command
On Thursday, August 07, 2014 at 06:52:44 PM, Steve Rae wrote:
[...]
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
I was referring to what you mention below...
852 - Safe
printf()functions853 Define CONFIG_SYS_VSNPRINTF to compile in safe versions of 854 the printf() functions. These are defined in 855 include/vsprintf.h and include snprintf(), vsnprintf() and 856 so on. Code size increase is approximately 300-500 bytes. 857 If this option is not given then these functions will 858 silently discard their buffer size argument - this means 859 you are not getting any overflow checking in this case.
Quoted 8 lines
Quoted 3 lines
I really don't see the "cautionary statements" here , no . I see that it discards the size checking if this
CONFIG_SYS_VSNPRINTFis not enabled, but that does not obstruct the operation of those functions.I'm really confused: my code ensures that the buffer is not overflowed and that it is terminated properly. If
snprintf()(withoutCONFIG_SYS_VSNPRINTFdefined) doesn't provide "any overflow checking", then why would I use it?
That's why I suggested to enable CONFIG_SYS_VSNPRINTF unconditionally. Then your code would not need to duplicate all the overflow checks, would it ?
Quoted 25 lines
Quoted 25 lines
Quoted 14 lines
Quoted 14 lines
Quoted 2 lines
and the fact that
CONFIG_SYS_VSNPRINTFis not defined for armv7 builds, I amnot going to use it....
Is it a problem to define it? Also, even without
CONFIG_SYS_VSNPRINTF, thefunctions are still available, see the README:
857 If this option is not given then these functions
will 858 silently discard their buffer size argument
- this means 859 you are not getting any overflow
checking in this case.Quoted 4 lines
Quoted 2 lines
I have yet to see some hard-evidence against using safe printing functions here.
I don't want to be the first to defined it for all of armv7....
Honestly, we should just enable this
CONFIG_SYS_VSNPRINTFby default for the good of humanity and all the things, since this unbounded string handling is just evil (see how OpenSSL ended up, partly because of that ... and I am just starting to see the pattern in all the security code). I don't want to go down that road with U-Boot.
Quoted 6 lines
Quoted 2 lines
So, would you please cook a separate patch to enable this by default, so it would spur the right kind of discussion on this matter ?
I will apologize in advance, but I just don't know anything about SPL or TPL or any other boards (outside of my very limited armv7 and armv8 scope)....
That's OK.
Quoted 3 lines
I would be happy to review and test this suggested patch (on our boards), but would be uncomfortable with proposing this patch. Please go ahead and submit a patch, and I'll check it!
The patch would go something like:
#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_TPL_BUILD) #define CONFIG_SYS_VSNPRINTF
#endif
and this would go into include/config_cmd_default.h . Unless I'm wrong.
PATCH v3 3/4 usb/gadget: fastboot: add support for flash command
In reply to Steve Rae- implement 'fastboot flash' for eMMC devices
---
Changes in v3: None
Changes in v2:
- update README.android-fastboot file
README | 10 ++++++++++ doc/README.android-fastboot | 5 +++-- drivers/usb/gadget/f_fastboot.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-)
README +10 -0
@@ -1623,6 +1623,16 @@ The following options need to be configured: downloads. This buffer should be as large as possible for a platform. Define this to the size available RAM for fastboot. + CONFIG_FASTBOOT_FLASH + The fastboot protocol includes a "flash" command for writing + the downloaded image to a non-volatile storage device. Define + this to enable the "fastboot flash" command. + + CONFIG_FASTBOOT_FLASH_MMC_DEV + The fastboot "flash" command requires addition information + regarding the non-volatile storage device. Define this to + the eMMC device that fastboot should use to store the image. + - Journaling Flash filesystem support: CONFIG_JFFS2_NAND, CONFIG_JFFS2_NAND_OFF, CONFIG_JFFS2_NAND_SIZE, CONFIG_JFFS2_NAND_DEV
doc/README.android-fastboot +3 -2
@@ -6,8 +6,9 @@ Overview The protocol that is used over USB is described in README.android-fastboot-protocol in same directory. -The current implementation does not yet support the flash and erase -commands. +The current implementation does not yet support the erase command or the +"oem format" command, and there is minimal support for the flash command; +it only supports eMMC devices. Client installation ===================
drivers/usb/gadget/f_fastboot.c +31 -0
@@ -19,6 +19,9 @@#include <linux/compiler.h> #include <version.h> #include <g_dnl.h> +#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV +#include <fb_mmc.h> +#endif #define FASTBOOT_VERSION "0.4"
@@ -466,6 +469,28 @@ static void cb_boot(struct usb_ep *ep, struct usb_request *req)fastboot_tx_write_str ("OKAY" ); } +#ifdef CONFIG_FASTBOOT_FLASH +static void cb_flash (struct usb_ep *ep,struct usb_request *req) +{ +char *cmd = req->buf; +char response[RESPONSE_LEN ]; + +strsep (&cmd,":" ); +if (!cmd) { +printf ("%s: missing partition name\n" , __func__); +fastboot_tx_write_str ("FAILmissing partition name" ); +return ; + } + +strcpy (response,"FAILno flash device defined" ); +#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV +fb_mmc_flash_write (cmd, (void *)CONFIG_USB_FASTBOOT_BUF_ADDR , + download_bytes, response); +#endif +fastboot_tx_write_str (response); +} +#endif +struct cmd_dispatch_info {char *cmd;void (*cb)(struct usb_ep *ep,struct usb_request *req);
@@ -485,6 +510,12 @@ static const struct cmd_dispatch_info cmd_dispatch_info[] = { .cmd = "boot", .cb = cb_boot, }, +#ifdef CONFIG_FASTBOOT_FLASH + { + .cmd = "flash", + .cb = cb_flash, + }, +#endif };static void rx_handler_command (struct usb_ep *ep,struct usb_request *req)
Signature · 1 line
1.8.5
PATCH v3 3/4 usb/gadget: fastboot: add support for flash command
Quoted 3 lines · Steve Rae
- implement 'fastboot flash' for eMMC devices
Signed-off-by Steve Rae <[email protected]>
Best regards,
Marek Vasut
PATCH v3 3/4 usb/gadget: fastboot: add support for flash command
Quoted 6 lines · Marek Vasut
Quoted 3 lines · Steve Rae
- implement 'fastboot flash' for eMMC devices
Signed-off-by Steve Rae <[email protected]>Reviewed-by Marek Vasut <[email protected]>
Thanks, Steve
Quoted 4 lines
Best regards,
Marek Vasut
PATCH v3 4/4 usb/gadget: fastboot: minor cleanup
In reply to Steve Rae- update static function
- additional debugging statements
---
Changes in v3: None
Changes in v2:
- new in v2
drivers/usb/gadget/f_fastboot.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/usb/gadget/f_fastboot.c +7 -3
@@ -293,7 +293,7 @@ static int fastboot_add(struct usb_configuration *c) }DECLARE_GADGET_BIND_CALLBACK (usb_dnl_fastboot, fastboot_add); -int fastboot_tx_write (const char *buffer,unsigned int buffer_size) +static int fastboot_tx_write (const char *buffer,unsigned int buffer_size) {struct usb_request *in_req = fastboot_func->in_req;int ret;
@@ -338,6 +338,7 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)strcpy (response,"OKAY" );strsep (&cmd,":" );if (!cmd) { +printf ("%s: missing var\n" , __func__);fastboot_tx_write_str ("FAILmissing var" );return ; }
@@ -358,6 +359,7 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)else strcpy (response,"FAILValue not set" ); }else { +printf ("%s: unknown variable: %s\n" , __func__, cmd);strcpy (response,"FAILVariable not implemented" ); }fastboot_tx_write_str (response);
@@ -531,10 +533,12 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) } } -if (!func_cb) +if (!func_cb) { +printf ("%s: unknown command: %s\n" , __func__, cmdbuf);fastboot_tx_write_str ("FAILunknown command" ); -else + }else {func_cb (ep, req); + }if (req->status ==0 ) { *cmdbuf ='\0' ;
Signature · 1 line
1.8.5
PATCH v3 4/4 usb/gadget: fastboot: minor cleanup
Quoted 27 lines · Steve Rae
- update static function
- additional debugging statementsSigned-off-by Steve Rae <[email protected]>---
Changes in v3: None
Changes in v2:
- new in v2drivers/usb/gadget/f_fastboot.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/usb/gadget/f_fastboot.cindex 89c2d3e..3e6e47f 100644drivers/usb/gadget/f_fastboot.c +1
-1@@ -293,7 +293,7 @@ static int fastboot_add(struct usb_configuration *c) }DECLARE_GADGET_BIND_CALLBACK (usb_dnl_fastboot, fastboot_add); -int fastboot_tx_write (const char *buffer,unsigned int buffer_size) +static int fastboot_tx_write (const char *buffer,unsigned int buffer_size) {struct usb_request *in_req = fastboot_func->in_req;int ret;
drivers/usb/gadget/f_fastboot.c static void cb_getvar(struct usb_ep *ep, struct · line 341
Quoted 4 lines
drivers/usb/gadget/f_fastboot.c +1
-0@@ -338,6 +338,7 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) strcpy(response, "OKAY");strsep (&cmd,":" );if (!cmd) { +printf ("%s: missing var\n" , __func__);
I'd spell it out completely -- variable -- but I'm not sure if you might need to maintain some kind of compatibility with the fastboot responses here or not. [...]
Best regards,
Marek Vasut
PATCH v3 4/4 usb/gadget: fastboot: minor cleanup
Quoted 22 lines · Marek Vasut
Quoted 21 lines · Steve Rae
- update static function
- additional debugging statementsSigned-off-by Steve Rae <[email protected]>---
Changes in v3: None
Changes in v2:
- new in v2drivers/usb/gadget/f_fastboot.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/usb/gadget/f_fastboot.cindex 89c2d3e..3e6e47f 100644drivers/usb/gadget/f_fastboot.c +0
-0@@ -293,7 +293,7 @@ static int fastboot_add(struct usb_configuration *c) }DECLARE_GADGET_BIND_CALLBACK (usb_dnl_fastboot, fastboot_add);
Quoted 13 lines
drivers/usb/gadget/f_fastboot.c static void cb_getvar(struct usb_ep *ep, struct · line 341
Quoted 10 lines
drivers/usb/gadget/f_fastboot.c +2
-1-int fastboot_tx_write (const char *buffer,unsigned int buffer_size) +static int fastboot_tx_write (const char *buffer,unsigned int buffer_size) {struct usb_request *in_req = fastboot_func->in_req;int ret;@@ -338,6 +338,7 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) strcpy(response, "OKAY");strsep (&cmd,":" );if (!cmd) { +printf ("%s: missing var\n" , __func__);I'd spell it out completely -- variable -- but I'm not sure if you might need to maintain some kind of compatibility with the fastboot responses here or not.
nope -- done in v4
Thanks, Steve
Quoted 5 lines
[...]
Best regards,
Marek Vasut
v3 cover Implement "fastboot flash" for eMMC
In reply to Steve RaeQuoted 35 lines · Steve Rae
This series implements the "fastboot flash" command for eMMC devices. It supports both raw and sparse images.
NOTES:
- the support for the "fastboot flash" command is enabled withCONFIG_FASTBOOT_FLASH
- the support for eMMC is enabled withCONFIG_FASTBOOT_FLASH_MMC_DEV
- (future) the support for NAND would be enabled withCONFIG_FASTBOOT_FLASH_NAND(???)This has been tested on ARMv7.
Changes in v3:
- remove most references to 'mmc',
which leaves only one mmc specific function:mmc_get_dev()Changes in v2:
- split large function into three
- improved handling of response messages
- additional partition size checking when writing sparse image
- update README.android-fastboot file
- new in v2Steve Rae (4):
usb/gadget: fastboot: add sparse image definitions usb/gadget: fastboot: add eMMC support for flash command usb/gadget: fastboot: add support for flash command usb/gadget: fastboot: minor cleanupREADME | 10 +++ common/Makefile | 5 ++ common/fb_mmc.c | 190 ++++++++++++++++++++++++++++++++++++++++ doc/README.android-fastboot | 5 +- drivers/usb/gadget/f_fastboot.c | 41 ++++++++- include/fb_mmc.h | 8 ++ include/sparse_format.h | 58 ++++++++++++ 7 files changed, 312 insertions(+), 5 deletions(-)
Quoted 4 lines
create mode 100644
common/fb_mmc.c
create mode 100644include/fb_mmc.h
create mode 100644include/sparse_format.h
Marek,
I notice that this patchset is assigned to you:
http://patchwork.ozlabs.org/patch/364705/
Has there been any progress?
Thanks, Steve
v3 cover Implement "fastboot flash" for eMMC
Quoted 43 lines · Steve Rae
Quoted 38 lines · Steve Rae
This series implements the "fastboot flash" command for eMMC devices. It supports both raw and sparse images.
NOTES:
- the support for the "fastboot flash" command is enabled withCONFIG_FASTBOOT_FLASH- the support for eMMC is enabled withCONFIG_FASTBOOT_FLASH_MMC_DEV- (future) the support for NAND would be enabled withCONFIG_FASTBOOT_FLASH_NAND(???)This has been tested on ARMv7.
Changes in v3:
- remove most references to 'mmc',which leaves only one mmc specific function:
mmc_get_dev()Changes in v2:
- split large function into three
- improved handling of response messages
- additional partition size checking when writing sparse image
- update README.android-fastboot file
- new in v2Steve Rae (4):
usb/gadget: fastboot: add sparse image definitions usb/gadget: fastboot: add eMMC support for flash command usb/gadget: fastboot: add support for flash command usb/gadget: fastboot: minor cleanupREADME | 10 +++ common/Makefile | 5 ++ common/fb_mmc.c | 190 ++++++++++++++++++++++++++++++++++++++++ doc/README.android-fastboot | 5 +- drivers/usb/gadget/f_fastboot.c | 41 ++++++++- include/fb_mmc.h | 8 ++ include/sparse_format.h | 58 ++++++++++++ 7 files changed, 312 insertions(+), 5 deletions(-)Quoted 3 lines
create mode 100644
common/fb_mmc.c
create mode 100644include/fb_mmc.h
create mode 100644include/sparse_format.h
Quoted 4 lines
Marek,
I notice that this patchset is assigned to you:http://patchwork.ozlabs.org/patch/364705/Has there been any progress?
Hm, dang. I merrily ignored this patchset in the hope Lukasz will pick it up, but he was not in the loop. Sorry about delaying this so much. Given Lukasz is now on a vacation, I'll take a look later today.
Quoted 1 line
Thanks, Steve
Best regards,
Marek Vasut
v3 cover Implement "fastboot flash" for eMMC
Quoted 49 lines · Marek Vasut
Quoted 43 lines · Steve Rae
Quoted 38 lines · Steve Rae
This series implements the "fastboot flash" command for eMMC devices. It supports both raw and sparse images.
NOTES:
- the support for the "fastboot flash" command is enabled withCONFIG_FASTBOOT_FLASH- the support for eMMC is enabled withCONFIG_FASTBOOT_FLASH_MMC_DEV- (future) the support for NAND would be enabled withCONFIG_FASTBOOT_FLASH_NAND(???)This has been tested on ARMv7.
Changes in v3:
- remove most references to 'mmc',which leaves only one mmc specific function:
mmc_get_dev()Changes in v2:
- split large function into three
- improved handling of response messages
- additional partition size checking when writing sparse image
- update README.android-fastboot file
- new in v2Steve Rae (4):
usb/gadget: fastboot: add sparse image definitions usb/gadget: fastboot: add eMMC support for flash command usb/gadget: fastboot: add support for flash command usb/gadget: fastboot: minor cleanupREADME | 10 +++ common/Makefile | 5 ++ common/fb_mmc.c | 190 ++++++++++++++++++++++++++++++++++++++++ doc/README.android-fastboot | 5 +- drivers/usb/gadget/f_fastboot.c | 41 ++++++++- include/fb_mmc.h | 8 ++ include/sparse_format.h | 58 ++++++++++++ 7 files changed, 312 insertions(+), 5 deletions(-)Quoted 3 lines
create mode 100644
common/fb_mmc.c
create mode 100644include/fb_mmc.h
create mode 100644include/sparse_format.hQuoted 4 lines
Marek,
I notice that this patchset is assigned to you:http://patchwork.ozlabs.org/patch/364705/Has there been any progress?
Quoted 9 lines
Hm, dang. I merrily ignored this patchset in the hope Lukasz will pick it up, but he was not in the loop. Sorry about delaying this so much. Given Lukasz is now on a vacation, I'll take a look later today.
Quoted 1 line
Thanks, Steve
Best regards,
Marek Vasut
Thanks -- appreciate that....
And I'll work through your reviews; but I'll comment on one right now.....