[PATCH 4/4] cmd: dtimg: Get start and size based on --id and --rev
Sam Protsenko
semen.protsenko at linaro.org
Wed Dec 4 19:56:48 CET 2019
Hi,
On Fri, Nov 29, 2019 at 9:31 PM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> Currently, it is only possible to get the ${index}'s entry of a DTB/DTBO
> image [*]. The "dtimg" command is agnostic on the "id" and "rev" fields
> and is unable to take them as input for a more fine-grained DTB/DTBO
> search/retrieval.
>
> This is a major limitation, as users would like [**] to employ the
> "id"/"rev" fields to e.g. differentiate between DTBs/DTBOs
> associated to multiple HW revisions or several platforms.
>
> Given a sample DTBO image [***], the new options work like below:
>
> => dtimg start 0x48000000 0 --id invalid
> Error: Bad value '--id=invalid'
> => dtimg start 0x48000000 0 --id 0x100
> Error: No #0 entry having id=0x100 && rev=0x0
> => dtimg start 0x48000000 0 --id 00779000
> 0x480006ac
> => dtimg start 0x48000000 1 --id 00779000
> 0x48000b46
> => dtimg start 0x48000000 2 --id 00779000
> Error: No #2 entry having id=0x779000 && rev=0x0
> => dtimg start 0x48000000 99 --id 00779000
> Error: index >= dt_entry_count (99 >= 6)
> => dtimg start 0x48000000 0
> 0x480000e0
> => dtimg start 0x48000000 1
> 0x480000e0
> => dtimg start 0x48000000 2
> 0x480004d0
> => dtimg start 0x48000000 3
> 0x480005be
> => dtimg start 0x48000000 4
> 0x480006ac
> => dtimg start 0x48000000 5
> 0x48000b46
> => dtimg start 0x48000000 6
> Error: index >= dt_entry_count (6 >= 6)
> => dtimg size 0x48000000 0 --id 00779000
> 0x49a
> => dtimg size 0x48000000 1 --id 00779000
> 0x248
>
> [*] https://source.android.com/devices/architecture/dto/partitions
> [**] https://patchwork.ozlabs.org/patch/958594/#2302310
> [***] Sample/dummy DTBO image:
> => dtimg dump 0x48000000
> dt_table_header:
> magic = d7b7ab1e
> total_size = 3470
> header_size = 32
> dt_entry_size = 32
> dt_entry_count = 6
> dt_entries_offset = 32
> page_size = 4096
> version = 0
> dt_table_entry[0]:
> dt_size = 1008
> dt_offset = 224
> id = 0b779530
> rev = 00000000
> custom[0] = 00000000
> custom[1] = 00000000
> custom[2] = 00000000
> custom[3] = 00000000
> (FDT)size = 1008
> (FDT)compatible = (unknown)
> dt_table_entry[1]:
> dt_size = 1008
> dt_offset = 224
> id = 0b779520
> rev = 00000000
> custom[0] = 00000000
> custom[1] = 00000000
> custom[2] = 00000000
> custom[3] = 00000000
> (FDT)size = 1008
> (FDT)compatible = (unknown)
> dt_table_entry[2]:
> dt_size = 238
> dt_offset = 1232
> id = 0b779530
> rev = 00000000
> custom[0] = 00000000
> custom[1] = 00000000
> custom[2] = 00000000
> custom[3] = 00000000
> (FDT)size = 238
> (FDT)compatible = (unknown)
> dt_table_entry[3]:
> dt_size = 238
> dt_offset = 1470
> id = 0b779520
> rev = 00000000
> custom[0] = 00000000
> custom[1] = 00000000
> custom[2] = 00000000
> custom[3] = 00000000
> (FDT)size = 238
> (FDT)compatible = (unknown)
> dt_table_entry[4]:
> dt_size = 1178
> dt_offset = 1708
> id = 00779000
> rev = 00000000
> custom[0] = 00000000
> custom[1] = 00000000
> custom[2] = 00000000
> custom[3] = 00000000
> (FDT)size = 1178
> (FDT)compatible = (unknown)
> dt_table_entry[5]:
> dt_size = 584
> dt_offset = 2886
> id = 00779000
> rev = 00000000
> custom[0] = 00000000
> custom[1] = 00000000
> custom[2] = 00000000
> custom[3] = 00000000
> (FDT)size = 584
> (FDT)compatible = (unknown)
>
> Signed-off-by: Eugeniu Rosca <erosca at de.adit-jv.com>
> ---
> cmd/dtimg.c | 81 +++++++++++++++++++++++++++++++++++---
> common/image-android-dt.c | 64 ++++++++++++++++++++++++++++--
> include/image-android-dt.h | 5 ++-
[strong opinion] Can you please split it into two patches? First to
introduce new functionality to common/image-android-dt.c, second one
to use it in dtimg. Atomicity reasons... I understand that we don't
introduce unused code, but it will be used in the same patch series,
though later (in some hypothetical scenario) we would be able to e.g.
revert cmd related changes, if someone else (see 'abootimg') already
uses API from common/image-android-dt.c. Also easier to review this
way.
> 3 files changed, 138 insertions(+), 12 deletions(-)
>
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 5348a4ad46e8..10e909ce551b 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -7,6 +7,7 @@
> #include <env.h>
> #include <image-android-dt.h>
> #include <common.h>
> +#include <linux/ctype.h>
>
> enum cmd_dtimg_info {
> CMD_DTIMG_START = 0,
> @@ -48,6 +49,61 @@ static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
> return CMD_RET_SUCCESS;
> }
>
> +static int dtimg_get_opthex(int *argcp, char * const *argvp[], u32 *valp)
Maybe make it _hex? For naming uniformity reasons, like in env_set_hex().
Also, (but that's only my opinion), don't really like "p" sufficies
for "pointer", I'd avoid using it (didn't see that much in U-Boot or
in kernel sources). For my taste, it only clutters the name (similar
to Hungarian notation).
Also, maybe it would be prettier to change the return type to 'bool'
and return true/false instead of CMD_RET* (as you don't "return
dtimg_get_opthex() further).
Last bit: as we don't change argcp and argvp, maybe it's better to
make them const. And I'd probably add some kernel-doc comment for such
function, mentioning, which parameters are [in] and which are [out].
Minor though.
> +{
> + char *endp;
> + u32 val;
> +
> + if (!argcp || !argvp || !valp)
Hmm, do we have something like assert() in U-Boot? Seems like it fits
here. Just a thought.
> + return CMD_RET_FAILURE;
> +
> + if (*argcp < 2) {
> + printf("Error: Option '%s' expects an argument\n", (*argvp)[0]);
> + return CMD_RET_FAILURE;
> + }
> +
> + val = simple_strtoul((*argvp)[1], &endp, 16);
> + if (*endp != '\0') {
> + printf("Error: Bad value '%s=%s'\n", (*argvp)[0], (*argvp)[1]);
Maybe would be easier to read if using alias variables. Compiler
should optimize that I guess.
> + return CMD_RET_FAILURE;
> + }
> +
> + *valp = val;
> + (*argcp)--;
> + (*argvp)++;
> +
> + return CMD_RET_SUCCESS;
> +}
> +
> +static int dtimg_get_opt(int argc, char * const argv[],
> + struct dt_table_entry *ep, char **envstrp)
> +{
> + if (!ep || argc < 0 || !argv || !envstrp)
> + return CMD_RET_FAILURE;
> +
> + for (; argc > 0; argc--, argv++) {
> + int ret;
> +
> + if (!strcmp(argv[0], "--id")) {
After giving it some thought, not a lot of people are using 'dtimg'
yet. So if you think that existing interface is ugly, maybe now is a
good time to change it. Or after merging this patch series and my
Android Boot Flow patch series, to make development easier. Looking at
that '--id'... I never saw such params in other U-Boot commands, so
probably it's better to avoid using that, for consistency.
Even the code itself gets hairy because I failed to think ahead when
coming up with proper extensible interface for 'dtimg'... Can we
really go ahead and rework it like this:
dtimg start index <num> <addr> [varname]
dtimg start id <num> <addr> [varname]
dtimg start rev <num> <addr> [varname]
Not only it will be more consistent with what I'm trying to do in
'abootimg' right now, even the command code should become easier (I
guess). Again, I don't have *strong* preference here. Methodologically
it's bad to change existing interface, but it wasn't too long since I
introduced it, so maybe not a lot of people are using it at this
moment. Your decision.
Also, probably sandbox unit test should be added for new
functionality. The 'abootimg' test can be used as a template. Later we
should add some proper documentation to doc/ as well...
> + ret = dtimg_get_opthex(&argc, &argv, &ep->id);
> + if (ret != CMD_RET_SUCCESS)
> + return ret;
> + } else if (!strcmp(argv[0], "--rev")) {
> + ret = dtimg_get_opthex(&argc, &argv, &ep->rev);
> + if (ret != CMD_RET_SUCCESS)
> + return ret;
> + } else if (argc == 1 && argv[0][0] != '-' &&
> + !isdigit(argv[0][0])) {
> + *envstrp = argv[0];
> + } else {
> + printf("Error: Option '%s' not supported\n", argv[0]);
> + return CMD_RET_FAILURE;
> + }
> + }
> +
> + return CMD_RET_SUCCESS;
> +}
> +
> static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
> {
> ulong hdr_addr;
> @@ -55,6 +111,8 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
> char *endp;
> ulong fdt_addr;
> u32 fdt_size;
> + struct dt_table_entry entry = { 0 };
> + char *envstr = NULL;
> ulong envval;
> int ret;
>
> @@ -70,7 +128,18 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
> return CMD_RET_FAILURE;
> }
>
> - if (!android_dt_get_fdt_by_index(hdr_addr, index, &fdt_addr, &fdt_size))
> + /*
> + * Consume all processed mandatory arguments.
> + * Prepare for parsing the optional ones.
> + */
> + argc -= 3;
> + argv += 3;
> +
> + ret = dtimg_get_opt(argc, argv, &entry, &envstr);
> + if (ret != CMD_RET_SUCCESS)
> + return ret;
> +
> + if (!android_dt_get_fdt(hdr_addr, index, &entry, &fdt_addr, &fdt_size))
> return CMD_RET_FAILURE;
>
> switch (cmd) {
> @@ -85,11 +154,11 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
> return CMD_RET_FAILURE;
> }
>
> - if (argv[3]) {
> - ret = env_set_hex(argv[3], envval);
> + if (envstr) {
> + ret = env_set_hex(envstr, envval);
> if (ret)
> printf("Error(%d) env-setting '%s=0x%lx'\n",
> - ret, argv[3], envval);
> + ret, envstr, envval);
> } else {
> printf("0x%lx\n", envval);
> }
> @@ -111,8 +180,8 @@ static int do_dtimg_size(cmd_tbl_t *cmdtp, int flag, int argc,
>
> static cmd_tbl_t cmd_dtimg_sub[] = {
> U_BOOT_CMD_MKENT(dump, 2, 0, do_dtimg_dump, "", ""),
> - U_BOOT_CMD_MKENT(start, 4, 0, do_dtimg_start, "", ""),
> - U_BOOT_CMD_MKENT(size, 4, 0, do_dtimg_size, "", ""),
> + U_BOOT_CMD_MKENT(start, 8, 0, do_dtimg_start, "", ""),
> + U_BOOT_CMD_MKENT(size, 8, 0, do_dtimg_size, "", ""),
> };
>
Have you fixed command usage (help) strings as well? Can't find it in
this patch... If it's somewhere else, please point me out.
> static int do_dtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> diff --git a/common/image-android-dt.c b/common/image-android-dt.c
> index a2d52df4a2a9..91e6b4eca23a 100644
> --- a/common/image-android-dt.c
> +++ b/common/image-android-dt.c
> @@ -5,7 +5,6 @@
> */
>
> #include <image-android-dt.h>
> -#include <dt_table.h>
> #include <common.h>
> #include <linux/libfdt.h>
> #include <mapmem.h>
> @@ -38,14 +37,15 @@ bool android_dt_check_header(ulong hdr_addr)
> *
> * @return true on success or false on error
> */
> -bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
> - u32 *size)
> +bool android_dt_get_fdt(ulong hdr_addr, u32 index,
> + struct dt_table_entry *ep, ulong *addr, u32 *size)
[strong opinion] I don't really like the way index/id/rev is handled
in this function... Would really like to see 3 different functions to
handle getting DTB/DTBO file by index/id/rev correspondingly. *OR* one
single function, but providing some flag, telling what to use
(index/id/rev). What I don't like in current implementation is a lot
of suggesting about which criteria to use for DTB lookup. As I see it,
user must know exactly what should be used. And function shouldn't be
too smart in that regard. If you have a different opinion, please
elaborate.
Also, please fix the Doxygen/kernel-doc comment for this function
(above) accordingly, describing its usage, parameters, etc. User
should only read the comment to understand how to use the function,
not the code itself. At least in an ideal world :)
> {
> const struct dt_table_header *hdr;
> const struct dt_table_entry *e;
> u32 entry_count, entries_offset, entry_size;
> ulong e_addr;
> - u32 dt_offset, dt_size;
> + u32 dt_offset, dt_size, dt_id, dt_rev;
> + int i, cnt;
>
> hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> entry_count = fdt32_to_cpu(hdr->dt_entry_count);
> @@ -59,6 +59,62 @@ bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
> return false;
> }
>
> + /*
> + * In case of a NULL dt_table_entry _or_ if both its 'id' and 'rev'
> + * fields are empty/zero, return the ${index}'th DTB/DTBO entry
> + */
> + if (!ep || (!ep->id && !ep->rev))
> + goto found;
> +
> + /*
> + * - In case of non-zero value received in both 'id' and 'rev' fields,
> + * return the ${cnt}'th DTB/DTBO entry matching both fields
> + * - In case of non-zero value received in 'id' and zero in 'rev',
> + * return the ${cnt}'th DTB/DTBO entry matching the 'id' field only
> + * - In case of non-zero value received in 'rev' and zero in 'id',
> + * return the ${cnt}'th DTB/DTBO entry matching the 'rev' field only
> + * In any of the above cases: 0 <= ${cnt} <= ${index} < entry_count
> + */
Comments like this should go to Doxygen comment above...
> + for (i = 0, cnt = -1; i < entry_count; i++) {
> + e_addr = hdr_addr + entries_offset + i * entry_size;
> + e = map_sysmem(e_addr, sizeof(*e));
> + dt_id = fdt32_to_cpu(e->id);
> + dt_rev = fdt32_to_cpu(e->rev);
> + unmap_sysmem(e);
> +
> + if (ep->id && ep->rev) {
> + if (ep->id == dt_id && ep->rev == dt_rev) {
> + cnt++;
> + if (cnt == index) {
> + index = i;
> + goto found;
> + }
> + }
> + } else if (ep->id) {
> + if (ep->id == dt_id) {
> + cnt++;
> + if (cnt == index) {
> + index = i;
> + goto found;
> + }
> + }
> + } else if (ep->rev) {
> + if (ep->rev == dt_rev) {
> + cnt++;
> + if (cnt == index) {
> + index = i;
> + goto found;
> + }
> + }
> + }
> + }
> +
> + printf("Error: No #%d entry having id=0x%x && rev=0x%x\n",
> + index, ep->id, ep->rev);
> +
> + return false;
> +
> +found:
> e_addr = hdr_addr + entries_offset + index * entry_size;
> e = map_sysmem(e_addr, sizeof(*e));
> dt_offset = fdt32_to_cpu(e->dt_offset);
> diff --git a/include/image-android-dt.h b/include/image-android-dt.h
> index 9a3aa8fa30fb..15f6115eedf0 100644
> --- a/include/image-android-dt.h
> +++ b/include/image-android-dt.h
> @@ -8,10 +8,11 @@
> #define IMAGE_ANDROID_DT_H
>
> #include <linux/types.h>
> +#include <dt_table.h>
>
> bool android_dt_check_header(ulong hdr_addr);
> -bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
> - u32 *size);
> +bool android_dt_get_fdt(ulong hdr_addr, u32 index,
> + struct dt_table_entry *ep, ulong *addr, u32 *size);
>
> #if !defined(CONFIG_SPL_BUILD)
> void android_dt_print_contents(ulong hdr_addr);
> --
Sorry, right now I don't agree with overall design decisions made in
this patch. Although I can see how my failure to provide decent
'dtimg' interface from the beginning led to these implications, I
still think we shouldn't do it this way. Let's discuss.
To repeat myself, that's how I see the better solution:
1. Interface: 3 different uses:
dtimg start index <num> <addr> [varname]
dtimg start id <num> <addr> [varname]
dtimg start rev <num> <addr> [varname]
and the same for dtimg size probably. Anyway, the point is, those
should be separated, in a way that no guessing should be done in
programming.
2. No guesswork in programming. That means we only look for index, if
"index" specified, etc. My preference is "stupid" functions, and smart
user :)
Hope it makes sense. Please let me know what you think (I could easily
missed something, patch is quite big and I'm also focusing on Boot
Flow series in parallel).
Another thing we should discuss is the process, i.e. how exactly our
series should intertwine, their dependencies on each other, and merge
order. Right now I can't finish the "abootimg get_dtb_file id/rev ..."
sub-command implementation, as it depends on this patch of yours... So
we have two choices here:
1. I only implement
abootimg get_dtb_file index <num> [addr_var] [size_var]
and don't implement next two usages at all (for now):
abootimg get_dtb_file id <num> [addr_var] [size_var]
abootimg get_dtb_file rev <num> [addr_var] [size_var]
We can easily add those once your series is merged.
2. Your series *must* be merged first, so my series can use it.
I prefer (1), as it makes it possible to break the unnecessary
dependency. What do you think?
> 2.24.0
>
More information about the U-Boot
mailing list