[U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command

[Marek Vasut]

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!