| Message ID | 3950d7da35130a850ba9217ac7bfef781fa850b2.1613042485.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [DTC,RFC] dtc: Allow better error reporting | expand |
On Thu, Feb 11, 2021 at 5:27 AM Viresh Kumar <viresh.kumar@linaro.org> 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. > > 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@linaro.org> > > --- > 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@gmail.com/ > > 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 This needs to go in libfdt. The executables can always print, it's the libfdt library that may or may not be able to. Rob
On 11-02-21, 08:22, Rob Herring wrote: > This needs to go in libfdt. > > The executables can always print, it's the libfdt library that may or > may not be able to. Sure, Will add this delta to the patch.. $ gd 3950d7da35130a850ba9217ac7bfef781fa850b2.. diff --git a/dtc.h b/dtc.h index b8ffec155263..d3e82fb8e3db 100644 --- a/dtc.h +++ b/dtc.h @@ -29,12 +29,6 @@ #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 - #define DEFAULT_FDT_VERSION 17 /* diff --git a/fdtoverlay.c b/fdtoverlay.c index 5f60ce4e4cea..5350af65679f 100644 --- a/fdtoverlay.c +++ b/fdtoverlay.c @@ -16,7 +16,6 @@ #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 b24286ac8c6c..17fc6075412f 100644 --- a/libfdt/fdt_overlay.c +++ b/libfdt/fdt_overlay.c @@ -10,7 +10,6 @@ #include <libfdt.h> #include "libfdt_internal.h" -#include "../dtc.h" /** * overlay_get_target_phandle - retrieves the target phandle of a fragment diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h index 16bda1906a7b..fc7b3fa2eb2a 100644 --- a/libfdt/libfdt_internal.h +++ b/libfdt/libfdt_internal.h @@ -7,6 +7,12 @@ */ #include <fdt.h> +#ifdef VERBOSE +#define dtc_err(fmt, ...) fprintf(stderr, "DTC: %s: %d: " fmt, __func__, __LINE__, ##__VA_ARGS__) +#else +#define dtc_err(fmt, ...) +#endif + #define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1)) #define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE)) -- viresh
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@linaro.org> > > --- > 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@gmail.com/ > > 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
On 17-02-21, 16:07, David Gibson wrote: > > 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. Okay, I will move it to libfdt_env.h. And using -DVERBOSE to enable rich errors look fine to you, right ? > 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. Actually it worked for me because of how we compile this in kernel, we don't link different libraries there. libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o libfdt = $(addprefix libfdt/,$(libfdt-objs)) fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o HOST_EXTRACFLAGS := -DVERBOSE > Except.... it might make more sense to do this in dtc rather than > libfdt, more on that in different mails. I am not sure about this comment, are you going to send more emails on this ? -- viresh
On Fri, Feb 19, 2021 at 11:07:53AM +0530, Viresh Kumar wrote: > On 17-02-21, 16:07, David Gibson wrote: > > > 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. > > Okay, I will move it to libfdt_env.h. And using -DVERBOSE to enable > rich errors look fine to you, right ? Well, using -DVERBOSE looks like an option worth considering further to me. I'm not yet totally convinced its the best approach. > > 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. > > Actually it worked for me because of how we compile this in kernel, we > don't link different libraries there. > > libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > libfdt = $(addprefix libfdt/,$(libfdt-objs)) > fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o > > HOST_EXTRACFLAGS := -DVERBOSE Um.. I think this is only working because the objects are compiled one way for the target and another for the host. If you ever wanted to build any other host side tools that shouldn't have -DVERBOSE, this would break. > > Except.... it might make more sense to do this in dtc rather than > > libfdt, more on that in different mails. > > I am not sure about this comment, are you going to send more emails on > this ? Sorry, I got sidetracked and didn't get around to following that up. I'm still trying to do that. -- 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
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 + #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" /** * 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)
The dtc tool doesn't do very elaborate error reporting to keep the size of the executables small enough for all kind of applications. 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@linaro.org> --- 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@gmail.com/ Thanks. --- dtc.h | 6 ++ fdtoverlay.c | 1 + libfdt/fdt_overlay.c | 156 ++++++++++++++++++++++++++++++++++--------- 3 files changed, 132 insertions(+), 31 deletions(-) -- 2.25.0.rc1.19.g042ed3e048af