On Thu, Feb 11, 2021 at 04:57:21PM +0530, Viresh Kumar wrote: > The dtc tool doesn't do very elaborate error reporting to keep the size > of the executables small enough for all kind of applications. dtc itself does plenty of error reporting, it's just libfdt that tries to keep things minimal. > This patch tries to provide a way to do better error reporting, without > increasing the size of the executables for such cases (where we don't > want elaborate error reporting). > > The error reporting will only be enabled if 'VERBOSE' (as -DVERBOSE) > flag is passed during compilation of the tools. > > This also updates the fdtoverlay tool to do better error reporting, > which is required by the Linux Kernel for now. > > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx> > > --- > Hi David, > > Unfortunately I am not a core DT guy and don't understand most of the > stuff going on here. I tried to do minimal changes to get more > information out on errors and it may require someone who understands the > code well to write better error logs. > > FWIW, I stumbled upon this because of the discussion that happened here: > > https://lore.kernel.org/lkml/74f8aa8f-ffab-3b0f-186f-31fb7395ebbb@xxxxxxxxx/ > > Thanks. > > --- > dtc.h | 6 ++ > fdtoverlay.c | 1 + > libfdt/fdt_overlay.c | 156 ++++++++++++++++++++++++++++++++++--------- > 3 files changed, 132 insertions(+), 31 deletions(-) > > diff --git a/dtc.h b/dtc.h > index d3e82fb8e3db..b8ffec155263 100644 > --- a/dtc.h > +++ b/dtc.h > @@ -29,6 +29,12 @@ > #define debug(...) > #endif > > +#ifdef VERBOSE > +#define dtc_err(fmt, ...) fprintf(stderr, "DTC: %s: %d: " fmt, __func__, __LINE__, ##__VA_ARGS__) > +#else > +#define dtc_err(fmt, ...) > +#endif Actually, the natural way to handle this is to make dtc_err() - hrm... bad name, since this is libfdt - be something that must be provided by libfdt_env.h. The default version would have it be a no-op, with a define that makes it use stdio. This has the additional advantage that it would be relatively straightfoward to enable the rich reporting in a non-POSIXish environment (these should provide their own libfdt_env.h and it can implement the error reporting callback in a way that makes sense in that environment. You will also definitely need Makefile changes, because you'll need to make the fdtoverlay binary link to the verbose-compiled version of libfdt not the normal one. Except.... it might make more sense to do this in dtc rather than libfdt, more on that in different mails. > + > #define DEFAULT_FDT_VERSION 17 > > /* > diff --git a/fdtoverlay.c b/fdtoverlay.c > index 5350af65679f..5f60ce4e4cea 100644 > --- a/fdtoverlay.c > +++ b/fdtoverlay.c > @@ -16,6 +16,7 @@ > > #include <libfdt.h> > > +#include "dtc.h" > #include "util.h" > > #define BUF_INCREMENT 65536 > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c > index d217e79b6722..b24286ac8c6c 100644 > --- a/libfdt/fdt_overlay.c > +++ b/libfdt/fdt_overlay.c > @@ -10,6 +10,7 @@ > #include <libfdt.h> > > #include "libfdt_internal.h" > +#include "../dtc.h" Yeah, the libfdt code can't include dtc.h. > > /** > * overlay_get_target_phandle - retrieves the target phandle of a fragment > @@ -160,12 +161,16 @@ static int overlay_adjust_node_phandles(void *fdto, int node, > int ret; > > ret = overlay_phandle_add_offset(fdto, node, "phandle", delta); > - if (ret && ret != -FDT_ERR_NOTFOUND) > + if (ret && ret != -FDT_ERR_NOTFOUND) { > + dtc_err("Failed to add phandle offset (%d: %s)\n", node, fdt_strerror(ret)); > return ret; > + } > > ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta); > - if (ret && ret != -FDT_ERR_NOTFOUND) > + if (ret && ret != -FDT_ERR_NOTFOUND) { > + dtc_err("Failed to add linux,phandle offset (%d: %s)\n", node, fdt_strerror(ret)); > return ret; > + } > > fdt_for_each_subnode(child, fdto, node) { > ret = overlay_adjust_node_phandles(fdto, child, delta); > @@ -239,12 +244,17 @@ static int overlay_update_local_node_references(void *fdto, > if (!fixup_val) > return fixup_len; > > - if (fixup_len % sizeof(uint32_t)) > + if (fixup_len % sizeof(uint32_t)) { > + dtc_err("Invalid fixup length\n"); > return -FDT_ERR_BADOVERLAY; > + } > fixup_len /= sizeof(uint32_t); > > tree_val = fdt_getprop(fdto, tree_node, name, &tree_len); > if (!tree_val) { > + dtc_err("Failed to get property: %s: %d\n", name, > + tree_len); > + > if (tree_len == -FDT_ERR_NOTFOUND) > return -FDT_ERR_BADOVERLAY; > > @@ -274,11 +284,17 @@ static int overlay_update_local_node_references(void *fdto, > poffset, > &adj_val, > sizeof(adj_val)); > - if (ret == -FDT_ERR_NOSPACE) > + if (ret == -FDT_ERR_NOSPACE) { > + dtc_err("Failed to update property's name: %s\n", > + name); > return -FDT_ERR_BADOVERLAY; > + } > > - if (ret) > + if (ret) { > + dtc_err("Failed to update property's name: %s: %s\n", > + name, fdt_strerror(ret)); > return ret; > + } > } > } > > @@ -289,10 +305,16 @@ static int overlay_update_local_node_references(void *fdto, > > tree_child = fdt_subnode_offset(fdto, tree_node, > fixup_child_name); > - if (tree_child == -FDT_ERR_NOTFOUND) > + if (tree_child == -FDT_ERR_NOTFOUND) { > + dtc_err("Failed to find subnode: %s\n", > + fixup_child_name); > return -FDT_ERR_BADOVERLAY; > - if (tree_child < 0) > + } > + if (tree_child < 0) { > + dtc_err("Failed to find subnode: %s: %s\n", > + fixup_child_name, fdt_strerror(tree_child)); > return tree_child; > + } > > ret = overlay_update_local_node_references(fdto, > tree_child, > @@ -332,6 +354,8 @@ static int overlay_update_local_references(void *fdto, uint32_t delta) > if (fixups == -FDT_ERR_NOTFOUND) > return 0; > > + dtc_err("Failed to find local_fixups (%s)\n", > + fdt_strerror(fixups)); > return fixups; > } > > @@ -435,6 +459,8 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off, > value = fdt_getprop_by_offset(fdto, property, > &label, &len); > if (!value) { > + dtc_err("Failed to get property by offset (%s)\n", > + fdt_strerror(len)); > if (len == -FDT_ERR_NOTFOUND) > return -FDT_ERR_INTERNAL; > > @@ -450,8 +476,10 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off, > int poffset, ret; > > fixup_end = memchr(value, '\0', len); > - if (!fixup_end) > + if (!fixup_end) { > + dtc_err("fixup_end can't be 0: %s: %s\n", value, label); > return -FDT_ERR_BADOVERLAY; > + } > fixup_len = fixup_end - fixup_str; > > len -= fixup_len + 1; > @@ -459,32 +487,47 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off, > > path = fixup_str; > sep = memchr(fixup_str, ':', fixup_len); > - if (!sep || *sep != ':') > + if (!sep || *sep != ':') { > + dtc_err("Missing ':' separator: %s: %s\n", value, > + label); > return -FDT_ERR_BADOVERLAY; > + } > > path_len = sep - path; > - if (path_len == (fixup_len - 1)) > + if (path_len == (fixup_len - 1)) { > + dtc_err("Invalid path_len: %s: %s\n", value, label); > return -FDT_ERR_BADOVERLAY; > + } > > fixup_len -= path_len + 1; > name = sep + 1; > sep = memchr(name, ':', fixup_len); > - if (!sep || *sep != ':') > + if (!sep || *sep != ':') { > + dtc_err("Missing ':' separator: %s: %s\n", value, > + label); > return -FDT_ERR_BADOVERLAY; > + } > > name_len = sep - name; > - if (!name_len) > + if (!name_len) { > + dtc_err("name_len can't be 0: %s: %s\n", value, label); > return -FDT_ERR_BADOVERLAY; > + } > > poffset = strtoul(sep + 1, &endptr, 10); > - if ((*endptr != '\0') || (endptr <= (sep + 1))) > + if ((*endptr != '\0') || (endptr <= (sep + 1))) { > + dtc_err("Invalid name string: %s: %s\n", value, label); > return -FDT_ERR_BADOVERLAY; > + } > > ret = overlay_fixup_one_phandle(fdt, fdto, symbols_off, > path, path_len, name, name_len, > poffset, label); > - if (ret) > + if (ret) { > + dtc_err("failed to fixup one phandle: %s: %s: %s\n", > + value, label, fdt_strerror(ret)); > return ret; > + } > } while (len > 0); > > return 0; > @@ -516,13 +559,19 @@ static int overlay_fixup_phandles(void *fdt, void *fdto) > fixups_off = fdt_path_offset(fdto, "/__fixups__"); > if (fixups_off == -FDT_ERR_NOTFOUND) > return 0; /* nothing to do */ > - if (fixups_off < 0) > + if (fixups_off < 0) { > + dtc_err("Failed to get fixups offset (%s)\n", > + fdt_strerror(fixups_off)); > return fixups_off; > + } > > /* And base DTs without symbols */ > symbols_off = fdt_path_offset(fdt, "/__symbols__"); > - if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND))) > + if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND))) { > + dtc_err("Failed to get symbols offset (%s)\n", > + fdt_strerror(symbols_off)); > return symbols_off; > + } > > fdt_for_each_property_offset(property, fdto, fixups_off) { > int ret; > @@ -633,16 +682,27 @@ static int overlay_merge(void *fdt, void *fdto) > if (overlay == -FDT_ERR_NOTFOUND) > continue; > > - if (overlay < 0) > + if (overlay < 0) { > + dtc_err("Failed to find __overlay__ tag (%d: %s)\n", > + fragment, fdt_strerror(overlay)); > return overlay; > + } > > target = overlay_get_target(fdt, fdto, fragment, NULL); > - if (target < 0) > + if (target < 0) { > + dtc_err("Failed to retrieve fragment's target (%d: %s)\n", > + fragment, fdt_strerror(target)); > return target; > + } > > ret = overlay_apply_node(fdt, target, fdto, overlay); > - if (ret) > + if (ret) { > + if (ret != -FDT_ERR_NOSPACE) { > + dtc_err("Failed to apply overlay node (%d: %d: %s)\n", > + fragment, target, fdt_strerror(ret)); > + } > return ret; > + } > } > > return 0; > @@ -718,24 +778,35 @@ static int overlay_symbol_update(void *fdt, void *fdto) > root_sym = fdt_add_subnode(fdt, 0, "__symbols__"); > > /* any error is fatal now */ > - if (root_sym < 0) > + if (root_sym < 0) { > + dtc_err("Failed to get root __symbols__ (%s)\n", > + fdt_strerror(root_sym)); > return root_sym; > + } > > /* iterate over each overlay symbol */ > fdt_for_each_property_offset(prop, fdto, ov_sym) { > path = fdt_getprop_by_offset(fdto, prop, &name, &path_len); > - if (!path) > + if (!path) { > + dtc_err("Failed to get prop by offset (%s)\n", > + fdt_strerror(path_len)); > return path_len; > + } > > /* verify it's a string property (terminated by a single \0) */ > - if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1]) > + if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1]) { > + dtc_err("Invalid property (%d)\n", path_len); > return -FDT_ERR_BADVALUE; > + } > > /* keep end marker to avoid strlen() */ > e = path + path_len; > > - if (*path != '/') > + if (*path != '/') { > + dtc_err("Path should start with '/' (%s : %s)\n", path, > + name); > return -FDT_ERR_BADVALUE; > + } > > /* get fragment name first */ > s = strchr(path + 1, '/'); > @@ -769,26 +840,38 @@ static int overlay_symbol_update(void *fdt, void *fdto) > ret = fdt_subnode_offset_namelen(fdto, 0, frag_name, > frag_name_len); > /* not found? */ > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to find fragment index (%s: %s: %d)\n", > + path, frag_name, ret); > return -FDT_ERR_BADOVERLAY; > + } > fragment = ret; > > /* an __overlay__ subnode must exist */ > ret = fdt_subnode_offset(fdto, fragment, "__overlay__"); > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to find __overlay__ subnode (%s: %s: %d)\n", > + path, frag_name, ret); > return -FDT_ERR_BADOVERLAY; > + } > > /* get the target of the fragment */ > ret = overlay_get_target(fdt, fdto, fragment, &target_path); > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to get target for the fragment (%s: %s: %d)\n", > + path, frag_name, ret); > return ret; > + } > target = ret; > > /* if we have a target path use */ > if (!target_path) { > ret = get_path_len(fdt, target); > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to get path length (%s: %s: %d)\n", > + path, name, ret); > return ret; > + } > len = ret; > } else { > len = strlen(target_path); > @@ -796,14 +879,20 @@ static int overlay_symbol_update(void *fdt, void *fdto) > > ret = fdt_setprop_placeholder(fdt, root_sym, name, > len + (len > 1) + rel_path_len + 1, &p); > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to set prop placeholder (%s: %s: %d)\n", > + path, name, ret); > return ret; > + } > > if (!target_path) { > /* again in case setprop_placeholder changed it */ > ret = overlay_get_target(fdt, fdto, fragment, &target_path); > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to get target (%s: %s: %d)\n", > + path, name, ret); > return ret; > + } > target = ret; > } > > @@ -811,8 +900,11 @@ static int overlay_symbol_update(void *fdt, void *fdto) > if (len > 1) { /* target is not root */ > if (!target_path) { > ret = fdt_get_path(fdt, target, buf, len + 1); > - if (ret < 0) > + if (ret < 0) { > + dtc_err("Failed to get target path (%s: %s: %d)\n", > + path, name, ret); > return ret; > + } > } else > memcpy(buf, target_path, len + 1); > > @@ -836,8 +928,10 @@ int fdt_overlay_apply(void *fdt, void *fdto) > FDT_RO_PROBE(fdto); > > ret = fdt_find_max_phandle(fdt, &delta); > - if (ret) > + if (ret) { > + dtc_err("Failed to find max phandle (%s)\n", fdt_strerror(ret)); > goto err; > + } > > ret = overlay_adjust_local_phandles(fdto, delta); > if (ret) -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
