* [PATCH v2 1/5] of: rename *_node_sysfs to _node_post
2016-05-16 16:52 [PATCH v2 0/5] of: generic infrastructure fixes Pantelis Antoniou
@ 2016-05-16 16:52 ` Pantelis Antoniou
2016-05-16 16:52 ` [PATCH v2 2/5] of: Support hashtable lookups for phandles Pantelis Antoniou
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 16: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, Pantelis Antoniou, Pantelis Antoniou
Renames the *_node_sysfs methods to _node_post which is more accurate
when more work takes place besides sysfs tweaking (as in with
phandle hash management).
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 b07fec2..34ca783 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] 14+ messages in thread
* [PATCH v2 2/5] of: Support hashtable lookups for phandles
2016-05-16 16:52 [PATCH v2 0/5] of: generic infrastructure fixes Pantelis Antoniou
2016-05-16 16:52 ` [PATCH v2 1/5] of: rename *_node_sysfs to _node_post Pantelis Antoniou
@ 2016-05-16 16:52 ` Pantelis Antoniou
2016-05-16 19:37 ` Rob Herring
2016-05-16 16:52 ` [PATCH v2 3/5] of: unittest: hashed phandles unitest Pantelis Antoniou
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 16: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, Pantelis Antoniou, Pantelis Antoniou
When a device tree contains a lot of phandles, resolving one
takes time because the original method uses a search against
all nodes (not just the ones with phandles).
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
drivers/of/base.c | 41 ++++++++++++++++++++++++++++++++++++++---
drivers/of/dynamic.c | 8 ++++++++
drivers/of/of_private.h | 33 +++++++++++++++++++++++++++++++++
include/linux/of.h | 2 ++
4 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20bbc2f..436a088 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,18 @@ 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;
+
+/* default is false */
+bool of_phandle_ht_is_disabled;
+
/*
* 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 +175,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 +213,17 @@ void __init of_core_init(void)
struct device_node *np;
int ret;
+ of_phandle_ht = kzalloc(sizeof(*of_phandle_ht), GFP_KERNEL);
+ if (!of_phandle_ht) {
+ pr_warn("devicetree: Failed to allocate hashtable\n");
+ return;
+ }
+ ret = rhashtable_init(of_phandle_ht, &of_phandle_ht_params);
+ if (ret) {
+ pr_warn("devicetree: Failed to initialize hashtable\n");
+ return;
+ }
+
/* Create the kset, and register existing nodes */
mutex_lock(&of_mutex);
of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
@@ -1073,9 +1103,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 (and not disabled) */
+ if (of_phandle_ht_available() && !of_phandle_ht_is_disabled)
+ 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 34ca783..931b905 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..386ae71 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -101,4 +101,37 @@ static inline int of_overlay_init(void)
}
#endif
+extern const struct rhashtable_params of_phandle_ht_params;
+extern struct rhashtable *of_phandle_ht;
+
+/* for unittest use */
+extern bool of_phandle_ht_is_disabled;
+
+static inline bool of_phandle_ht_available(void)
+{
+ return of_phandle_ht != NULL;
+}
+
+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 18a8e59..9de9a3f 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] 14+ messages in thread
* Re: [PATCH v2 2/5] of: Support hashtable lookups for phandles
2016-05-16 16:52 ` [PATCH v2 2/5] of: Support hashtable lookups for phandles Pantelis Antoniou
@ 2016-05-16 19:37 ` Rob Herring
2016-05-16 20:13 ` Pantelis Antoniou
0 siblings, 1 reply; 14+ messages in thread
From: Rob Herring @ 2016-05-16 19:37 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 16, 2016 at 11:52 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> When a device tree contains a lot of phandles, resolving one
> takes time because the original method uses a search against
> all nodes (not just the ones with phandles).
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> drivers/of/base.c | 41 ++++++++++++++++++++++++++++++++++++++---
> drivers/of/dynamic.c | 8 ++++++++
> drivers/of/of_private.h | 33 +++++++++++++++++++++++++++++++++
> include/linux/of.h | 2 ++
> 4 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 20bbc2f..436a088 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,18 @@ static const char *of_stdout_options;
>
> struct kset *of_kset;
>
> +const struct rhashtable_params of_phandle_ht_params = {
static
> + .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;
> +
> +/* default is false */
> +bool of_phandle_ht_is_disabled;
I think this should go. The unittest alone is not enough reason to keep.
> +
> /*
> * 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 +175,12 @@ int __of_attach_node_post(struct device_node *np)
> struct property *pp;
> int rc;
>
> + if (of_phandle_ht_available()) {
This can never be false as this function should only get called after init.
> + 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 +213,17 @@ void __init of_core_init(void)
> struct device_node *np;
> int ret;
>
> + of_phandle_ht = kzalloc(sizeof(*of_phandle_ht), GFP_KERNEL);
> + if (!of_phandle_ht) {
> + pr_warn("devicetree: Failed to allocate hashtable\n");
> + return;
> + }
> + ret = rhashtable_init(of_phandle_ht, &of_phandle_ht_params);
> + if (ret) {
> + pr_warn("devicetree: Failed to initialize hashtable\n");
> + return;
> + }
> +
> /* Create the kset, and register existing nodes */
> mutex_lock(&of_mutex);
> of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
> @@ -1073,9 +1103,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 (and not disabled) */
> + if (of_phandle_ht_available() && !of_phandle_ht_is_disabled)
Just "if (of_phandle_ht)"
> + 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 34ca783..931b905 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()) {
Likewise, this can't be false either.
> + 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..386ae71 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -101,4 +101,37 @@ static inline int of_overlay_init(void)
> }
> #endif
>
> +extern const struct rhashtable_params of_phandle_ht_params;
> +extern struct rhashtable *of_phandle_ht;
> +
> +/* for unittest use */
> +extern bool of_phandle_ht_is_disabled;
> +
> +static inline bool of_phandle_ht_available(void)
> +{
> + return of_phandle_ht != NULL;
> +}
> +
> +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 18a8e59..9de9a3f 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;
Place next to .phandle as both will be accessed at the same time.
>
> struct property *properties;
> struct property *deadprops; /* removed properties */
> --
> 1.7.12
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/5] of: Support hashtable lookups for phandles
2016-05-16 19:37 ` Rob Herring
@ 2016-05-16 20:13 ` Pantelis Antoniou
0 siblings, 0 replies; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 20:13 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 16, 2016, at 22:37 , Rob Herring <robherring2@gmail.com> wrote:
>
> On Mon, May 16, 2016 at 11:52 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> When a device tree contains a lot of phandles, resolving one
>> takes time because the original method uses a search against
>> all nodes (not just the ones with phandles).
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/base.c | 41 ++++++++++++++++++++++++++++++++++++++---
>> drivers/of/dynamic.c | 8 ++++++++
>> drivers/of/of_private.h | 33 +++++++++++++++++++++++++++++++++
>> include/linux/of.h | 2 ++
>> 4 files changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 20bbc2f..436a088 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,18 @@ static const char *of_stdout_options;
>>
>> struct kset *of_kset;
>>
>> +const struct rhashtable_params of_phandle_ht_params = {
>
> static
>
No, it won’t work. The rhashtable API requires using this at every call.
>> + .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;
>> +
>> +/* default is false */
>> +bool of_phandle_ht_is_disabled;
>
> I think this should go. The unittest alone is not enough reason to keep.
>
Hmm, OK.
>> +
>> /*
>> * 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 +175,12 @@ int __of_attach_node_post(struct device_node *np)
>> struct property *pp;
>> int rc;
>>
>> + if (of_phandle_ht_available()) {
>
> This can never be false as this function should only get called after init.
>
How about a BUG_ON() just in case something weird is going on?
>> + 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 +213,17 @@ void __init of_core_init(void)
>> struct device_node *np;
>> int ret;
>>
>> + of_phandle_ht = kzalloc(sizeof(*of_phandle_ht), GFP_KERNEL);
>> + if (!of_phandle_ht) {
>> + pr_warn("devicetree: Failed to allocate hashtable\n");
>> + return;
>> + }
>> + ret = rhashtable_init(of_phandle_ht, &of_phandle_ht_params);
>> + if (ret) {
>> + pr_warn("devicetree: Failed to initialize hashtable\n");
>> + return;
>> + }
>> +
>> /* Create the kset, and register existing nodes */
>> mutex_lock(&of_mutex);
>> of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
>> @@ -1073,9 +1103,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 (and not disabled) */
>> + if (of_phandle_ht_available() && !of_phandle_ht_is_disabled)
>
> Just "if (of_phandle_ht)”
>
OK
>> + 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 34ca783..931b905 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()) {
>
> Likewise, this can't be false either.
>
OK
>> + 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..386ae71 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -101,4 +101,37 @@ static inline int of_overlay_init(void)
>> }
>> #endif
>>
>> +extern const struct rhashtable_params of_phandle_ht_params;
>> +extern struct rhashtable *of_phandle_ht;
>> +
>> +/* for unittest use */
>> +extern bool of_phandle_ht_is_disabled;
>> +
>> +static inline bool of_phandle_ht_available(void)
>> +{
>> + return of_phandle_ht != NULL;
>> +}
>> +
>> +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 18a8e59..9de9a3f 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;
>
> Place next to .phandle as both will be accessed at the same time.
>
OK.
>>
>> struct property *properties;
>> struct property *deadprops; /* removed properties */
>> --
>> 1.7.12
Regards
— Pantelis
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] of: unittest: hashed phandles unitest
2016-05-16 16:52 [PATCH v2 0/5] of: generic infrastructure fixes Pantelis Antoniou
2016-05-16 16:52 ` [PATCH v2 1/5] of: rename *_node_sysfs to _node_post Pantelis Antoniou
2016-05-16 16:52 ` [PATCH v2 2/5] of: Support hashtable lookups for phandles Pantelis Antoniou
@ 2016-05-16 16:52 ` Pantelis Antoniou
2016-05-16 19:04 ` Geert Uytterhoeven
2016-05-16 19:38 ` Rob Herring
2016-05-16 16:52 ` [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
2016-05-16 16:52 ` [PATCH v2 5/5] of: overlay: Add pr_fmt for clarity Pantelis Antoniou
4 siblings, 2 replies; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 16: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, Pantelis Antoniou, Pantelis Antoniou
Add a benchmarking hashed phandles unittest which report what kind
of speed up we get switching to hashed phandle lookups.
### dt-test ### the hash method is 8.2 times faster than the original
On the beaglebone we perform about 1877 phandle lookups until that
point in the unittest. Each non-hashed lookup takes about 23us when
the cash is hot, while the hash lookup takes about 3us.
For those 1877 lookup we get a speedup in the boot sequence of
1877 * (23 - 3) = 37.5ms, which is not spectacular but there's no
point in wasting cycles and energy.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
drivers/of/unittest.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 7ea3689..59cad84 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -25,6 +25,9 @@
#include <linux/bitops.h>
+#include <linux/timekeeping.h>
+#include <linux/random.h>
+
#include "of_private.h"
static struct unittest_results {
@@ -2266,6 +2269,70 @@ out:
static inline void __init of_unittest_overlay(void) { }
#endif
+#define PHANDLE_LOOKUPS 1000
+
+static void __init of_unittest_phandle_hash(void)
+{
+ struct device_node *node;
+ phandle max_phandle;
+ u32 ph;
+ unsigned long flags;
+ int i, j, total;
+ ktime_t start, end;
+ s64 dur[2];
+ int dec, frac;
+
+ /* test only available when hashing is available */
+ if (!of_phandle_ht_available()) {
+ pr_warn("phandle hash test requires hash to be initialized\n");
+ return;
+ }
+
+ /* find the maximum phandle of the tree */
+ raw_spin_lock_irqsave(&devtree_lock, flags);
+ max_phandle = 0;
+ total = 0;
+ for_each_of_allnodes(node) {
+ if (node->phandle != (phandle)-1U &&
+ node->phandle > max_phandle)
+ max_phandle = node->phandle;
+ total++;
+ }
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
+ max_phandle++;
+
+ pr_debug("phandle: max-phandle #%u, #%d total nodes\n",
+ (u32)max_phandle, total);
+
+ /* perform random lookups using the hash */
+ for (j = 0; j < 2; j++) {
+
+ /* disabled for pass #0, enabled for pass #1 */
+ of_phandle_ht_is_disabled = j == 0;
+
+ start = ktime_get_raw();
+ for (i = 0; i < PHANDLE_LOOKUPS; i++) {
+ ph = prandom_u32() % max_phandle;
+ node = of_find_node_by_phandle(ph);
+ of_node_put(node);
+ }
+ end = ktime_get_raw();
+
+ dur[j] = ktime_to_us(end) - ktime_to_us(start);
+ pr_debug("#%d lookups in %lld us (%s)\n",
+ PHANDLE_LOOKUPS, dur[j],
+ j == 0 ? "original" : "hashed");
+ }
+
+ unittest(dur[0] > dur[1], "Non hashing phandles are faster!?");
+
+ dec = (int)div64_s64(dur[0] * 10 + 5, dur[1]);
+ frac = dec % 10;
+ dec /= 10;
+ pr_info("the hash method is %d.%d times faster than the original\n",
+ dec, frac);
+}
+
static int __init of_unittest(void)
{
struct device_node *np;
@@ -2300,6 +2367,7 @@ static int __init of_unittest(void)
of_unittest_match_node();
of_unittest_platform_populate();
of_unittest_overlay();
+ of_unittest_phandle_hash();
/* Double check linkage after removing testcase data */
of_unittest_check_tree_linkage();
--
1.7.12
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/5] of: unittest: hashed phandles unitest
2016-05-16 16:52 ` [PATCH v2 3/5] of: unittest: hashed phandles unitest Pantelis Antoniou
@ 2016-05-16 19:04 ` Geert Uytterhoeven
2016-05-16 19:38 ` Rob Herring
1 sibling, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2016-05-16 19:04 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
On Mon, May 16, 2016 at 6:52 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Add a benchmarking hashed phandles unittest which report what kind
> of speed up we get switching to hashed phandle lookups.
>
> ### dt-test ### the hash method is 8.2 times faster than the original
>
> On the beaglebone we perform about 1877 phandle lookups until that
> point in the unittest. Each non-hashed lookup takes about 23us when
> the cash is hot, while the hash lookup takes about 3us.
cache
> For those 1877 lookup we get a speedup in the boot sequence of
> 1877 * (23 - 3) = 37.5ms, which is not spectacular but there's no
> point in wasting cycles and energy.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> drivers/of/unittest.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 7ea3689..59cad84 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -25,6 +25,9 @@
>
> #include <linux/bitops.h>
>
> +#include <linux/timekeeping.h>
> +#include <linux/random.h>
> +
> #include "of_private.h"
>
> static struct unittest_results {
> @@ -2266,6 +2269,70 @@ out:
> static inline void __init of_unittest_overlay(void) { }
> #endif
>
> +#define PHANDLE_LOOKUPS 1000
> +
> +static void __init of_unittest_phandle_hash(void)
> +{
> + struct device_node *node;
> + phandle max_phandle;
> + u32 ph;
> + unsigned long flags;
> + int i, j, total;
unsigned int
> + ktime_t start, end;
> + s64 dur[2];
No idea why ktime_to_us() returns s64 i.s.o. u64...
> + int dec, frac;
unsigned int?
> + /* test only available when hashing is available */
> + if (!of_phandle_ht_available()) {
> + pr_warn("phandle hash test requires hash to be initialized\n");
> + return;
> + }
> +
> + /* find the maximum phandle of the tree */
> + raw_spin_lock_irqsave(&devtree_lock, flags);
> + max_phandle = 0;
> + total = 0;
> + for_each_of_allnodes(node) {
> + if (node->phandle != (phandle)-1U &&
Drop the "U" suffix?
> + node->phandle > max_phandle)
> + max_phandle = node->phandle;
> + total++;
> + }
> + raw_spin_unlock_irqrestore(&devtree_lock, flags);
> + max_phandle++;
> +
> + pr_debug("phandle: max-phandle #%u, #%d total nodes\n",
> + (u32)max_phandle, total);
phandle is already u32, so no need for the cast.
> +
> + /* perform random lookups using the hash */
> + for (j = 0; j < 2; j++) {
> +
> + /* disabled for pass #0, enabled for pass #1 */
> + of_phandle_ht_is_disabled = j == 0;
> +
> + start = ktime_get_raw();
> + for (i = 0; i < PHANDLE_LOOKUPS; i++) {
> + ph = prandom_u32() % max_phandle;
> + node = of_find_node_by_phandle(ph);
> + of_node_put(node);
> + }
> + end = ktime_get_raw();
> +
> + dur[j] = ktime_to_us(end) - ktime_to_us(start);
> + pr_debug("#%d lookups in %lld us (%s)\n",
$u
> + PHANDLE_LOOKUPS, dur[j],
> + j == 0 ? "original" : "hashed");
> + }
> +
> + unittest(dur[0] > dur[1], "Non hashing phandles are faster!?");
> +
> + dec = (int)div64_s64(dur[0] * 10 + 5, dur[1]);
I'd expect div64_u64(), if not for ktime_to_us() returning s64...
> + frac = dec % 10;
> + dec /= 10;
> + pr_info("the hash method is %d.%d times faster than the original\n",
%u.%u once dec and frac are unsigned.
> + dec, frac);
> +}
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] 14+ messages in thread
* Re: [PATCH v2 3/5] of: unittest: hashed phandles unitest
2016-05-16 16:52 ` [PATCH v2 3/5] of: unittest: hashed phandles unitest Pantelis Antoniou
2016-05-16 19:04 ` Geert Uytterhoeven
@ 2016-05-16 19:38 ` Rob Herring
1 sibling, 0 replies; 14+ messages in thread
From: Rob Herring @ 2016-05-16 19:38 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 16, 2016 at 11:52 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Add a benchmarking hashed phandles unittest which report what kind
> of speed up we get switching to hashed phandle lookups.
>
> ### dt-test ### the hash method is 8.2 times faster than the original
>
> On the beaglebone we perform about 1877 phandle lookups until that
> point in the unittest. Each non-hashed lookup takes about 23us when
> the cash is hot, while the hash lookup takes about 3us.
>
> For those 1877 lookup we get a speedup in the boot sequence of
> 1877 * (23 - 3) = 37.5ms, which is not spectacular but there's no
> point in wasting cycles and energy.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> drivers/of/unittest.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 7ea3689..59cad84 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -25,6 +25,9 @@
>
> #include <linux/bitops.h>
>
> +#include <linux/timekeeping.h>
> +#include <linux/random.h>
> +
> #include "of_private.h"
>
> static struct unittest_results {
> @@ -2266,6 +2269,70 @@ out:
> static inline void __init of_unittest_overlay(void) { }
> #endif
>
> +#define PHANDLE_LOOKUPS 1000
> +
> +static void __init of_unittest_phandle_hash(void)
> +{
> + struct device_node *node;
> + phandle max_phandle;
> + u32 ph;
> + unsigned long flags;
> + int i, j, total;
> + ktime_t start, end;
> + s64 dur[2];
> + int dec, frac;
> +
> + /* test only available when hashing is available */
> + if (!of_phandle_ht_available()) {
> + pr_warn("phandle hash test requires hash to be initialized\n");
> + return;
As the point of the unittest is to test the core DT code, this should
be a test fail.
> + }
> +
> + /* find the maximum phandle of the tree */
> + raw_spin_lock_irqsave(&devtree_lock, flags);
> + max_phandle = 0;
> + total = 0;
> + for_each_of_allnodes(node) {
> + if (node->phandle != (phandle)-1U &&
> + node->phandle > max_phandle)
> + max_phandle = node->phandle;
> + total++;
> + }
> + raw_spin_unlock_irqrestore(&devtree_lock, flags);
> + max_phandle++;
> +
> + pr_debug("phandle: max-phandle #%u, #%d total nodes\n",
> + (u32)max_phandle, total);
> +
> + /* perform random lookups using the hash */
> + for (j = 0; j < 2; j++) {
> +
> + /* disabled for pass #0, enabled for pass #1 */
> + of_phandle_ht_is_disabled = j == 0;
I'm not wild about having this variable leaked from the core code just
for the unit test. Yet another step away from the unittest being a
module.
I think you should just measure current performance and users can
revert the hash table if they want to measure the slow path.
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays.
2016-05-16 16:52 [PATCH v2 0/5] of: generic infrastructure fixes Pantelis Antoniou
` (2 preceding siblings ...)
2016-05-16 16:52 ` [PATCH v2 3/5] of: unittest: hashed phandles unitest Pantelis Antoniou
@ 2016-05-16 16:52 ` Pantelis Antoniou
2016-05-16 19:06 ` Geert Uytterhoeven
2016-05-16 16:52 ` [PATCH v2 5/5] of: overlay: Add pr_fmt for clarity Pantelis Antoniou
4 siblings, 1 reply; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 16: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, 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>
---
drivers/of/overlay.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index a274d65..89ebb70 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -8,6 +8,7 @@
* modify it under the terms of the GNU General Public License
* version 2 as published by the Free Software Foundation.
*/
+
#undef DEBUG
#include <linux/kernel.h>
#include <linux/module.h>
@@ -514,6 +515,91 @@ 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;
+
+ /* both may fail (if no fixups are required) */
+ root_sym = of_find_node_by_path("/__symbols__");
+ child = of_get_child_by_name(tree, "__symbols__");
+
+ err = 0;
+ /* do nothing if either is NULL */
+ if (!root_sym || !child)
+ goto out;
+
+ 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("Could not find symbol '%s'\n", prop->name);
+ continue;
+ }
+
+ /* now find fragment index */
+ s = path;
+
+ /* compare paths to find fragment index */
+ for (i = 0, ovinfo = NULL, len = -1; i < ov->count; i++) {
+ ovinfo = &ov->ovinfo_tab[i];
+
+ pr_debug("#%d: overlay->name=%s target->name=%s\n",
+ 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("found target at #%d\n", i);
+ new_path = kasprintf(GFP_KERNEL, "%s%s",
+ ovinfo->target->full_name,
+ path + len);
+ if (!new_path) {
+ pr_err("Failed to allocate propname for \"%s\"\n",
+ prop->name);
+ err = -ENOMEM;
+ break;
+ }
+
+ err = of_changeset_add_property_string(&ov->cset, root_sym,
+ prop->name, new_path);
+
+ /* free always */
+ kfree(new_path);
+
+ if (err) {
+ pr_err("Failed to add property for \"%s\"\n",
+ prop->name);
+ break;
+ }
+ }
+
+out:
+ of_node_put(child);
+ of_node_put(root_sym);
+
+ return err;
+}
+
static LIST_HEAD(ov_list);
static DEFINE_IDR(ov_idr);
@@ -642,6 +728,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] 14+ messages in thread
* Re: [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays.
2016-05-16 16:52 ` [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
@ 2016-05-16 19:06 ` Geert Uytterhoeven
2016-05-16 19:27 ` Pantelis Antoniou
0 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2016-05-16 19:06 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
On Mon, May 16, 2016 at 6:52 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>
This patch hasn't changed, so I think you can keep my
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
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] 14+ messages in thread
* Re: [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays.
2016-05-16 19:06 ` Geert Uytterhoeven
@ 2016-05-16 19:27 ` Pantelis Antoniou
2016-05-16 19:40 ` Geert Uytterhoeven
0 siblings, 1 reply; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 19:27 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rob Herring, Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, devicetree, linux-kernel
Hi Geert,
> On May 16, 2016, at 22:06 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Mon, May 16, 2016 at 6:52 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>
>
> This patch hasn't changed, so I think you can keep my
It’s been tweaked slightly that’s why I dropped your tested-by
signoff. Could you test this version again please to verify it
works for you?
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
> Geert
>
Regards
— Pantelis
> --
> 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] 14+ messages in thread
* Re: [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays.
2016-05-16 19:27 ` Pantelis Antoniou
@ 2016-05-16 19:40 ` Geert Uytterhoeven
2016-05-17 6:36 ` Geert Uytterhoeven
0 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2016-05-16 19:40 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Rob Herring, Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, devicetree, linux-kernel
Hi Pantelis,
On Mon, May 16, 2016 at 9:27 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
>> On May 16, 2016, at 22:06 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, May 16, 2016 at 6:52 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>
>>
>> This patch hasn't changed, so I think you can keep my
>
> It’s been tweaked slightly that’s why I dropped your tested-by
> signoff. Could you test this version again please to verify it
> works for you?
Oh, I missed that you did make some changes...
Will test again and report back to you...
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] 14+ messages in thread
* Re: [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays.
2016-05-16 19:40 ` Geert Uytterhoeven
@ 2016-05-17 6:36 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2016-05-17 6:36 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Rob Herring, Frank Rowand, Matt Porter, Grant Likely, Koen Kooi,
Guenter Roeck, Marek Vasut, devicetree, linux-kernel
On Mon, May 16, 2016 at 9:40 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, May 16, 2016 at 9:27 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>>> On May 16, 2016, at 22:06 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, May 16, 2016 at 6:52 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>
>>>
>>> This patch hasn't changed, so I think you can keep my
>>
>> It’s been tweaked slightly that’s why I dropped your tested-by
>> signoff. Could you test this version again please to verify it
>> works for you?
>
> Oh, I missed that you did make some changes...
>
> Will test again and report back to you...
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
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] 14+ messages in thread
* [PATCH v2 5/5] of: overlay: Add pr_fmt for clarity
2016-05-16 16:52 [PATCH v2 0/5] of: generic infrastructure fixes Pantelis Antoniou
` (3 preceding siblings ...)
2016-05-16 16:52 ` [PATCH v2 4/5] of: overlay: Pick up label symbols from overlays Pantelis Antoniou
@ 2016-05-16 16:52 ` Pantelis Antoniou
4 siblings, 0 replies; 14+ messages in thread
From: Pantelis Antoniou @ 2016-05-16 16: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, Pantelis Antoniou, Pantelis Antoniou
There are a bunch of pr_.*() messages in the file, use a common pr_fmt
for making them a little bit shorter.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
drivers/of/overlay.c | 84 ++++++++++++++++++++++++----------------------------
1 file changed, 38 insertions(+), 46 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 89ebb70..eecd6d2 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -9,6 +9,8 @@
* version 2 as published by the Free Software Foundation.
*/
+#define pr_fmt(fmt) "overlay: %s() " fmt, __func__
+
#undef DEBUG
#include <linux/kernel.h>
#include <linux/module.h>
@@ -175,8 +177,8 @@ static int of_overlay_apply_one(struct of_overlay *ov,
for_each_property_of_node(overlay, prop) {
ret = of_overlay_apply_single_property(ov, target, prop);
if (ret) {
- pr_err("%s: Failed to apply prop @%s/%s\n",
- __func__, target->full_name, prop->name);
+ pr_err("Failed to apply prop @%s/%s\n",
+ target->full_name, prop->name);
return ret;
}
}
@@ -184,9 +186,8 @@ static int of_overlay_apply_one(struct of_overlay *ov,
for_each_child_of_node(overlay, child) {
ret = of_overlay_apply_single_device_node(ov, target, child);
if (ret != 0) {
- pr_err("%s: Failed to apply single node @%s/%s\n",
- __func__, target->full_name,
- child->name);
+ pr_err("Failed to apply single node @%s/%s\n",
+ target->full_name, child->name);
of_node_put(child);
return ret;
}
@@ -214,8 +215,8 @@ static int of_overlay_apply(struct of_overlay *ov)
err = of_overlay_apply_one(ov, ovinfo->target, ovinfo->overlay);
if (err != 0) {
- pr_err("%s: overlay failed '%s'\n",
- __func__, ovinfo->target->full_name);
+ pr_err("overlay failed '%s'\n",
+ ovinfo->target->full_name);
return err;
}
}
@@ -237,8 +238,7 @@ static struct device_node *find_target_node_direct(struct of_overlay *ov,
if (ret == 0) {
target = of_find_node_by_phandle(val);
if (!target) {
- pr_err("%s: Could not find target phandle 0x%x\n",
- __func__, val);
+ pr_err("Could not find target phandle 0x%x\n", val);
return NULL;
}
goto check_root;
@@ -251,8 +251,8 @@ static struct device_node *find_target_node_direct(struct of_overlay *ov,
if (!ov->target_root) {
target = of_find_node_by_path(path);
if (!target)
- pr_err("%s: Could not find target path \"%s\"\n",
- __func__, path);
+ pr_err("Could not find target path \"%s\"\n",
+ path);
return target;
}
@@ -265,8 +265,7 @@ static struct device_node *find_target_node_direct(struct of_overlay *ov,
of_node_full_name(ov->target_root),
*path ? "/" : "", path);
if (!newpath) {
- pr_err("%s: Could not allocate \"%s%s%s\"\n",
- __func__,
+ pr_err("Could not allocate \"%s%s%s\"\n",
of_node_full_name(ov->target_root),
*path ? "/" : "", path);
return NULL;
@@ -280,8 +279,7 @@ static struct device_node *find_target_node_direct(struct of_overlay *ov,
/* target is an alias, need to check */
target = of_find_node_by_path(path);
if (!target) {
- pr_err("%s: Could not find alias \"%s\"\n",
- __func__, path);
+ pr_err("Could not find alias \"%s\"\n", path);
return NULL;
}
goto check_root;
@@ -298,8 +296,8 @@ check_root:
if (np == ov->target_root)
return target;
}
- pr_err("%s: target \"%s\" not under target_root \"%s\"\n",
- __func__, of_node_full_name(target),
+ pr_err("target \"%s\" not under target_root \"%s\"\n",
+ of_node_full_name(target),
of_node_full_name(ov->target_root));
/* target is not under target_root */
of_node_put(target);
@@ -331,8 +329,7 @@ static struct device_node *find_target_node(struct of_overlay *ov,
target_indirect = of_get_child_by_name(info_node, "target-indirect");
if (!target_indirect) {
- pr_err("%s: Failed to find target-indirect node at %s\n",
- __func__,
+ pr_err("Failed to find target-indirect node at %s\n",
of_node_full_name(info_node));
return NULL;
}
@@ -340,8 +337,8 @@ static struct device_node *find_target_node(struct of_overlay *ov,
indirect = of_get_child_by_name(target_indirect, ov->indirect_id);
of_node_put(target_indirect);
if (!indirect) {
- pr_err("%s: Failed to find indirect child node \"%s\" at %s\n",
- __func__, ov->indirect_id,
+ pr_err("Failed to find indirect child node \"%s\" at %s\n",
+ ov->indirect_id,
of_node_full_name(info_node));
return NULL;
}
@@ -349,8 +346,8 @@ static struct device_node *find_target_node(struct of_overlay *ov,
target = find_target_node_direct(ov, indirect);
if (!target) {
- pr_err("%s: Failed to find target for \"%s\" at %s\n",
- __func__, ov->indirect_id,
+ pr_err("Failed to find target for \"%s\" at %s\n",
+ ov->indirect_id,
of_node_full_name(indirect));
}
of_node_put(indirect);
@@ -705,8 +702,7 @@ static int __of_overlay_create(struct device_node *tree,
id = idr_alloc(&ov_idr, ov, 0, 0, GFP_KERNEL);
if (id < 0) {
- pr_err("%s: idr_alloc() failed for tree@%s\n",
- __func__, tree->full_name);
+ pr_err("idr_alloc() failed for tree@%s\n", tree->full_name);
err = id;
goto err_destroy_trans;
}
@@ -715,46 +711,46 @@ static int __of_overlay_create(struct device_node *tree,
/* build the overlay info structures */
err = of_build_overlay_info(ov, tree);
if (err) {
- pr_err("%s: of_build_overlay_info() failed for tree@%s\n",
- __func__, tree->full_name);
+ pr_err("of_build_overlay_info() failed for tree@%s\n",
+ tree->full_name);
goto err_free_idr;
}
/* apply the overlay */
err = of_overlay_apply(ov);
if (err) {
- pr_err("%s: of_overlay_apply() failed for tree@%s\n",
- __func__, tree->full_name);
+ pr_err("of_overlay_apply() failed for tree@%s\n",
+ tree->full_name);
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);
+ pr_err("of_overlay_add_symbols() failed for tree@%s\n",
+ tree->full_name);
goto err_abort_trans;
}
/* apply the changeset */
err = __of_changeset_apply(&ov->cset);
if (err) {
- pr_err("%s: __of_changeset_apply() failed for tree@%s\n",
- __func__, tree->full_name);
+ pr_err("__of_changeset_apply() failed for tree@%s\n",
+ tree->full_name);
goto err_revert_overlay;
}
ov->kobj.kset = ov_kset;
err = kobject_add(&ov->kobj, NULL, "%d", id);
if (err != 0) {
- pr_err("%s: kobject_add() failed for tree@%s\n",
- __func__, tree->full_name);
+ pr_err("kobject_add() failed for tree@%s\n",
+ tree->full_name);
goto err_cancel_overlay;
}
err = sysfs_create_groups(&ov->kobj, ov->attr_groups);
if (err != 0) {
- pr_err("%s: sysfs_create_groups() failed for tree@%s\n",
- __func__, tree->full_name);
+ pr_err("sysfs_create_groups() failed for tree@%s\n",
+ tree->full_name);
goto err_remove_kobj;
}
@@ -871,9 +867,8 @@ static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)
/* check against each subtree affected by this overlay */
list_for_each_entry(ce, &ovt->cset.entries, node) {
if (overlay_subtree_check(ce->np, dn)) {
- pr_err("%s: #%d clashes #%d @%s\n",
- __func__, ov->id, ovt->id,
- dn->full_name);
+ pr_err("#%d clashes #%d @%s\n",
+ ov->id, ovt->id, dn->full_name);
return 0;
}
}
@@ -899,8 +894,7 @@ static int overlay_removal_is_ok(struct of_overlay *ov)
list_for_each_entry(ce, &ov->cset.entries, node) {
if (!overlay_is_topmost(ov, ce->np)) {
- pr_err("%s: overlay #%d is not topmost\n",
- __func__, ov->id);
+ pr_err("overlay #%d is not topmost\n", ov->id);
return 0;
}
}
@@ -926,16 +920,14 @@ int of_overlay_destroy(int id)
ov = idr_find(&ov_idr, id);
if (ov == NULL) {
err = -ENODEV;
- pr_err("%s: Could not find overlay #%d\n",
- __func__, id);
+ pr_err("Could not find overlay #%d\n", id);
goto out;
}
/* check whether the overlay is safe to remove */
if (!overlay_removal_is_ok(ov)) {
err = -EBUSY;
- pr_err("%s: removal check failed for overlay #%d\n",
- __func__, id);
+ pr_err("removal check failed for overlay #%d\n", id);
goto out;
}
--
1.7.12
^ permalink raw reply related [flat|nested] 14+ messages in thread