Re: [PATCH v7 44/50] drivers/of: Split unflatten_dt_node()
From: Rob Herring <hidden>
Date: 2015-11-04 18:43:08
Also in:
linux-pci, linuxppc-dev
On Wed, Nov 4, 2015 at 7:12 AM, Gavin Shan [off-list ref] wrote:
quoted hunk ↗ jump to hunk
The function unflatten_dt_node() is called recursively to unflatten device nodes and properties in the FDT blob. It looks complicated and hard to be understood. This splits the function into 3 functions: populate_properties(), populate_node() and unflatten_dt_node(). populate_properties(), which is called by populate_node(), creates properties for the indicated device node. The later one creates the device nodes from FDT blob. populate_node() gets the offset in FDT blob for next device nodes and then calls populate_node(). No logical changes introduced. Signed-off-by: Gavin Shan <redacted> --- drivers/of/fdt.c | 275 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 160 insertions(+), 115 deletions(-)diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 6e82bc42..173b036 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c@@ -160,39 +160,127 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size, return res; } -/** - * unflatten_dt_node - Alloc and populate a device_node from the flat tree - * @blob: The parent device tree blob - * @mem: Memory chunk to use for allocating device nodes and properties - * @poffset: pointer to node in flat tree - * @dad: Parent struct device_node - * @nodepp: The device_node tree created by the call - * @fpsize: Size of the node path up at the current depth. - * @dryrun: If true, do not allocate device nodes but still calculate needed - * memory size - */ -static void * unflatten_dt_node(const void *blob, - void *mem, - int *poffset, - struct device_node *dad, - struct device_node **nodepp, - unsigned long fpsize, +static void populate_properties(const void *blob, + int offset, + void **mem, + struct device_node *np, + const char *nodename, bool dryrun)
I'd like to make dryrun implicit. It is basically a function of NULL or near NULL pointers.
{
- const __be32 *p;
+ struct property *pp, **pprev = NULL;
+ int cur;
+ bool has_name = false;
+
+ pprev = &np->properties;
+ cur = fdt_first_property_offset(blob, offset);
+ while (cur >= 0) {This could be better written as a for loop to avoid the gotos: for (cur = fdt_first_property_offset(blob, offset); cur >=0; cur = fdt_next_property_offset(blob, cur))
+ const __be32 *val;
+ const char *pname;
+ u32 sz;
+
+ val = fdt_getprop_by_offset(blob, cur, &pname, &sz);
+ if (!val) {
+ pr_warn("%s: Cannot locate property at 0x%x\n",
+ __func__, cur);
+ goto next;
+ }
+
+ if (!pname) {
+ pr_warn("%s: Cannot find property name at 0x%x\n",
+ __func__, cur);
+ goto next;
+ } else if (!strcmp(pname, "name")) {
+ has_name = true;
+ }
+
+ pp = unflatten_dt_alloc(mem, sizeof(struct property),
+ __alignof__(struct property));
+ if (!dryrun) {Then: if (dryrun) continue; to save some indentation and vertical code.
+ /* We accept flattened tree phandles either in
+ * ePAPR-style "phandle" properties, or the
+ * legacy "linux,phandle" properties. If both
+ * appear and have different values, things
+ * will get weird. Don't do that.
+ */
+ if (!strcmp(pname, "phandle") ||
+ !strcmp(pname, "linux,phandle")) {
+ if (!np->phandle)
+ np->phandle = be32_to_cpup(val);
+ }
+
+ /* And we process the "ibm,phandle" property
+ * used in pSeries dynamic device tree
+ * stuff
+ */
+ if (!strcmp(pname, "ibm,phandle"))
+ np->phandle = be32_to_cpup(val);
+
+ pp->name = (char *)pname;
+ pp->length = sz;
+ pp->value = (__be32 *)val;
+ *pprev = pp;
+ pprev = &pp->next;
+ }
+next:
+ cur = fdt_next_property_offset(blob, cur);
+ }
+
+ /* With version 0x10 we may not have the name property,
+ * recreate it here from the unit name if absent
+ */
+ if (!has_name) {
+ const char *p = nodename, *ps = p, *pa = NULL;
+ int len;
+
+ while (*p) {
+ if ((*p) == '@')
+ pa = p;
+ else if ((*p) == '/')
+ ps = p + 1;
+ p++;
+ }
+
+ if (pa < ps)
+ pa = p;
+ len = (pa - ps) + 1;
+ pp = unflatten_dt_alloc(mem, sizeof(struct property) + len,
+ __alignof__(struct property));
+ if (!dryrun) {
+ pp->name = "name";
+ pp->length = len;
+ pp->value = pp + 1;
+ *pprev = pp;
+ pprev = &pp->next;
+ memcpy(pp->value, ps, len - 1);
+ ((char *)pp->value)[len - 1] = 0;
+ pr_debug("fixed up name for %s -> %s\n",
+ nodename, (char *)pp->value);
+ }
+ }
+
+ if (!dryrun)
+ *pprev = NULL;
+}
+
+static unsigned long populate_node(const void *blob,
+ int offset,
+ void **mem,
+ struct device_node *dad,
+ unsigned long fpsize,
+ struct device_node **pnp,
+ bool dryrun)I think dryrun could be implied here too.
+{
struct device_node *np;
- struct property *pp, **prev_pp = NULL;
const char *pathp;
unsigned int l, allocl;
- static int depth = 0;
- int old_depth;
- int offset;
- int has_name = 0;
- int new_format = 0;
+ bool new_format = false;
+ char *fname;
- pathp = fdt_get_name(blob, *poffset, &l);
- if (!pathp)
- return mem;
+ pathp = fdt_get_name(blob, offset, &l);
+ if (!pathp) {
+ *pnp = NULL;Can't pnp be NULL?
quoted hunk ↗ jump to hunk
+ return 0; + } allocl = ++l;@@ -202,7 +290,7 @@ static void * unflatten_dt_node(const void *blob, * not '/'. */ if ((*pathp) != '/') { - new_format = 1; + new_format = true; if (fpsize == 0) { /* root node: special case. fpsize accounts for path * plus terminating zero. root node only has '/', so@@ -222,112 +310,38 @@ static void * unflatten_dt_node(const void *blob, } } - np = unflatten_dt_alloc(&mem, sizeof(struct device_node) + allocl, + np = unflatten_dt_alloc(mem, sizeof(struct device_node) + allocl, __alignof__(struct device_node)); if (!dryrun) { - char *fn; of_node_init(np); - np->full_name = fn = ((char *)np) + sizeof(*np); + np->full_name = fname = ((char *)np) + sizeof(*np);
If you kept "fn" that would cut down the diff and make it a bit easier to review.
if (new_format) {
- /* rebuild full path for new format */
+ /* Rebuild full path for new format */
if (dad && dad->parent) {
- strcpy(fn, dad->full_name);
+ strcpy(fname, dad->full_name);
#ifdef DEBUG
- if ((strlen(fn) + l + 1) != allocl) {
+ if ((strlen(fname) + l + 1) != allocl) {
pr_debug("%s: p: %d, l: %d, a: %d\n",
- pathp, (int)strlen(fn),
- l, allocl);
+ pathp, (int)strlen(fn),This won't compile if enabled (should be fname).
+ l, allocl);
}
#endif
- fn += strlen(fn);
+ fname += strlen(fname);
}
- *(fn++) = '/';
+ *(fname++) = '/';
}
- memcpy(fn, pathp, l);
+ memcpy(fname, pathp, l);
- prev_pp = &np->properties;
- if (dad != NULL) {
+ if (dad) {
np->parent = dad;
np->sibling = dad->child;
dad->child = np;
}
}
- /* process properties */
- for (offset = fdt_first_property_offset(blob, *poffset);
- (offset >= 0);
- (offset = fdt_next_property_offset(blob, offset))) {
- const char *pname;
- u32 sz;
- if (!(p = fdt_getprop_by_offset(blob, offset, &pname, &sz))) {
- offset = -FDT_ERR_INTERNAL;
- break;
- }
-
- if (pname == NULL) {
- pr_info("Can't find property name in list !\n");
- break;
- }
- if (strcmp(pname, "name") == 0)
- has_name = 1;
- pp = unflatten_dt_alloc(&mem, sizeof(struct property),
- __alignof__(struct property));
- if (!dryrun) {
- /* We accept flattened tree phandles either in
- * ePAPR-style "phandle" properties, or the
- * legacy "linux,phandle" properties. If both
- * appear and have different values, things
- * will get weird. Don't do that. */
- if ((strcmp(pname, "phandle") == 0) ||
- (strcmp(pname, "linux,phandle") == 0)) {
- if (np->phandle == 0)
- np->phandle = be32_to_cpup(p);
- }
- /* And we process the "ibm,phandle" property
- * used in pSeries dynamic device tree
- * stuff */
- if (strcmp(pname, "ibm,phandle") == 0)
- np->phandle = be32_to_cpup(p);
- pp->name = (char *)pname;
- pp->length = sz;
- pp->value = (__be32 *)p;
- *prev_pp = pp;
- prev_pp = &pp->next;
- }
- }
- /* with version 0x10 we may not have the name property, recreate
- * it here from the unit name if absent
- */
- if (!has_name) {
- const char *p1 = pathp, *ps = pathp, *pa = NULL;
- int sz;
-
- while (*p1) {
- if ((*p1) == '@')
- pa = p1;
- if ((*p1) == '/')
- ps = p1 + 1;
- p1++;
- }
- if (pa < ps)
- pa = p1;
- sz = (pa - ps) + 1;
- pp = unflatten_dt_alloc(&mem, sizeof(struct property) + sz,
- __alignof__(struct property));
- if (!dryrun) {
- pp->name = "name";
- pp->length = sz;
- pp->value = pp + 1;
- *prev_pp = pp;
- prev_pp = &pp->next;
- memcpy(pp->value, ps, sz - 1);
- ((char *)pp->value)[sz - 1] = 0;
- pr_debug("fixed up name for %s -> %s\n", pathp,
- (char *)pp->value);
- }
- }
+ /* Populate the properties */Kind of a useless comment.
quoted hunk ↗ jump to hunk
+ populate_properties(blob, offset, mem, np, pathp, dryrun); if (!dryrun) { - *prev_pp = NULL; np->name = of_get_property(np, "name", NULL); np->type = of_get_property(np, "device_type", NULL);@@ -337,6 +351,37 @@ static void * unflatten_dt_node(const void *blob, np->type = "<NULL>"; } + *pnp = np; + return fpsize; +} + +/** + * unflatten_dt_node - Alloc and populate a device_node from the flat tree + * @blob: The parent device tree blob + * @mem: Memory chunk to use for allocating device nodes and properties + * @poffset: pointer to node in flat tree + * @dad: Parent struct device_node + * @nodepp: The device_node tree created by the call + * @fpsize: Size of the node path up at the current depth. + * @dryrun: If true, do not allocate device nodes but still calculate needed + * memory size + */ +static void *unflatten_dt_node(const void *blob, + void *mem, + int *poffset, + struct device_node *dad, + struct device_node **nodepp, + unsigned long fpsize, + bool dryrun) +{ + struct device_node *np; + static int depth; + int old_depth; + + fpsize = populate_node(blob, *poffset, &mem, dad, fpsize, &np, dryrun);
Doesn't this give a warning assigning a ptr to long? Looks like np can be moved internal to populate_node.
+ if (!fpsize)
+ return mem;
+
old_depth = depth;
*poffset = fdt_next_node(blob, *poffset, &depth);
if (depth < 0)
--
2.1.0