[U-Boot] [PATCH v2 1/3] image: fix bootm failure for FIT image
Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced a bug for booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.
Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
Reported-by: York Sun yorksun@freescale.com Signed-off-by: Bryan Wu pengw@nvidia.com --- common/bootm.c | 9 +++++---- common/cmd_pxe.c | 9 +++++++++ common/image.c | 19 ++++++++++--------- include/image.h | 6 ++++++ 4 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/common/bootm.c b/common/bootm.c index 76d811c..85b71ba 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -731,7 +731,12 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, int os_noffset; #endif
+#if defined(CONFIG_FIT) + img_addr = genimg_get_kernel_addr(argv[0], &fit_uname_config, + &fit_uname_kernel); +#else img_addr = genimg_get_kernel_addr(argv[0]); +#endif
bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
@@ -788,10 +793,6 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, #endif #if defined(CONFIG_FIT) case IMAGE_FORMAT_FIT: - if (!fit_parse_conf(argv[0], load_addr, &img_addr, - &fit_uname_config)) - fit_parse_subimage(argv[0], load_addr, &img_addr, - &fit_uname_kernel); os_noffset = fit_image_load(images, img_addr, &fit_uname_kernel, &fit_uname_config, IH_ARCH_DEFAULT, IH_TYPE_KERNEL, diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index c816339..9a2c370 100644 --- a/common/cmd_pxe.c +++ b/common/cmd_pxe.c @@ -612,6 +612,10 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label) int len = 0; ulong kernel_addr; void *buf; +#if defined(CONFIG_FIT) + const char *fit_uname_config = NULL; + const char *fit_uname_kernel = NULL; +#endif
label_print(label);
@@ -774,7 +778,12 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label) if (bootm_argv[3]) bootm_argc = 4;
+#if defined(CONFIG_FIT) + kernel_addr = genimg_get_kernel_addr(bootm_argv[1], &fit_uname_config, + &fit_uname_kernel); +#else kernel_addr = genimg_get_kernel_addr(bootm_argv[1]); +#endif buf = map_sysmem(kernel_addr, 0); /* Try bootm for legacy and FIT format image */ if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID) diff --git a/common/image.c b/common/image.c index a2999c0..ea980cb 100644 --- a/common/image.c +++ b/common/image.c @@ -652,13 +652,14 @@ int genimg_get_comp_id(const char *name) * returns: * kernel start address */ -ulong genimg_get_kernel_addr(char * const img_addr) -{ #if defined(CONFIG_FIT) - const char *fit_uname_config = NULL; - const char *fit_uname_kernel = NULL; +ulong genimg_get_kernel_addr(char * const img_addr, + const char **fit_uname_config, + const char **fit_uname_kernel) +#else +ulong genimg_get_kernel_addr(char * const img_addr) #endif - +{ ulong kernel_addr;
/* find out kernel image address */ @@ -668,13 +669,13 @@ ulong genimg_get_kernel_addr(char * const img_addr) load_addr); #if defined(CONFIG_FIT) } else if (fit_parse_conf(img_addr, load_addr, &kernel_addr, - &fit_uname_config)) { + fit_uname_config)) { debug("* kernel: config '%s' from image at 0x%08lx\n", - fit_uname_config, kernel_addr); + *fit_uname_config, kernel_addr); } else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr, - &fit_uname_kernel)) { + fit_uname_kernel)) { debug("* kernel: subimage '%s' from image at 0x%08lx\n", - fit_uname_kernel, kernel_addr); + *fit_uname_kernel, kernel_addr); #endif } else { kernel_addr = simple_strtoul(img_addr, NULL, 16); diff --git a/include/image.h b/include/image.h index ca2fe86..a47c146 100644 --- a/include/image.h +++ b/include/image.h @@ -424,7 +424,13 @@ enum fit_load_op { #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ #define IMAGE_FORMAT_ANDROID 0x03 /* Android boot image */
+#if defined(CONFIG_FIT) +ulong genimg_get_kernel_addr(char * const img_addr, + const char **fit_uname_config, + const char **fit_uname_kernel); +#else ulong genimg_get_kernel_addr(char * const img_addr); +#endif int genimg_get_format(const void *img_addr); int genimg_has_config(bootm_headers_t *images); ulong genimg_get_image(ulong img_addr);
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com --- common/image.c | 186 --------------------------------------------------- include/image.h | 203 +++++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 195 insertions(+), 194 deletions(-)
diff --git a/common/image.c b/common/image.c index ea980cb..faccf88 100644 --- a/common/image.c +++ b/common/image.c @@ -181,19 +181,6 @@ int image_check_dcrc(const image_header_t *hdr) return (dcrc == image_get_dcrc(hdr)); }
-/** - * image_multi_count - get component (sub-image) count - * @hdr: pointer to the header of the multi component image - * - * image_multi_count() returns number of components in a multi - * component image. - * - * Note: no checking of the image type is done, caller must pass - * a valid multi component image. - * - * returns: - * number of components - */ ulong image_multi_count(const image_header_t *hdr) { ulong i, count = 0; @@ -210,23 +197,6 @@ ulong image_multi_count(const image_header_t *hdr) return count; }
-/** - * image_multi_getimg - get component data address and size - * @hdr: pointer to the header of the multi component image - * @idx: index of the requested component - * @data: pointer to a ulong variable, will hold component data address - * @len: pointer to a ulong variable, will hold component size - * - * image_multi_getimg() returns size and data address for the requested - * component in a multi component image. - * - * Note: no checking of the image type is done, caller must pass - * a valid multi component image. - * - * returns: - * data address and size of the component, if idx is valid - * 0 in data and len, if idx is out of range - */ void image_multi_getimg(const image_header_t *hdr, ulong idx, ulong *data, ulong *len) { @@ -275,18 +245,6 @@ static void image_print_type(const image_header_t *hdr) printf("%s %s %s (%s)\n", arch, os, type, comp); }
-/** - * image_print_contents - prints out the contents of the legacy format image - * @ptr: pointer to the legacy format image header - * @p: pointer to prefix string - * - * image_print_contents() formats a multi line legacy image contents description. - * The routine prints out all header fields followed by the size/offset data - * for MULTI/SCRIPT images. - * - * returns: - * no returned results - */ void image_print_contents(const void *ptr) { const image_header_t *hdr = (const image_header_t *)ptr; @@ -524,20 +482,6 @@ void genimg_print_time(time_t timestamp) } #endif
-/** - * get_table_entry_name - translate entry id to long name - * @table: pointer to a translation table for entries of a specific type - * @msg: message to be returned when translation fails - * @id: entry id to be translated - * - * get_table_entry_name() will go over translation table trying to find - * entry that matches given id. If matching entry is found, its long - * name is returned to the caller. - * - * returns: - * long entry name if translation succeeds - * msg otherwise - */ char *get_table_entry_name(const table_entry_t *table, char *msg, int id) { for (; table->id >= 0; ++table) { @@ -573,20 +517,6 @@ const char *genimg_get_comp_name(uint8_t comp) comp)); }
-/** - * get_table_entry_id - translate short entry name to id - * @table: pointer to a translation table for entries of a specific type - * @table_name: to be used in case of error - * @name: entry short name to be translated - * - * get_table_entry_id() will go over translation table trying to find - * entry that matches given short name. If matching entry is found, - * its id returned to the caller. - * - * returns: - * entry id if translation succeeds - * -1 otherwise - */ int get_table_entry_id(const table_entry_t *table, const char *table_name, const char *name) { @@ -642,16 +572,6 @@ int genimg_get_comp_id(const char *name) }
#ifndef USE_HOSTCC -/** - * genimg_get_kernel_addr - get the real kernel address - * @img_addr: a string might contain real image address - * - * genimg_get_kernel_addr() get the real kernel start address from a string - * which is normally the first argv of bootm/bootz - * - * returns: - * kernel start address - */ #if defined(CONFIG_FIT) ulong genimg_get_kernel_addr(char * const img_addr, const char **fit_uname_config, @@ -686,20 +606,6 @@ ulong genimg_get_kernel_addr(char * const img_addr) return kernel_addr; }
-/** - * genimg_get_format - get image format type - * @img_addr: image start address - * - * genimg_get_format() checks whether provided address points to a valid - * legacy or FIT image. - * - * New uImage format and FDT blob are based on a libfdt. FDT blob - * may be passed directly or embedded in a FIT image. In both situations - * genimg_get_format() must be able to dectect libfdt header. - * - * returns: - * image format type or IMAGE_FORMAT_INVALID if no image is present - */ int genimg_get_format(const void *img_addr) { #if defined(CONFIG_IMAGE_FORMAT_LEGACY) @@ -721,16 +627,6 @@ int genimg_get_format(const void *img_addr) return IMAGE_FORMAT_INVALID; }
-/** - * genimg_get_image - get image from special storage (if necessary) - * @img_addr: image start address - * - * genimg_get_image() checks if provided image start adddress is located - * in a dataflash storage. If so, image is moved to a system RAM memory. - * - * returns: - * image start address after possible relocation from special storage - */ ulong genimg_get_image(ulong img_addr) { ulong ram_addr = img_addr; @@ -796,17 +692,6 @@ ulong genimg_get_image(ulong img_addr) return ram_addr; }
-/** - * fit_has_config - check if there is a valid FIT configuration - * @images: pointer to the bootm command headers structure - * - * fit_has_config() checks if there is a FIT configuration in use - * (if FTI support is present). - * - * returns: - * 0, no FIT support or no configuration found - * 1, configuration found - */ int genimg_has_config(bootm_headers_t *images) { #if defined(CONFIG_FIT) @@ -816,28 +701,6 @@ int genimg_has_config(bootm_headers_t *images) return 0; }
-/** - * boot_get_ramdisk - main ramdisk handling routine - * @argc: command argument count - * @argv: command argument list - * @images: pointer to the bootm images structure - * @arch: expected ramdisk architecture - * @rd_start: pointer to a ulong variable, will hold ramdisk start address - * @rd_end: pointer to a ulong variable, will hold ramdisk end - * - * boot_get_ramdisk() is responsible for finding a valid ramdisk image. - * Curently supported are the following ramdisk sources: - * - multicomponent kernel/ramdisk image, - * - commandline provided address of decicated ramdisk image. - * - * returns: - * 0, if ramdisk image was found and valid, or skiped - * rd_start and rd_end are set to ramdisk start/end addresses if - * ramdisk image is found and valid - * - * 1, if ramdisk image is found but corrupted, or invalid - * rd_start and rd_end are set to 0 if no ramdisk exists - */ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, ulong *rd_start, ulong *rd_end) { @@ -1020,27 +883,6 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, }
#ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH -/** - * boot_ramdisk_high - relocate init ramdisk - * @lmb: pointer to lmb handle, will be used for memory mgmt - * @rd_data: ramdisk data start address - * @rd_len: ramdisk data length - * @initrd_start: pointer to a ulong variable, will hold final init ramdisk - * start address (after possible relocation) - * @initrd_end: pointer to a ulong variable, will hold final init ramdisk - * end address (after possible relocation) - * - * boot_ramdisk_high() takes a relocation hint from "initrd_high" environment - * variable and if requested ramdisk data is moved to a specified location. - * - * Initrd_start and initrd_end are set to final (after relocation) ramdisk - * start/end addresses if ramdisk image start and len were provided, - * otherwise set initrd_start and initrd_end set to zeros. - * - * returns: - * 0 - success - * -1 - failure - */ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, ulong *initrd_start, ulong *initrd_end) { @@ -1121,21 +963,6 @@ error: #endif /* CONFIG_SYS_BOOT_RAMDISK_HIGH */
#ifdef CONFIG_SYS_BOOT_GET_CMDLINE -/** - * boot_get_cmdline - allocate and initialize kernel cmdline - * @lmb: pointer to lmb handle, will be used for memory mgmt - * @cmd_start: pointer to a ulong variable, will hold cmdline start - * @cmd_end: pointer to a ulong variable, will hold cmdline end - * - * boot_get_cmdline() allocates space for kernel command line below - * BOOTMAPSZ + getenv_bootm_low() address. If "bootargs" U-boot environemnt - * variable is present its contents is copied to allocated kernel - * command line. - * - * returns: - * 0 - success - * -1 - failure - */ int boot_get_cmdline(struct lmb *lmb, ulong *cmd_start, ulong *cmd_end) { char *cmdline; @@ -1162,19 +989,6 @@ int boot_get_cmdline(struct lmb *lmb, ulong *cmd_start, ulong *cmd_end) #endif /* CONFIG_SYS_BOOT_GET_CMDLINE */
#ifdef CONFIG_SYS_BOOT_GET_KBD -/** - * boot_get_kbd - allocate and initialize kernel copy of board info - * @lmb: pointer to lmb handle, will be used for memory mgmt - * @kbd: double pointer to board info data - * - * boot_get_kbd() allocates space for kernel copy of board info data below - * BOOTMAPSZ + getenv_bootm_low() address and kernel board info is initialized - * with the current u-boot board info data. - * - * returns: - * 0 - success - * -1 - failure - */ int boot_get_kbd(struct lmb *lmb, bd_t **kbd) { *kbd = (bd_t *)(ulong)lmb_alloc_base(lmb, sizeof(bd_t), 0xf, diff --git a/include/image.h b/include/image.h index a47c146..b95b4d5 100644 --- a/include/image.h +++ b/include/image.h @@ -376,17 +376,36 @@ typedef struct table_entry { char *lname; /* long (output) name to print for messages */ } table_entry_t;
-/* - * get_table_entry_id() scans the translation table trying to find an - * entry that matches the given short name. If a matching entry is - * found, it's id is returned to the caller. +/** + * get_table_entry_id - translate short entry name to id + * @table: pointer to a translation table for entries of a specific type + * @table_name: to be used in case of error + * @name: entry short name to be translated + * + * get_table_entry_id() will go over translation table trying to find + * entry that matches given short name. If matching entry is found, + * its id returned to the caller. + * + * returns: + * entry id if translation succeeds + * -1 otherwise */ int get_table_entry_id(const table_entry_t *table, const char *table_name, const char *name); -/* - * get_table_entry_name() scans the translation table trying to find - * an entry that matches the given id. If a matching entry is found, - * its long name is returned to the caller. + +/** + * get_table_entry_name - translate entry id to long name + * @table: pointer to a translation table for entries of a specific type + * @msg: message to be returned when translation fails + * @id: entry id to be translated + * + * get_table_entry_name() will go over translation table trying to find + * entry that matches given id. If matching entry is found, its long + * name is returned to the caller. + * + * returns: + * long entry name if translation succeeds + * msg otherwise */ char *get_table_entry_name(const table_entry_t *table, char *msg, int id);
@@ -424,6 +443,20 @@ enum fit_load_op { #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ #define IMAGE_FORMAT_ANDROID 0x03 /* Android boot image */
+/** + * genimg_get_kernel_addr - get the real kernel address + * @img_addr: a string might contain real image address + * @fit_uname_config: double pointer to a char, will hold pointer to a + * configuration unit name + * @fit_uname_kernel: double pointer to a char, will hold pointer to a + * subimage name + * + * genimg_get_kernel_addr() get the real kernel start address from a string + * which is normally the first argv of bootm/bootz + * + * returns: + * kernel start address + */ #if defined(CONFIG_FIT) ulong genimg_get_kernel_addr(char * const img_addr, const char **fit_uname_config, @@ -431,10 +464,70 @@ ulong genimg_get_kernel_addr(char * const img_addr, #else ulong genimg_get_kernel_addr(char * const img_addr); #endif + +/** + * genimg_get_format - get image format type + * @img_addr: image start address + * + * genimg_get_format() checks whether provided address points to a valid + * legacy or FIT image. + * + * New uImage format and FDT blob are based on a libfdt. FDT blob + * may be passed directly or embedded in a FIT image. In both situations + * genimg_get_format() must be able to dectect libfdt header. + * + * returns: + * image format type or IMAGE_FORMAT_INVALID if no image is present + */ int genimg_get_format(const void *img_addr); + +/** + * fit_has_config - check if there is a valid FIT configuration + * @images: pointer to the bootm command headers structure + * + * fit_has_config() checks if there is a FIT configuration in use + * (if FTI support is present). + * + * returns: + * 0, no FIT support or no configuration found + * 1, configuration found + */ int genimg_has_config(bootm_headers_t *images); + +/** + * genimg_get_image - get image from special storage (if necessary) + * @img_addr: image start address + * + * genimg_get_image() checks if provided image start adddress is located + * in a dataflash storage. If so, image is moved to a system RAM memory. + * + * returns: + * image start address after possible relocation from special storage + */ ulong genimg_get_image(ulong img_addr);
+/** + * boot_get_ramdisk - main ramdisk handling routine + * @argc: command argument count + * @argv: command argument list + * @images: pointer to the bootm images structure + * @arch: expected ramdisk architecture + * @rd_start: pointer to a ulong variable, will hold ramdisk start address + * @rd_end: pointer to a ulong variable, will hold ramdisk end + * + * boot_get_ramdisk() is responsible for finding a valid ramdisk image. + * Curently supported are the following ramdisk sources: + * - multicomponent kernel/ramdisk image, + * - commandline provided address of decicated ramdisk image. + * + * returns: + * 0, if ramdisk image was found and valid, or skiped + * rd_start and rd_end are set to ramdisk start/end addresses if + * ramdisk image is found and valid + * + * 1, if ramdisk image is found but corrupted, or invalid + * rd_start and rd_end are set to 0 if no ramdisk exists + */ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, ulong *rd_start, ulong *rd_end); #endif @@ -511,10 +604,61 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob); int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size);
+/** + * boot_ramdisk_high - relocate init ramdisk + * @lmb: pointer to lmb handle, will be used for memory mgmt + * @rd_data: ramdisk data start address + * @rd_len: ramdisk data length + * @initrd_start: pointer to a ulong variable, will hold final init ramdisk + * start address (after possible relocation) + * @initrd_end: pointer to a ulong variable, will hold final init ramdisk + * end address (after possible relocation) + * + * boot_ramdisk_high() takes a relocation hint from "initrd_high" environment + * variable and if requested ramdisk data is moved to a specified location. + * + * Initrd_start and initrd_end are set to final (after relocation) ramdisk + * start/end addresses if ramdisk image start and len were provided, + * otherwise set initrd_start and initrd_end set to zeros. + * + * returns: + * 0 - success + * -1 - failure + */ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, ulong *initrd_start, ulong *initrd_end); + +/** + * boot_get_cmdline - allocate and initialize kernel cmdline + * @lmb: pointer to lmb handle, will be used for memory mgmt + * @cmd_start: pointer to a ulong variable, will hold cmdline start + * @cmd_end: pointer to a ulong variable, will hold cmdline end + * + * boot_get_cmdline() allocates space for kernel command line below + * BOOTMAPSZ + getenv_bootm_low() address. If "bootargs" U-boot environemnt + * variable is present its contents is copied to allocated kernel + * command line. + * + * returns: + * 0 - success + * -1 - failure + */ int boot_get_cmdline(struct lmb *lmb, ulong *cmd_start, ulong *cmd_end); + #ifdef CONFIG_SYS_BOOT_GET_KBD +/** + * boot_get_kbd - allocate and initialize kernel copy of board info + * @lmb: pointer to lmb handle, will be used for memory mgmt + * @kbd: double pointer to board info data + * + * boot_get_kbd() allocates space for kernel copy of board info data below + * BOOTMAPSZ + getenv_bootm_low() address and kernel board info is initialized + * with the current u-boot board info data. + * + * returns: + * 0 - success + * -1 - failure + */ int boot_get_kbd(struct lmb *lmb, bd_t **kbd); #endif /* CONFIG_SYS_BOOT_GET_KBD */ #endif /* !USE_HOSTCC */ @@ -639,10 +783,53 @@ static inline int image_check_os(const image_header_t *hdr, uint8_t os) return (image_get_os(hdr) == os); }
+/** + * image_multi_count - get component (sub-image) count + * @hdr: pointer to the header of the multi component image + * + * image_multi_count() returns number of components in a multi + * component image. + * + * Note: no checking of the image type is done, caller must pass + * a valid multi component image. + * + * returns: + * number of components + */ ulong image_multi_count(const image_header_t *hdr); + +/** + * image_multi_getimg - get component data address and size + * @hdr: pointer to the header of the multi component image + * @idx: index of the requested component + * @data: pointer to a ulong variable, will hold component data address + * @len: pointer to a ulong variable, will hold component size + * + * image_multi_getimg() returns size and data address for the requested + * component in a multi component image. + * + * Note: no checking of the image type is done, caller must pass + * a valid multi component image. + * + * returns: + * data address and size of the component, if idx is valid + * 0 in data and len, if idx is out of range + */ void image_multi_getimg(const image_header_t *hdr, ulong idx, ulong *data, ulong *len);
+/** + * image_print_contents - prints out the contents of the legacy format image + * @ptr: pointer to the legacy format image header + * @p: pointer to prefix string + * + * image_print_contents() formats a multi line legacy image contents description. + * The routine prints out all header fields followed by the size/offset data + * for MULTI/SCRIPT images. + * + * returns: + * no returned results + */ void image_print_contents(const void *hdr);
#ifndef USE_HOSTCC
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
Regards, Jeroen
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
-Bryan
On 08/15/2014 03:07 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
I prefer to see comments near the implementations.
York
On Fri, Aug 15, 2014 at 3:10 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:07 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
I prefer to see comments near the implementations.
Then we need another patch to move those comments from header file to C file.
On 08/15/2014 04:11 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:10 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:07 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
I prefer to see comments near the implementations.
Then we need another patch to move those comments from header file to C file.
Well, I wouldn't do anything just yet. Simon and York need to sort out an agreement first, so we don't just keep writing patches that move stuff back and forth.
On 08/15/2014 03:14 PM, Stephen Warren wrote:
On 08/15/2014 04:11 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:10 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:07 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
I prefer to see comments near the implementations.
Then we need another patch to move those comments from header file to C file.
Well, I wouldn't do anything just yet. Simon and York need to sort out an agreement first, so we don't just keep writing patches that move stuff back and forth.
I don't have strong opinion for this case. I would prefer to have the comments near the implementation. I understand current code may have mixed style here and there. Unless it is a clean up effort, I wouldn't add such patch to a set with function change or bug fix.
York
On Fri, Aug 15, 2014 at 3:25 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:14 PM, Stephen Warren wrote:
On 08/15/2014 04:11 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:10 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:07 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Bryan,
On 15-08-14 22:55, Bryan Wu wrote: > > Several functions comments are C file with function definition, they > should be moved to header file with function declaration. > > Also update genimg_get_kernel_addr() comments for CONFIG_FIT case. > > Signed-off-by: Bryan Wu pengw@nvidia.com >
Why _should_ this be done. In general I would not do it to keep comment and implementation close to each other. (In the hope they actually match). Doxygen and likely the kernel doc thing can pick this up. The only reason I can think of this being useful is for proprietary code with a public api, but this is not applicable for u-boot.
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
I prefer to see comments near the implementations.
Then we need another patch to move those comments from header file to C file.
Well, I wouldn't do anything just yet. Simon and York need to sort out an agreement first, so we don't just keep writing patches that move stuff back and forth.
I don't have strong opinion for this case. I would prefer to have the comments near the implementation. I understand current code may have mixed style here and there. Unless it is a clean up effort, I wouldn't add such patch to a set with function change or bug fix.
Sure, let's sort out the style firstly and patch is easy to move stuff. I will folder the comment change of my genimg_get_kernel_addr() to Patch 1/3 then. and let's ignore this one now.
-Bryan
Hi,
On 15 August 2014 16:56, Bryan Wu cooloney@gmail.com wrote:
On Fri, Aug 15, 2014 at 3:25 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:14 PM, Stephen Warren wrote:
On 08/15/2014 04:11 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:10 PM, York Sun yorksun@freescale.com wrote:
On 08/15/2014 03:07 PM, Bryan Wu wrote:
On Fri, Aug 15, 2014 at 3:01 PM, Jeroen Hofstee dasuboot@myspectrum.nl wrote: > Hello Bryan, > > > On 15-08-14 22:55, Bryan Wu wrote: >> >> Several functions comments are C file with function definition, they >> should be moved to header file with function declaration. >> >> Also update genimg_get_kernel_addr() comments for CONFIG_FIT case. >> >> Signed-off-by: Bryan Wu pengw@nvidia.com >> > > Why _should_ this be done. In general I would not do it > to keep comment and implementation close to each other. > (In the hope they actually match). Doxygen and likely the > kernel doc thing can pick this up. The only reason I can > think of this being useful is for proprietary code with a public > api, but this is not applicable for u-boot. >
I was asked to do that by Simon and right now in image.c and image.h it's quite mix. Some of them are in C code with implementation others are in header file with declaration.
I was confused by this in u-boot, although in kernel we put comments in C code with implementation.
I prefer to see comments near the implementations.
Then we need another patch to move those comments from header file to C file.
Well, I wouldn't do anything just yet. Simon and York need to sort out an agreement first, so we don't just keep writing patches that move stuff back and forth.
I don't have strong opinion for this case. I would prefer to have the comments near the implementation. I understand current code may have mixed style here and there. Unless it is a clean up effort, I wouldn't add such patch to a set with function change or bug fix.
Sure, let's sort out the style firstly and patch is easy to move stuff. I will folder the comment change of my genimg_get_kernel_addr() to Patch 1/3 then. and let's ignore this one now.
My point is that the implementation is just that, and shouldn't need to be consulting for people wanting to call the API. By putting the function comments in the header you can read through the API in one place. As we add more comments into header files, it should be possible to leave the implementation file as a 'detail', only for those digging deeply.
Static functions should still be commented in the C files, since they don't form part of the API.
Regards, Simon
On Fri, Aug 15, 2014 at 01:55:27PM -0700, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Applied to u-boot/master, thanks!
On Sat, Aug 23, 2014 at 08:42:51AM -0400, Tom Rini wrote:
On Fri, Aug 15, 2014 at 01:55:27PM -0700, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Applied to u-boot/master, thanks!
OK, that's not true. I had intended to I believe but git am --skip'd over and didn't manually fix it up, so it's not there yet.
Hi Tom,
On 23 August 2014 11:48, Tom Rini trini@ti.com wrote:
On Sat, Aug 23, 2014 at 08:42:51AM -0400, Tom Rini wrote:
On Fri, Aug 15, 2014 at 01:55:27PM -0700, Bryan Wu wrote:
Several functions comments are C file with function definition, they should be moved to header file with function declaration.
Also update genimg_get_kernel_addr() comments for CONFIG_FIT case.
Signed-off-by: Bryan Wu pengw@nvidia.com
Applied to u-boot/master, thanks!
OK, that's not true. I had intended to I believe but git am --skip'd over and didn't manually fix it up, so it's not there yet.
Did this get applied?
Regards, Simon
Hi.
I vote for comments near the implementation.
I have been digging into the driver model code recently, but I have to admit it is unreadable because most of comments are placed in its header files. (and I am planning to send a patch to move comments to C sources.)
I really want to know "what does this function do?" and "How is it used?" things before I start to read the detailed implementation.
If they are written separately, I need to open two windows of my editor, one for reading the comments in a header file, the other for reading the implementation in a C source. I am really unhappy about that.
I guess people often use tag jump utilities. (I like GNU Global, someone may use ctags/etags, cscope, etc. I don't know..)
Such utilities allow us to jump over to the implementation place. If comments are not there, we have to look for comments by hand.
I think we should keep in our mind this: source files are much more read than written. I believe we put the readers' benefits at the top priority.
Hi Masahiro,
On Sat, 20 Sep 2014 14:34:29 +0900, Masahiro YAMADA yamada.m@jp.panasonic.com wrote:
Hi.
I vote for comments near the implementation.
I have been digging into the driver model code recently, but I have to admit it is unreadable because most of comments are placed in its header files. (and I am planning to send a patch to move comments to C sources.)
I really want to know "what does this function do?" and "How is it used?" things before I start to read the detailed implementation.
If they are written separately, I need to open two windows of my editor, one for reading the comments in a header file, the other for reading the implementation in a C source. I am really unhappy about that.
I guess people often use tag jump utilities. (I like GNU Global, someone may use ctags/etags, cscope, etc. I don't know..)
Such utilities allow us to jump over to the implementation place. If comments are not there, we have to look for comments by hand.
I think we should keep in our mind this: source files are much more read than written. I believe we put the readers' benefits at the top priority.
The way I comment my own code (i.e., when not trying to adhere to coding rules/guidelines, or to the custom in existing code) is that I put comments in both places, declaration and definition, for two different purposes.
I put a block comment before the declaration to tell readers how to *use* the function -- what the arguments mean, what the result shall be, what particular conditions should be met on call, etc.
That's for people who *use* my code.
I put a block comment before the definition to tell readers how the function does its job, similar to a detailed design description but without the algorithm, because I consider the source code to *be* the detailed design algorithm.
That's for people who need to *change* my code.
(Then I put comments in the body code either to break it into identified steps, or to indicate things that do not derive from the design, e.g. compiler or linker bug workarounds, etc. but that's a bit beyond the point.)
As I said, that's /my/ way of writing /my/ code, my .02 EUR given just to point out that we don't necessarily have to decide to put comments in header *xor* body.
Amicalement,
Hi Masahiro,
On 19 September 2014 23:34, Masahiro YAMADA yamada.m@jp.panasonic.com wrote:
Hi.
I vote for comments near the implementation.
I have been digging into the driver model code recently, but I have to admit it is unreadable because most of comments are placed in its header files. (and I am planning to send a patch to move comments to C sources.)
I really want to know "what does this function do?" and "How is it used?" things before I start to read the detailed implementation.
If they are written separately, I need to open two windows of my editor, one for reading the comments in a header file, the other for reading the implementation in a C source. I am really unhappy about that.
I guess people often use tag jump utilities. (I like GNU Global, someone may use ctags/etags, cscope, etc. I don't know..)
Such utilities allow us to jump over to the implementation place. If comments are not there, we have to look for comments by hand.
I think we should keep in our mind this: source files are much more read than written. I believe we put the readers' benefits at the top priority.
Of course the best location depends on what you are using the code for. In your case you are working through the C code I presume, figuring out how everything works.
I think more common is to read through the header files trying to understand the API. How things actually work until the hood can be useful at times, but if the API and docs are good then perhaps you don't need to read much further. For example, I figured out how SPI worked mostly by looking at spi.h, and the probe function. I didn't read all the C code. I think that the C code is a big burden for people who are coming into a subsystem and trying to understand it a bit.
It's nice to have a keystroke to jump between the declaration and implementation of a function.
Actually I think I have said a similar thing to Albert.
I also agree that we don't really need to decide - having comments at all is a bonus in some areas of the code!
Regards, Simon
arg[0] might not be NULL even if argc < 1, so fix this as before.
Signed-off-by: Bryan Wu pengw@nvidia.com --- common/bootm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/bootm.c b/common/bootm.c index 85b71ba..e75a825 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -732,10 +732,11 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, #endif
#if defined(CONFIG_FIT) - img_addr = genimg_get_kernel_addr(argv[0], &fit_uname_config, + img_addr = genimg_get_kernel_addr(argc < 1 ? NULL : argv[0], + &fit_uname_config, &fit_uname_kernel); #else - img_addr = genimg_get_kernel_addr(argv[0]); + img_addr = genimg_get_kernel_addr(argc < 1 ? NULL : argv[0]); #endif
bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
On 15 August 2014 14:55, Bryan Wu cooloney@gmail.com wrote:
Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced a bug for booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.
Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
Reported-by: York Sun yorksun@freescale.com Signed-off-by: Bryan Wu pengw@nvidia.com
I believe this is superseded.
Regards, Simon
participants (8)
-
Albert ARIBAUD -
Bryan Wu -
Jeroen Hofstee -
Masahiro YAMADA -
Simon Glass -
Stephen Warren -
Tom Rini -
York Sun