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

Steve Rae
2014-08-07T00:28:13Z

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