From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Wed, 6 Aug 2014 17:28:13 -0700 Subject: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command In-Reply-To: <201408070213.35459.marex@denx.de> References: <1403813604-31685-1-git-send-email-srae@broadcom.com> <201407310337.50805.marex@denx.de> <53E2BEB6.8030604@broadcom.com> <201408070213.35459.marex@denx.de> Message-ID: <53E2C81D.6050003@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +/* 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