* [PATCH 0/3] of: generic infrastructure fixes
@ 2016-05-09 18:11 Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 1/3] of: rename *_node_sysfs to _node_post Pantelis Antoniou
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-09 18:11 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou, Pantelis Antoniou
The first patch renames the *_node_sysfs methods to _node_post since
this is more accurate when more work takes place besides sysfs
tweaking when the next patch is using a hashtable for phandles.
This is a win for very large blobs with a large number of phandles.
Current we use an exhaustive search which is not optimal at all.
Finally, a long standing request of applied overlays populating
the symbol list is implemented; this allows staggered overlay
application with a symbol from a previous overlay being used
by another after it.
Pantelis Antoniou (3):
of: rename *_node_sysfs to _node_post
of: Support hashtable lookups for phandles
of: overlay: Pick up label symbols from overlays.
drivers/of/base.c | 39 +++++++++++++++---
drivers/of/dynamic.c | 18 ++++++---
drivers/of/of_private.h | 35 ++++++++++++++++-
drivers/of/overlay.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/of/unittest.c | 4 +-
include/linux/of.h | 2 +
6 files changed, 186 insertions(+), 14 deletions(-)
--
1.7.12
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] of: rename *_node_sysfs to _node_post
2016-05-09 18:11 [PATCH 0/3] of: generic infrastructure fixes Pantelis Antoniou
@ 2016-05-09 18:11 ` Pantelis Antoniou
2016-05-09 21:49 ` Rob Herring
2016-05-09 18:11 ` [PATCH 2/3] of: Support hashtable lookups for phandles Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 3/3] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
2 siblings, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-09 18:11 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou, Pantelis Antoniou
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
drivers/of/base.c | 4 ++--
drivers/of/dynamic.c | 10 +++++-----
drivers/of/of_private.h | 4 ++--
drivers/of/unittest.c | 4 ++--
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9bbca56..20bbc2f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -156,7 +156,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
return rc;
}
-int __of_attach_node_sysfs(struct device_node *np)
+int __of_attach_node_post(struct device_node *np)
{
const char *name;
struct property *pp;
@@ -203,7 +203,7 @@ void __init of_core_init(void)
return;
}
for_each_of_allnodes(np)
- __of_attach_node_sysfs(np);
+ __of_attach_node_post(np);
mutex_unlock(&of_mutex);
/* Symlink in /proc as required by userspace ABI */
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 350b1cf..eb31a55 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -41,7 +41,7 @@ void of_node_put(struct device_node *node)
}
EXPORT_SYMBOL(of_node_put);
-void __of_detach_node_sysfs(struct device_node *np)
+void __of_detach_node_post(struct device_node *np)
{
struct property *pp;
@@ -251,7 +251,7 @@ int of_attach_node(struct device_node *np)
__of_attach_node(np);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
- __of_attach_node_sysfs(np);
+ __of_attach_node_post(np);
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
@@ -304,7 +304,7 @@ int of_detach_node(struct device_node *np)
__of_detach_node(np);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
- __of_detach_node_sysfs(np);
+ __of_detach_node_post(np);
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
@@ -628,10 +628,10 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
switch (ce->action) {
case OF_RECONFIG_ATTACH_NODE:
- __of_attach_node_sysfs(ce->np);
+ __of_attach_node_post(ce->np);
break;
case OF_RECONFIG_DETACH_NODE:
- __of_detach_node_sysfs(ce->np);
+ __of_detach_node_post(ce->np);
break;
case OF_RECONFIG_ADD_PROPERTY:
/* ignore duplicate names */
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 092cba7..10b0342 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -79,9 +79,9 @@ extern void __of_update_property_sysfs(struct device_node *np,
struct property *newprop, struct property *oldprop);
extern void __of_attach_node(struct device_node *np);
-extern int __of_attach_node_sysfs(struct device_node *np);
+extern int __of_attach_node_post(struct device_node *np);
extern void __of_detach_node(struct device_node *np);
-extern void __of_detach_node_sysfs(struct device_node *np);
+extern void __of_detach_node_post(struct device_node *np);
/* iterators for transactions, used for overlays */
/* forward iterator */
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e5a5ec0..7ea3689 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -931,7 +931,7 @@ static int attach_node_and_children(struct device_node *np)
of_node_clear_flag(np, OF_DETACHED);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
- __of_attach_node_sysfs(np);
+ __of_attach_node_post(np);
mutex_unlock(&of_mutex);
while (child) {
@@ -989,7 +989,7 @@ static int __init unittest_data_add(void)
if (!of_root) {
of_root = unittest_data_node;
for_each_of_allnodes(np)
- __of_attach_node_sysfs(np);
+ __of_attach_node_post(np);
of_aliases = of_find_node_by_path("/aliases");
of_chosen = of_find_node_by_path("/chosen");
return 0;
--
1.7.12
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-09 18:11 [PATCH 0/3] of: generic infrastructure fixes Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 1/3] of: rename *_node_sysfs to _node_post Pantelis Antoniou
@ 2016-05-09 18:11 ` Pantelis Antoniou
2016-05-09 20:38 ` Geert Uytterhoeven
2016-05-09 21:21 ` Rob Herring
2016-05-09 18:11 ` [PATCH 3/3] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
2 siblings, 2 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-09 18:11 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou, Pantelis Antoniou
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
drivers/of/base.c | 35 ++++++++++++++++++++++++++++++++---
drivers/of/dynamic.c | 8 ++++++++
drivers/of/of_private.h | 31 +++++++++++++++++++++++++++++++
include/linux/of.h | 2 ++
4 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20bbc2f..770cb95 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -27,6 +27,7 @@
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/proc_fs.h>
+#include <linux/rhashtable.h>
#include "of_private.h"
@@ -41,6 +42,16 @@ static const char *of_stdout_options;
struct kset *of_kset;
+const struct rhashtable_params of_phandle_ht_params = {
+ .key_offset = offsetof(struct device_node, phandle), /* base offset */
+ .key_len = sizeof(phandle),
+ .head_offset = offsetof(struct device_node, ht_node),
+ .automatic_shrinking = true,
+};
+
+struct rhashtable of_phandle_ht;
+bool of_phandle_ht_initialized;
+
/*
* Used to protect the of_aliases, to hold off addition of nodes to sysfs.
* This mutex must be held whenever modifications are being made to the
@@ -162,6 +173,12 @@ int __of_attach_node_post(struct device_node *np)
struct property *pp;
int rc;
+ if (of_phandle_ht_available()) {
+ rc = of_phandle_ht_insert(np);
+ WARN(rc, "insert to phandle hash fail @%s\n",
+ of_node_full_name(np));
+ }
+
if (!IS_ENABLED(CONFIG_SYSFS))
return 0;
@@ -194,6 +211,13 @@ void __init of_core_init(void)
struct device_node *np;
int ret;
+ ret = rhashtable_init(&of_phandle_ht, &of_phandle_ht_params);
+ if (ret) {
+ pr_warn("devicetree: Failed to initialize hashtable\n");
+ return;
+ }
+ of_phandle_ht_initialized = 1;
+
/* Create the kset, and register existing nodes */
mutex_lock(&of_mutex);
of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
@@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
return NULL;
raw_spin_lock_irqsave(&devtree_lock, flags);
- for_each_of_allnodes(np)
- if (np->phandle == handle)
- break;
+ /* when we're ready use the hash table */
+ if (of_phandle_ht_available() && !in_interrupt())
+ np = of_phandle_ht_lookup(handle);
+ else { /* fallback */
+ for_each_of_allnodes(np)
+ if (np->phandle == handle)
+ break;
+ }
of_node_get(np);
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index eb31a55..bdebdf5 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -11,6 +11,7 @@
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/proc_fs.h>
+#include <linux/rhashtable.h>
#include "of_private.h"
@@ -44,6 +45,13 @@ EXPORT_SYMBOL(of_node_put);
void __of_detach_node_post(struct device_node *np)
{
struct property *pp;
+ int rc;
+
+ if (of_phandle_ht_available()) {
+ rc = of_phandle_ht_remove(np);
+ WARN(rc, "remove from phandle hash fail @%s\n",
+ of_node_full_name(np));
+ }
if (!IS_ENABLED(CONFIG_SYSFS))
return;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 10b0342..88b3b8f 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -101,4 +101,35 @@ static inline int of_overlay_init(void)
}
#endif
+extern const struct rhashtable_params of_phandle_ht_params;
+extern struct rhashtable of_phandle_ht;
+extern bool of_phandle_ht_initialized;
+
+static inline bool of_phandle_ht_available(void)
+{
+ return of_phandle_ht_initialized;
+}
+
+static inline int of_phandle_ht_insert(struct device_node *np)
+{
+ if (!np || !np->phandle)
+ return 0;
+ return rhashtable_insert_fast(&of_phandle_ht,
+ &np->ht_node, of_phandle_ht_params);
+}
+
+static inline int of_phandle_ht_remove(struct device_node *np)
+{
+ if (!np || !np->phandle)
+ return 0;
+ return rhashtable_remove_fast(&of_phandle_ht,
+ &np->ht_node, of_phandle_ht_params);
+}
+
+static inline struct device_node *of_phandle_ht_lookup(phandle handle)
+{
+ return rhashtable_lookup_fast(&of_phandle_ht,
+ &handle, of_phandle_ht_params);
+}
+
#endif /* _LINUX_OF_PRIVATE_H */
diff --git a/include/linux/of.h b/include/linux/of.h
index 582bc45..56bb62d 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -25,6 +25,7 @@
#include <linux/notifier.h>
#include <linux/property.h>
#include <linux/list.h>
+#include <linux/rhashtable.h>
#include <asm/byteorder.h>
#include <asm/errno.h>
@@ -52,6 +53,7 @@ struct device_node {
phandle phandle;
const char *full_name;
struct fwnode_handle fwnode;
+ struct rhash_head ht_node;
struct property *properties;
struct property *deadprops; /* removed properties */
--
1.7.12
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] of: overlay: Pick up label symbols from overlays.
2016-05-09 18:11 [PATCH 0/3] of: generic infrastructure fixes Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 1/3] of: rename *_node_sysfs to _node_post Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 2/3] of: Support hashtable lookups for phandles Pantelis Antoniou
@ 2016-05-09 18:11 ` Pantelis Antoniou
2016-05-09 20:32 ` Marek Vasut
2016-05-09 21:47 ` Rob Herring
2 siblings, 2 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-09 18:11 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou, Pantelis Antoniou
Insert overlay symbols to the base tree when applied.
This makes it possible to apply an overlay that references a label
that a previously inserted overlay had.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/of/overlay.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 102 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index a274d65..a7956a2 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -514,6 +514,101 @@ static int of_free_overlay_info(struct of_overlay *ov)
return 0;
}
+static int of_overlay_add_symbols(
+ struct device_node *tree,
+ struct of_overlay *ov)
+{
+ struct of_overlay_info *ovinfo;
+ struct device_node *root_sym = NULL;
+ struct device_node *child = NULL;
+ struct property *prop;
+ const char *path, *s;
+ char *new_path;
+ int i, len, err;
+
+ /* this may fail (if no fixups are required) */
+ root_sym = of_find_node_by_path("/__symbols__");
+
+ /* do nothing if no root symbols */
+ if (!root_sym)
+ return 0;
+
+ /* locate the symbols & fixups nodes on resolve */
+ for_each_child_of_node(tree, child) {
+ if (of_node_cmp(child->name, "__symbols__") == 0)
+ goto found;
+ }
+ /* no symbols, no problem */
+ of_node_put(root_sym);
+ return 0;
+
+found:
+ err = -EINVAL;
+ for_each_property_of_node(child, prop) {
+
+ /* skip properties added automatically */
+ if (of_prop_cmp(prop->name, "name") == 0)
+ continue;
+
+ err = of_property_read_string(child,
+ prop->name, &path);
+ if (err != 0) {
+ pr_err("%s: Could not find symbol '%s'\n",
+ __func__, prop->name);
+ continue;
+ }
+
+
+ /* now find fragment index */
+ s = path;
+
+ /* compare paths to find fragment index */
+ ovinfo = NULL;
+ len = -1;
+ for (i = 0; i < ov->count; i++) {
+ ovinfo = &ov->ovinfo_tab[i];
+
+ pr_debug("%s: #%d: overlay->name=%s target->name=%s\n",
+ __func__, i, ovinfo->overlay->full_name,
+ ovinfo->target->full_name);
+
+ len = strlen(ovinfo->overlay->full_name);
+ if (strncasecmp(path, ovinfo->overlay->full_name,
+ len) == 0 && path[len] == '/')
+ break;
+ }
+
+ if (i >= ov->count)
+ continue;
+
+ pr_debug("%s: found target at #%d\n", __func__, i);
+ new_path = kasprintf(GFP_KERNEL, "%s%s",
+ ovinfo->target->full_name,
+ path + len);
+ if (!new_path) {
+ pr_err("%s: Failed to allocate propname for \"%s\"\n",
+ __func__, prop->name);
+ continue;
+ }
+
+ err = of_changeset_add_property_string(&ov->cset, root_sym,
+ prop->name, new_path);
+
+ /* free always */
+ kfree(new_path);
+
+ if (err) {
+ pr_err("%s: Failed to add property for \"%s\"\n",
+ __func__, prop->name);
+ }
+ }
+
+ of_node_put(child);
+ of_node_put(root_sym);
+
+ return 0;
+}
+
static LIST_HEAD(ov_list);
static DEFINE_IDR(ov_idr);
@@ -642,6 +737,13 @@ static int __of_overlay_create(struct device_node *tree,
goto err_abort_trans;
}
+ err = of_overlay_add_symbols(tree, ov);
+ if (err) {
+ pr_err("%s: of_overlay_add_symbols() failed for tree@%s\n",
+ __func__, tree->full_name);
+ goto err_abort_trans;
+ }
+
/* apply the changeset */
err = __of_changeset_apply(&ov->cset);
if (err) {
--
1.7.12
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] of: overlay: Pick up label symbols from overlays.
2016-05-09 18:11 ` [PATCH 3/3] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
@ 2016-05-09 20:32 ` Marek Vasut
2016-05-09 21:47 ` Rob Herring
1 sibling, 0 replies; 16+ messages in thread
From: Marek Vasut @ 2016-05-09 20:32 UTC (permalink / raw)
To: Pantelis Antoniou, Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Geert Uytterhoeven, devicetree, linux-kernel,
Pantelis Antoniou
On 05/09/2016 08:11 PM, Pantelis Antoniou wrote:
> Insert overlay symbols to the base tree when applied.
> This makes it possible to apply an overlay that references a label
> that a previously inserted overlay had.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> ---
> drivers/of/overlay.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 102 insertions(+)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index a274d65..a7956a2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -514,6 +514,101 @@ static int of_free_overlay_info(struct of_overlay *ov)
> return 0;
> }
>
> +static int of_overlay_add_symbols(
> + struct device_node *tree,
> + struct of_overlay *ov)
> +{
> + struct of_overlay_info *ovinfo;
> + struct device_node *root_sym = NULL;
> + struct device_node *child = NULL;
> + struct property *prop;
> + const char *path, *s;
> + char *new_path;
> + int i, len, err;
> +
> + /* this may fail (if no fixups are required) */
> + root_sym = of_find_node_by_path("/__symbols__");
> +
> + /* do nothing if no root symbols */
> + if (!root_sym)
> + return 0;
> +
> + /* locate the symbols & fixups nodes on resolve */
> + for_each_child_of_node(tree, child) {
> + if (of_node_cmp(child->name, "__symbols__") == 0)
> + goto found;
> + }
> + /* no symbols, no problem */
> + of_node_put(root_sym);
> + return 0;
> +
> +found:
You might want to factor this loop into a separate function instead of
the elaborate use of goto .
> + err = -EINVAL;
> + for_each_property_of_node(child, prop) {
> +
> + /* skip properties added automatically */
> + if (of_prop_cmp(prop->name, "name") == 0)
> + continue;
> +
> + err = of_property_read_string(child,
> + prop->name, &path);
> + if (err != 0) {
> + pr_err("%s: Could not find symbol '%s'\n",
> + __func__, prop->name);
It is useful to print the errno here I think.
> + continue;
> + }
> +
> +
> + /* now find fragment index */
> + s = path;
> +
> + /* compare paths to find fragment index */
> + ovinfo = NULL;
> + len = -1;
> + for (i = 0; i < ov->count; i++) {
> + ovinfo = &ov->ovinfo_tab[i];
> +
> + pr_debug("%s: #%d: overlay->name=%s target->name=%s\n",
> + __func__, i, ovinfo->overlay->full_name,
> + ovinfo->target->full_name);
> +
> + len = strlen(ovinfo->overlay->full_name);
> + if (strncasecmp(path, ovinfo->overlay->full_name,
> + len) == 0 && path[len] == '/')
> + break;
> + }
> +
> + if (i >= ov->count)
> + continue;
> +
> + pr_debug("%s: found target at #%d\n", __func__, i);
> + new_path = kasprintf(GFP_KERNEL, "%s%s",
> + ovinfo->target->full_name,
> + path + len);
> + if (!new_path) {
> + pr_err("%s: Failed to allocate propname for \"%s\"\n",
> + __func__, prop->name);
You ran out of memory here, so the pr_err() will probably also fail.
> + continue;
> + }
> +
> + err = of_changeset_add_property_string(&ov->cset, root_sym,
> + prop->name, new_path);
> +
> + /* free always */
> + kfree(new_path);
> +
> + if (err) {
> + pr_err("%s: Failed to add property for \"%s\"\n",
> + __func__, prop->name);
> + }
> + }
> +
> + of_node_put(child);
> + of_node_put(root_sym);
> +
> + return 0;
> +}
> +
> static LIST_HEAD(ov_list);
> static DEFINE_IDR(ov_idr);
>
> @@ -642,6 +737,13 @@ static int __of_overlay_create(struct device_node *tree,
> goto err_abort_trans;
> }
>
> + err = of_overlay_add_symbols(tree, ov);
> + if (err) {
> + pr_err("%s: of_overlay_add_symbols() failed for tree@%s\n",
> + __func__, tree->full_name);
> + goto err_abort_trans;
> + }
> +
> /* apply the changeset */
> err = __of_changeset_apply(&ov->cset);
> if (err) {
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-09 18:11 ` [PATCH 2/3] of: Support hashtable lookups for phandles Pantelis Antoniou
@ 2016-05-09 20:38 ` Geert Uytterhoeven
2016-05-09 21:11 ` Rob Herring
2016-05-09 21:21 ` Rob Herring
1 sibling, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2016-05-09 20:38 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Rob Herring, Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, devicetree, linux-kernel,
Pantelis Antoniou
Hi Pantelis,
On Mon, May 9, 2016 at 8:11 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
> return NULL;
>
> raw_spin_lock_irqsave(&devtree_lock, flags);
> - for_each_of_allnodes(np)
> - if (np->phandle == handle)
> - break;
> + /* when we're ready use the hash table */
> + if (of_phandle_ht_available() && !in_interrupt())
I guess the !in_interrupt() test is because of the locking inside
rhashtable_lookup_fast()?
> + np = of_phandle_ht_lookup(handle);
> + else { /* fallback */
> + for_each_of_allnodes(np)
> + if (np->phandle == handle)
> + break;
> + }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-09 20:38 ` Geert Uytterhoeven
@ 2016-05-09 21:11 ` Rob Herring
2016-05-10 13:45 ` Pantelis Antoniou
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2016-05-09 21:11 UTC (permalink / raw)
To: Geert Uytterhoeven, Pantelis Antoniou
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, devicetree, linux-kernel,
Pantelis Antoniou
On Mon, May 9, 2016 at 3:38 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Pantelis,
>
> On Mon, May 9, 2016 at 8:11 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>
>> @@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
>> return NULL;
>>
>> raw_spin_lock_irqsave(&devtree_lock, flags);
>> - for_each_of_allnodes(np)
>> - if (np->phandle == handle)
>> - break;
>> + /* when we're ready use the hash table */
>> + if (of_phandle_ht_available() && !in_interrupt())
>
> I guess the !in_interrupt() test is because of the locking inside
> rhashtable_lookup_fast()?
Not a use we should support. Just warn for anyone parsing DT in
interrupt context.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-09 18:11 ` [PATCH 2/3] of: Support hashtable lookups for phandles Pantelis Antoniou
2016-05-09 20:38 ` Geert Uytterhoeven
@ 2016-05-09 21:21 ` Rob Herring
2016-05-10 13:52 ` Pantelis Antoniou
1 sibling, 1 reply; 16+ messages in thread
From: Rob Herring @ 2016-05-09 21:21 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou
On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
Why this is needed goes here.
Any data on how much time the current code takes?
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> drivers/of/base.c | 35 ++++++++++++++++++++++++++++++++---
> drivers/of/dynamic.c | 8 ++++++++
> drivers/of/of_private.h | 31 +++++++++++++++++++++++++++++++
> include/linux/of.h | 2 ++
> 4 files changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 20bbc2f..770cb95 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -27,6 +27,7 @@
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/proc_fs.h>
> +#include <linux/rhashtable.h>
>
> #include "of_private.h"
>
> @@ -41,6 +42,16 @@ static const char *of_stdout_options;
>
> struct kset *of_kset;
>
> +const struct rhashtable_params of_phandle_ht_params = {
> + .key_offset = offsetof(struct device_node, phandle), /* base offset */
> + .key_len = sizeof(phandle),
> + .head_offset = offsetof(struct device_node, ht_node),
> + .automatic_shrinking = true,
> +};
> +
> +struct rhashtable of_phandle_ht;
> +bool of_phandle_ht_initialized;
Get rid of this and of_phandle_ht_available. It should always be initialized.
> +
> /*
> * Used to protect the of_aliases, to hold off addition of nodes to sysfs.
> * This mutex must be held whenever modifications are being made to the
> @@ -162,6 +173,12 @@ int __of_attach_node_post(struct device_node *np)
> struct property *pp;
> int rc;
>
> + if (of_phandle_ht_available()) {
> + rc = of_phandle_ht_insert(np);
> + WARN(rc, "insert to phandle hash fail @%s\n",
> + of_node_full_name(np));
> + }
> +
> if (!IS_ENABLED(CONFIG_SYSFS))
> return 0;
>
> @@ -194,6 +211,13 @@ void __init of_core_init(void)
> struct device_node *np;
> int ret;
>
> + ret = rhashtable_init(&of_phandle_ht, &of_phandle_ht_params);
> + if (ret) {
> + pr_warn("devicetree: Failed to initialize hashtable\n");
> + return;
> + }
> + of_phandle_ht_initialized = 1;
> +
> /* Create the kset, and register existing nodes */
> mutex_lock(&of_mutex);
> of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
> @@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
> return NULL;
>
> raw_spin_lock_irqsave(&devtree_lock, flags);
> - for_each_of_allnodes(np)
> - if (np->phandle == handle)
> - break;
> + /* when we're ready use the hash table */
> + if (of_phandle_ht_available() && !in_interrupt())
> + np = of_phandle_ht_lookup(handle);
> + else { /* fallback */
> + for_each_of_allnodes(np)
> + if (np->phandle == handle)
> + break;
> + }
> of_node_get(np);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
> return np;
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index eb31a55..bdebdf5 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -11,6 +11,7 @@
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/proc_fs.h>
> +#include <linux/rhashtable.h>
>
> #include "of_private.h"
>
> @@ -44,6 +45,13 @@ EXPORT_SYMBOL(of_node_put);
> void __of_detach_node_post(struct device_node *np)
> {
> struct property *pp;
> + int rc;
> +
> + if (of_phandle_ht_available()) {
> + rc = of_phandle_ht_remove(np);
> + WARN(rc, "remove from phandle hash fail @%s\n",
> + of_node_full_name(np));
Can this really fail?
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] of: overlay: Pick up label symbols from overlays.
2016-05-09 18:11 ` [PATCH 3/3] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
2016-05-09 20:32 ` Marek Vasut
@ 2016-05-09 21:47 ` Rob Herring
2016-05-10 13:56 ` Pantelis Antoniou
1 sibling, 1 reply; 16+ messages in thread
From: Rob Herring @ 2016-05-09 21:47 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou
On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Insert overlay symbols to the base tree when applied.
> This makes it possible to apply an overlay that references a label
> that a previously inserted overlay had.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> ---
> drivers/of/overlay.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 102 insertions(+)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index a274d65..a7956a2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -514,6 +514,101 @@ static int of_free_overlay_info(struct of_overlay *ov)
> return 0;
> }
>
> +static int of_overlay_add_symbols(
> + struct device_node *tree,
> + struct of_overlay *ov)
> +{
> + struct of_overlay_info *ovinfo;
> + struct device_node *root_sym = NULL;
> + struct device_node *child = NULL;
> + struct property *prop;
> + const char *path, *s;
> + char *new_path;
> + int i, len, err;
> +
> + /* this may fail (if no fixups are required) */
> + root_sym = of_find_node_by_path("/__symbols__");
> +
> + /* do nothing if no root symbols */
> + if (!root_sym)
> + return 0;
> +
> + /* locate the symbols & fixups nodes on resolve */
> + for_each_child_of_node(tree, child) {
> + if (of_node_cmp(child->name, "__symbols__") == 0)
> + goto found;
Doesn't of_get_child_by_name work here?
> + }
> + /* no symbols, no problem */
> + of_node_put(root_sym);
> + return 0;
> +
> +found:
As Marek said, get rid of the goto.
> + err = -EINVAL;
> + for_each_property_of_node(child, prop) {
> +
> + /* skip properties added automatically */
> + if (of_prop_cmp(prop->name, "name") == 0)
> + continue;
> +
> + err = of_property_read_string(child,
> + prop->name, &path);
> + if (err != 0) {
> + pr_err("%s: Could not find symbol '%s'\n",
> + __func__, prop->name);
Can you introduce and use a common pr_fmt prefix here and elsewhere
(just what you are adding).
> + continue;
> + }
> +
> +
Extra blank line.
> + /* now find fragment index */
> + s = path;
> +
> + /* compare paths to find fragment index */
> + ovinfo = NULL;
> + len = -1;
Move these into the for init.
> + for (i = 0; i < ov->count; i++) {
> + ovinfo = &ov->ovinfo_tab[i];
> +
> + pr_debug("%s: #%d: overlay->name=%s target->name=%s\n",
> + __func__, i, ovinfo->overlay->full_name,
> + ovinfo->target->full_name);
> +
> + len = strlen(ovinfo->overlay->full_name);
> + if (strncasecmp(path, ovinfo->overlay->full_name,
> + len) == 0 && path[len] == '/')
> + break;
> + }
> +
> + if (i >= ov->count)
> + continue;
> +
> + pr_debug("%s: found target at #%d\n", __func__, i);
> + new_path = kasprintf(GFP_KERNEL, "%s%s",
> + ovinfo->target->full_name,
> + path + len);
> + if (!new_path) {
> + pr_err("%s: Failed to allocate propname for \"%s\"\n",
> + __func__, prop->name);
> + continue;
If this fails, is there much point in continuing?
> + }
> +
> + err = of_changeset_add_property_string(&ov->cset, root_sym,
> + prop->name, new_path);
> +
> + /* free always */
> + kfree(new_path);
> +
> + if (err) {
No need for brackets.
> + pr_err("%s: Failed to add property for \"%s\"\n",
> + __func__, prop->name);
> + }
> + }
> +
> + of_node_put(child);
> + of_node_put(root_sym);
> +
> + return 0;
> +}
> +
> static LIST_HEAD(ov_list);
> static DEFINE_IDR(ov_idr);
>
> @@ -642,6 +737,13 @@ static int __of_overlay_create(struct device_node *tree,
> goto err_abort_trans;
> }
>
> + err = of_overlay_add_symbols(tree, ov);
> + if (err) {
> + pr_err("%s: of_overlay_add_symbols() failed for tree@%s\n",
> + __func__, tree->full_name);
> + goto err_abort_trans;
> + }
> +
> /* apply the changeset */
> err = __of_changeset_apply(&ov->cset);
> if (err) {
> --
> 1.7.12
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] of: rename *_node_sysfs to _node_post
2016-05-09 18:11 ` [PATCH 1/3] of: rename *_node_sysfs to _node_post Pantelis Antoniou
@ 2016-05-09 21:49 ` Rob Herring
2016-05-10 13:57 ` Pantelis Antoniou
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2016-05-09 21:49 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel, Pantelis Antoniou
On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
The "why" goes here.
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> drivers/of/base.c | 4 ++--
> drivers/of/dynamic.c | 10 +++++-----
> drivers/of/of_private.h | 4 ++--
> drivers/of/unittest.c | 4 ++--
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 9bbca56..20bbc2f 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -156,7 +156,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
> return rc;
> }
>
> -int __of_attach_node_sysfs(struct device_node *np)
> +int __of_attach_node_post(struct device_node *np)
> {
> const char *name;
> struct property *pp;
> @@ -203,7 +203,7 @@ void __init of_core_init(void)
> return;
> }
> for_each_of_allnodes(np)
> - __of_attach_node_sysfs(np);
> + __of_attach_node_post(np);
> mutex_unlock(&of_mutex);
>
> /* Symlink in /proc as required by userspace ABI */
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 350b1cf..eb31a55 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -41,7 +41,7 @@ void of_node_put(struct device_node *node)
> }
> EXPORT_SYMBOL(of_node_put);
>
> -void __of_detach_node_sysfs(struct device_node *np)
> +void __of_detach_node_post(struct device_node *np)
> {
> struct property *pp;
>
> @@ -251,7 +251,7 @@ int of_attach_node(struct device_node *np)
> __of_attach_node(np);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> - __of_attach_node_sysfs(np);
> + __of_attach_node_post(np);
> mutex_unlock(&of_mutex);
>
> of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
> @@ -304,7 +304,7 @@ int of_detach_node(struct device_node *np)
> __of_detach_node(np);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> - __of_detach_node_sysfs(np);
> + __of_detach_node_post(np);
> mutex_unlock(&of_mutex);
>
> of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
> @@ -628,10 +628,10 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>
> switch (ce->action) {
> case OF_RECONFIG_ATTACH_NODE:
> - __of_attach_node_sysfs(ce->np);
> + __of_attach_node_post(ce->np);
> break;
> case OF_RECONFIG_DETACH_NODE:
> - __of_detach_node_sysfs(ce->np);
> + __of_detach_node_post(ce->np);
> break;
> case OF_RECONFIG_ADD_PROPERTY:
> /* ignore duplicate names */
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 092cba7..10b0342 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -79,9 +79,9 @@ extern void __of_update_property_sysfs(struct device_node *np,
> struct property *newprop, struct property *oldprop);
>
> extern void __of_attach_node(struct device_node *np);
> -extern int __of_attach_node_sysfs(struct device_node *np);
> +extern int __of_attach_node_post(struct device_node *np);
> extern void __of_detach_node(struct device_node *np);
> -extern void __of_detach_node_sysfs(struct device_node *np);
> +extern void __of_detach_node_post(struct device_node *np);
>
> /* iterators for transactions, used for overlays */
> /* forward iterator */
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e5a5ec0..7ea3689 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -931,7 +931,7 @@ static int attach_node_and_children(struct device_node *np)
> of_node_clear_flag(np, OF_DETACHED);
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> - __of_attach_node_sysfs(np);
> + __of_attach_node_post(np);
> mutex_unlock(&of_mutex);
>
> while (child) {
> @@ -989,7 +989,7 @@ static int __init unittest_data_add(void)
> if (!of_root) {
> of_root = unittest_data_node;
> for_each_of_allnodes(np)
> - __of_attach_node_sysfs(np);
> + __of_attach_node_post(np);
> of_aliases = of_find_node_by_path("/aliases");
> of_chosen = of_find_node_by_path("/chosen");
> return 0;
> --
> 1.7.12
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-09 21:11 ` Rob Herring
@ 2016-05-10 13:45 ` Pantelis Antoniou
2016-05-10 14:26 ` Rob Herring
0 siblings, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-10 13:45 UTC (permalink / raw)
To: Rob Herring
Cc: Geert Uytterhoeven, Frank Rowand, Matt Porter, Grant Likely,
Koen Kooi, Guenter Roeck, Marek Vasut, devicetree, linux-kernel
Hi Rob,
> On May 10, 2016, at 00:11 , Rob Herring <robherring2@gmail.com> wrote:
>
> On Mon, May 9, 2016 at 3:38 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Pantelis,
>>
>> On Mon, May 9, 2016 at 8:11 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>
>>> @@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
>>> return NULL;
>>>
>>> raw_spin_lock_irqsave(&devtree_lock, flags);
>>> - for_each_of_allnodes(np)
>>> - if (np->phandle == handle)
>>> - break;
>>> + /* when we're ready use the hash table */
>>> + if (of_phandle_ht_available() && !in_interrupt())
>>
>> I guess the !in_interrupt() test is because of the locking inside
>> rhashtable_lookup_fast()?
>
> Not a use we should support. Just warn for anyone parsing DT in
> interrupt context.
>
That’s not about users calling in interrupt context. It’s when we’re
very early in the boot sequence we’re under interrupt context and
calls to the hash methods cannot be made.
> Rob
Regards
— Pantelis
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-09 21:21 ` Rob Herring
@ 2016-05-10 13:52 ` Pantelis Antoniou
0 siblings, 0 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-10 13:52 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel
Hi Rob,
> On May 10, 2016, at 00:21 , Rob Herring <robherring2@gmail.com> wrote:
>
> On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>
> Why this is needed goes here.
>
> Any data on how much time the current code takes?
>
I’ll get some numbers. The current algorithm is exhaustive search of
all nodes (not even nodes that have phandles). So it’s kind of obvious
bad on large blobs.
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/base.c | 35 ++++++++++++++++++++++++++++++++---
>> drivers/of/dynamic.c | 8 ++++++++
>> drivers/of/of_private.h | 31 +++++++++++++++++++++++++++++++
>> include/linux/of.h | 2 ++
>> 4 files changed, 73 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 20bbc2f..770cb95 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -27,6 +27,7 @@
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> #include <linux/proc_fs.h>
>> +#include <linux/rhashtable.h>
>>
>> #include "of_private.h"
>>
>> @@ -41,6 +42,16 @@ static const char *of_stdout_options;
>>
>> struct kset *of_kset;
>>
>> +const struct rhashtable_params of_phandle_ht_params = {
>> + .key_offset = offsetof(struct device_node, phandle), /* base offset */
>> + .key_len = sizeof(phandle),
>> + .head_offset = offsetof(struct device_node, ht_node),
>> + .automatic_shrinking = true,
>> +};
>> +
>> +struct rhashtable of_phandle_ht;
>> +bool of_phandle_ht_initialized;
>
> Get rid of this and of_phandle_ht_available. It should always be initialized.
>
Not that simple. We have to support the case where calls are made very early
in the boot sequence before the initialization.
>> +
>> /*
>> * Used to protect the of_aliases, to hold off addition of nodes to sysfs.
>> * This mutex must be held whenever modifications are being made to the
>> @@ -162,6 +173,12 @@ int __of_attach_node_post(struct device_node *np)
>> struct property *pp;
>> int rc;
>>
>> + if (of_phandle_ht_available()) {
>> + rc = of_phandle_ht_insert(np);
>> + WARN(rc, "insert to phandle hash fail @%s\n",
>> + of_node_full_name(np));
>> + }
>> +
>> if (!IS_ENABLED(CONFIG_SYSFS))
>> return 0;
>>
>> @@ -194,6 +211,13 @@ void __init of_core_init(void)
>> struct device_node *np;
>> int ret;
>>
>> + ret = rhashtable_init(&of_phandle_ht, &of_phandle_ht_params);
>> + if (ret) {
>> + pr_warn("devicetree: Failed to initialize hashtable\n");
>> + return;
>> + }
>> + of_phandle_ht_initialized = 1;
>> +
>> /* Create the kset, and register existing nodes */
>> mutex_lock(&of_mutex);
>> of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
>> @@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
>> return NULL;
>>
>> raw_spin_lock_irqsave(&devtree_lock, flags);
>> - for_each_of_allnodes(np)
>> - if (np->phandle == handle)
>> - break;
>> + /* when we're ready use the hash table */
>> + if (of_phandle_ht_available() && !in_interrupt())
>> + np = of_phandle_ht_lookup(handle);
>> + else { /* fallback */
>> + for_each_of_allnodes(np)
>> + if (np->phandle == handle)
>> + break;
>> + }
>> of_node_get(np);
>> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> return np;
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index eb31a55..bdebdf5 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -11,6 +11,7 @@
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> #include <linux/proc_fs.h>
>> +#include <linux/rhashtable.h>
>>
>> #include "of_private.h"
>>
>> @@ -44,6 +45,13 @@ EXPORT_SYMBOL(of_node_put);
>> void __of_detach_node_post(struct device_node *np)
>> {
>> struct property *pp;
>> + int rc;
>> +
>> + if (of_phandle_ht_available()) {
>> + rc = of_phandle_ht_remove(np);
>> + WARN(rc, "remove from phandle hash fail @%s\n",
>> + of_node_full_name(np));
>
> Can this really fail?
>
No, unless the structure is corrupted. I can move it under DEBUG.
> Rob
> —
Regards
— Pantelis
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] of: overlay: Pick up label symbols from overlays.
2016-05-09 21:47 ` Rob Herring
@ 2016-05-10 13:56 ` Pantelis Antoniou
2016-05-10 14:23 ` Rob Herring
0 siblings, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-10 13:56 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel
Hi Rob,
> On May 10, 2016, at 00:47 , Rob Herring <robherring2@gmail.com> wrote:
>
> On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Insert overlay symbols to the base tree when applied.
>> This makes it possible to apply an overlay that references a label
>> that a previously inserted overlay had.
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> ---
>> drivers/of/overlay.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 102 insertions(+)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index a274d65..a7956a2 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -514,6 +514,101 @@ static int of_free_overlay_info(struct of_overlay *ov)
>> return 0;
>> }
>>
>> +static int of_overlay_add_symbols(
>> + struct device_node *tree,
>> + struct of_overlay *ov)
>> +{
>> + struct of_overlay_info *ovinfo;
>> + struct device_node *root_sym = NULL;
>> + struct device_node *child = NULL;
>> + struct property *prop;
>> + const char *path, *s;
>> + char *new_path;
>> + int i, len, err;
>> +
>> + /* this may fail (if no fixups are required) */
>> + root_sym = of_find_node_by_path("/__symbols__");
>> +
>> + /* do nothing if no root symbols */
>> + if (!root_sym)
>> + return 0;
>> +
>> + /* locate the symbols & fixups nodes on resolve */
>> + for_each_child_of_node(tree, child) {
>> + if (of_node_cmp(child->name, "__symbols__") == 0)
>> + goto found;
>
> Doesn't of_get_child_by_name work here?
>
No, we’re holding of_mutex.
>> + }
>> + /* no symbols, no problem */
>> + of_node_put(root_sym);
>> + return 0;
>> +
>> +found:
>
> As Marek said, get rid of the goto.
>
>> + err = -EINVAL;
>> + for_each_property_of_node(child, prop) {
>> +
>> + /* skip properties added automatically */
>> + if (of_prop_cmp(prop->name, "name") == 0)
>> + continue;
>> +
>> + err = of_property_read_string(child,
>> + prop->name, &path);
>> + if (err != 0) {
>> + pr_err("%s: Could not find symbol '%s'\n",
>> + __func__, prop->name);
>
> Can you introduce and use a common pr_fmt prefix here and elsewhere
> (just what you are adding).
OK
>
>> + continue;
>> + }
>> +
>> +
>
> Extra blank line.
>
>> + /* now find fragment index */
>> + s = path;
>> +
>> + /* compare paths to find fragment index */
>> + ovinfo = NULL;
>> + len = -1;
>
> Move these into the for init.
>
>> + for (i = 0; i < ov->count; i++) {
>> + ovinfo = &ov->ovinfo_tab[i];
>> +
>> + pr_debug("%s: #%d: overlay->name=%s target->name=%s\n",
>> + __func__, i, ovinfo->overlay->full_name,
>> + ovinfo->target->full_name);
>> +
>> + len = strlen(ovinfo->overlay->full_name);
>> + if (strncasecmp(path, ovinfo->overlay->full_name,
>> + len) == 0 && path[len] == '/')
>> + break;
>> + }
>> +
>> + if (i >= ov->count)
>> + continue;
>> +
>> + pr_debug("%s: found target at #%d\n", __func__, i);
>> + new_path = kasprintf(GFP_KERNEL, "%s%s",
>> + ovinfo->target->full_name,
>> + path + len);
>> + if (!new_path) {
>> + pr_err("%s: Failed to allocate propname for \"%s\"\n",
>> + __func__, prop->name);
>> + continue;
>
> If this fails, is there much point in continuing?
>
No
>> + }
>> +
>> + err = of_changeset_add_property_string(&ov->cset, root_sym,
>> + prop->name, new_path);
>> +
>> + /* free always */
>> + kfree(new_path);
>> +
>> + if (err) {
>
> No need for brackets.
>
>> + pr_err("%s: Failed to add property for \"%s\"\n",
>> + __func__, prop->name);
>> + }
>> + }
>> +
>> + of_node_put(child);
>> + of_node_put(root_sym);
>> +
>> + return 0;
>> +}
>> +
>> static LIST_HEAD(ov_list);
>> static DEFINE_IDR(ov_idr);
>>
>> @@ -642,6 +737,13 @@ static int __of_overlay_create(struct device_node *tree,
>> goto err_abort_trans;
>> }
>>
>> + err = of_overlay_add_symbols(tree, ov);
>> + if (err) {
>> + pr_err("%s: of_overlay_add_symbols() failed for tree@%s\n",
>> + __func__, tree->full_name);
>> + goto err_abort_trans;
>> + }
>> +
>> /* apply the changeset */
>> err = __of_changeset_apply(&ov->cset);
>> if (err) {
>> --
>> 1.7.12
>>
Regards
— Pantelis
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] of: rename *_node_sysfs to _node_post
2016-05-09 21:49 ` Rob Herring
@ 2016-05-10 13:57 ` Pantelis Antoniou
0 siblings, 0 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2016-05-10 13:57 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel
Hi Rob,
> On May 10, 2016, at 00:49 , Rob Herring <robherring2@gmail.com> wrote:
>
> On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>
> The "why" goes here.
>
OK.
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/base.c | 4 ++--
>> drivers/of/dynamic.c | 10 +++++-----
>> drivers/of/of_private.h | 4 ++--
>> drivers/of/unittest.c | 4 ++--
>> 4 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 9bbca56..20bbc2f 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -156,7 +156,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>> return rc;
>> }
>>
>> -int __of_attach_node_sysfs(struct device_node *np)
>> +int __of_attach_node_post(struct device_node *np)
>> {
>> const char *name;
>> struct property *pp;
>> @@ -203,7 +203,7 @@ void __init of_core_init(void)
>> return;
>> }
>> for_each_of_allnodes(np)
>> - __of_attach_node_sysfs(np);
>> + __of_attach_node_post(np);
>> mutex_unlock(&of_mutex);
>>
>> /* Symlink in /proc as required by userspace ABI */
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 350b1cf..eb31a55 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -41,7 +41,7 @@ void of_node_put(struct device_node *node)
>> }
>> EXPORT_SYMBOL(of_node_put);
>>
>> -void __of_detach_node_sysfs(struct device_node *np)
>> +void __of_detach_node_post(struct device_node *np)
>> {
>> struct property *pp;
>>
>> @@ -251,7 +251,7 @@ int of_attach_node(struct device_node *np)
>> __of_attach_node(np);
>> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>
>> - __of_attach_node_sysfs(np);
>> + __of_attach_node_post(np);
>> mutex_unlock(&of_mutex);
>>
>> of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
>> @@ -304,7 +304,7 @@ int of_detach_node(struct device_node *np)
>> __of_detach_node(np);
>> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>
>> - __of_detach_node_sysfs(np);
>> + __of_detach_node_post(np);
>> mutex_unlock(&of_mutex);
>>
>> of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
>> @@ -628,10 +628,10 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>>
>> switch (ce->action) {
>> case OF_RECONFIG_ATTACH_NODE:
>> - __of_attach_node_sysfs(ce->np);
>> + __of_attach_node_post(ce->np);
>> break;
>> case OF_RECONFIG_DETACH_NODE:
>> - __of_detach_node_sysfs(ce->np);
>> + __of_detach_node_post(ce->np);
>> break;
>> case OF_RECONFIG_ADD_PROPERTY:
>> /* ignore duplicate names */
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 092cba7..10b0342 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -79,9 +79,9 @@ extern void __of_update_property_sysfs(struct device_node *np,
>> struct property *newprop, struct property *oldprop);
>>
>> extern void __of_attach_node(struct device_node *np);
>> -extern int __of_attach_node_sysfs(struct device_node *np);
>> +extern int __of_attach_node_post(struct device_node *np);
>> extern void __of_detach_node(struct device_node *np);
>> -extern void __of_detach_node_sysfs(struct device_node *np);
>> +extern void __of_detach_node_post(struct device_node *np);
>>
>> /* iterators for transactions, used for overlays */
>> /* forward iterator */
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index e5a5ec0..7ea3689 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -931,7 +931,7 @@ static int attach_node_and_children(struct device_node *np)
>> of_node_clear_flag(np, OF_DETACHED);
>> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>
>> - __of_attach_node_sysfs(np);
>> + __of_attach_node_post(np);
>> mutex_unlock(&of_mutex);
>>
>> while (child) {
>> @@ -989,7 +989,7 @@ static int __init unittest_data_add(void)
>> if (!of_root) {
>> of_root = unittest_data_node;
>> for_each_of_allnodes(np)
>> - __of_attach_node_sysfs(np);
>> + __of_attach_node_post(np);
>> of_aliases = of_find_node_by_path("/aliases");
>> of_chosen = of_find_node_by_path("/chosen");
>> return 0;
>> --
>> 1.7.12
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] of: overlay: Pick up label symbols from overlays.
2016-05-10 13:56 ` Pantelis Antoniou
@ 2016-05-10 14:23 ` Rob Herring
0 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2016-05-10 14:23 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, Geert Uytterhoeven, devicetree,
linux-kernel
On Tue, May 10, 2016 at 8:56 AM, Pantelis Antoniou
<panto@antoniou-consulting.com> wrote:
> Hi Rob,
>
>> On May 10, 2016, at 00:47 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Insert overlay symbols to the base tree when applied.
>>> This makes it possible to apply an overlay that references a label
>>> that a previously inserted overlay had.
[...]
>>> + /* locate the symbols & fixups nodes on resolve */
>>> + for_each_child_of_node(tree, child) {
>>> + if (of_node_cmp(child->name, "__symbols__") == 0)
>>> + goto found;
>>
>> Doesn't of_get_child_by_name work here?
>>
>
> No, we’re holding of_mutex.
So. Go look at the function.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] of: Support hashtable lookups for phandles
2016-05-10 13:45 ` Pantelis Antoniou
@ 2016-05-10 14:26 ` Rob Herring
0 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2016-05-10 14:26 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Geert Uytterhoeven, Frank Rowand, Matt Porter, Grant Likely,
Koen Kooi, Guenter Roeck, Marek Vasut, devicetree, linux-kernel
On Tue, May 10, 2016 at 8:45 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On May 10, 2016, at 00:11 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Mon, May 9, 2016 at 3:38 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Hi Pantelis,
>>>
>>> On Mon, May 9, 2016 at 8:11 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>
>>>> @@ -1073,9 +1097,14 @@ struct device_node *of_find_node_by_phandle(phandle handle)
>>>> return NULL;
>>>>
>>>> raw_spin_lock_irqsave(&devtree_lock, flags);
>>>> - for_each_of_allnodes(np)
>>>> - if (np->phandle == handle)
>>>> - break;
>>>> + /* when we're ready use the hash table */
>>>> + if (of_phandle_ht_available() && !in_interrupt())
>>>
>>> I guess the !in_interrupt() test is because of the locking inside
>>> rhashtable_lookup_fast()?
>>
>> Not a use we should support. Just warn for anyone parsing DT in
>> interrupt context.
>>
>
> That’s not about users calling in interrupt context. It’s when we’re
> very early in the boot sequence we’re under interrupt context and
> calls to the hash methods cannot be made.
I don't understand. When exactly are we in interrupt context?
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-05-10 14:26 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-09 18:11 [PATCH 0/3] of: generic infrastructure fixes Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 1/3] of: rename *_node_sysfs to _node_post Pantelis Antoniou
2016-05-09 21:49 ` Rob Herring
2016-05-10 13:57 ` Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 2/3] of: Support hashtable lookups for phandles Pantelis Antoniou
2016-05-09 20:38 ` Geert Uytterhoeven
2016-05-09 21:11 ` Rob Herring
2016-05-10 13:45 ` Pantelis Antoniou
2016-05-10 14:26 ` Rob Herring
2016-05-09 21:21 ` Rob Herring
2016-05-10 13:52 ` Pantelis Antoniou
2016-05-09 18:11 ` [PATCH 3/3] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
2016-05-09 20:32 ` Marek Vasut
2016-05-09 21:47 ` Rob Herring
2016-05-10 13:56 ` Pantelis Antoniou
2016-05-10 14:23 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).