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

[Steve Rae <srae@broadcom.com>]

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