Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
On Thursday, August 07, 2014 at 02:28:13 AM, Steve Rae wrote:
On 14-08-06 05:13 PM, Marek Vasut wrote:
On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote:
On 14-07-30 06:37 PM, Marek Vasut wrote:
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
+
+#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.
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
OK, let's leave it this way for now.
+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.
true
, 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.
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.
and the fact that CONFIG_SYS_VSNPRINTF is 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.
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 ?
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 !
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
Hi Marek, [snip] 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 ? 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). 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
On Thursday, August 07, 2014 at 03:28:14 PM, Pantelis Antoniou wrote: Hi Marek, [snip] 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 ? 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
On 14-08-07 06:23 AM, Marek Vasut wrote:
On Thursday, August 07, 2014 at 02:28:13 AM, Steve Rae wrote:
On 14-08-06 05:13 PM, Marek Vasut wrote:
On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote:
On 14-07-30 06:37 PM, Marek Vasut wrote:
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
+
+#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.
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
OK, let's leave it this way for now.
+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.
true
, 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.
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.
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?
and the fact that CONFIG_SYS_VSNPRINTF is 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.
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 ?
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
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 !
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [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: [...] 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. 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. 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? 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 ? and the fact that CONFIG_SYS_VSNPRINTF is 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. 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 ? 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. 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
On 14-07-30 06:37 PM, Marek Vasut wrote:
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
+
+#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.
+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
+}
+
+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,
+ __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 .
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
+ return;
+ }
[...]
+static void write_raw_image(block_dev_desc_t *dev_desc, disk_partition_t
*info, +const char *part_name, void *buffer,
+ 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__,
+ 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.
done in v4 - Thanks!
+ 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote:
On 14-07-30 06:37 PM, Marek Vasut wrote:
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
+
+#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.
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 ?
+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.
, and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
and the fact that CONFIG_SYS_VSNPRINTF is 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.
+}
+
+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,
+ __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 .
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
OK, thanks!
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
On 14-08-06 05:13 PM, Marek Vasut wrote:
On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote:
On 14-07-30 06:37 PM, Marek Vasut wrote:
On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
+
+#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.
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
+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.
true
, 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.
and the fact that CONFIG_SYS_VSNPRINTF is 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.
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?
+}
+
+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,
+ __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 .
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
OK, thanks!
Thanks, Steve
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [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:
[...]
+
+#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.
+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.
+}
+
+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,
+__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 .
+ return;
+ }
[...]
+static void write_raw_image(block_dev_desc_t *dev_desc, disk_partition_t
*info, + const char *part_name, void *buffer,
+ 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__,
+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.
+ 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);
+}
[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command
- add support for 'fastboot flash' command for eMMC devices
Signed-off-by: Steve Rae s...@broadcom.com
---
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
diff --git a/common/Makefile b/common/Makefile
index de5cce8..daebe39 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -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)
diff --git a/common/fb_mmc.c b/common/fb_mmc.c
new file mode 100644
index 000..9163d8c
--- /dev/null
+++ b/common/fb_mmc.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright TODO
+ *
+ * SPDX-License-Identifier:GPL-2.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;
+
+static void fastboot_resp(const char *s)
+{
+ strncpy(response_str, s, RESPONSE_LEN);
+ response_str[RESPONSE_LEN - 1] = '\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,
+ __func__, blk_sz);
+ fastboot_resp(FAILsparse image block size issue);
+ return;
+ }
+
+ if ((le32_to_cpu(s_header-total_blks) * blk_sz)
+ (info-size * info-blksz)) {
+ printf(%s: Sparse image is too large for the partition\n,
+ __func__);
+ fastboot_resp(FAILsparse image is too large);
+ return;
+ }
+
+ printf(Flashing Sparse Image\n);
+
+ remaining_chunks = le32_to_cpu(s_header-total_chunks);
+ c_header = (chunk_header_t *)(buffer +
+ le16_to_cpu(s_header-file_hdr_sz));
+ blk = info-start;
+ while (remaining_chunks) {
+ blkcnt =
+ (le32_to_cpu(c_header-chunk_sz) * blk_sz) / info-blksz;
+
+ switch (le16_to_cpu(c_header-chunk_type)) {
+ case CHUNK_TYPE_RAW:
+ buf = (void *)c_header +
+ le16_to_cpu(s_header-chunk_hdr_sz);
+
+ if (blk + blkcnt info-start + info-size) {
+ printf(
+ %s: Request would exceed partition
size!\n,
+ __func__);
+ fastboot_resp(
+ FAILRequest would exceed partition size!);
+ return;
+ }
+
+ blks = dev_desc-block_write(dev_desc-dev, blk, blkcnt,
+ buf);
+ if (blks != blkcnt) {
+ printf(%s: Write failed LBAFU \n,
+ __func__, blks);
+ fastboot_resp(FAILwrite failure);
+ return;
+ }
+
+ bytes_written += blkcnt * info-blksz;
+ break;
+
+ case CHUNK_TYPE_FILL:
+ case CHUNK_TYPE_DONT_CARE:
+ case CHUNK_TYPE_CRC32:
+ /* do nothing */
+ break;
+
+ default:
+
