On Thu, Jun 2, 2016 at 12:33 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> On 06/02/16 08:14, Rob Herring wrote:
>> Clean-up all the DT printk functions to use common pr_fmt prefix.
>>
>> Some print statements such as kmalloc errors were redundant, so just
>> drop those.
>>
>> Cc: Frank Rowand <frowand.list@xxxxxxxxx>
>> Cc: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>> ---
>> drivers/of/address.c | 49 ++++++++++++++++++++++----------------------
>> drivers/of/base.c | 11 ++++++----
>> drivers/of/dynamic.c | 47 +++++++++++++++++++++---------------------
>> drivers/of/fdt.c | 12 +++++------
>> drivers/of/fdt_address.c | 35 ++++++++++++++++---------------
>> drivers/of/irq.c | 2 ++
>> drivers/of/of_numa.c | 22 ++++++--------------
>> drivers/of/of_pci.c | 6 ++++--
>> drivers/of/of_reserved_mem.c | 22 ++++++++++----------
>> drivers/of/overlay.c | 43 +++++++++++++++-----------------------
>> drivers/of/platform.c | 16 +++++++--------
>> drivers/of/resolver.c | 2 ++
>> 12 files changed, 130 insertions(+), 137 deletions(-)
>>
>
> Nice cleanup. A couple of comments below.
>
> Reviewed-by: Frank Rowand <frank.rowand@xxxxxxxxxxx>
>
> < snip >
>> @@ -65,11 +67,7 @@ static int __init of_numa_parse_memory_nodes(void)
>> u32 nid;
>> int r = 0;
>>
>> - for (;;) {
>> - np = of_find_node_by_type(np, "memory");
>> - if (!np)
>> - break;
>> -
>> + for_each_node_by_type(np, "memory") {
>> r = of_property_read_u32(np, "numa-node-id", &nid);
>> if (r == -EINVAL)
>> /*
>> @@ -83,18 +81,10 @@ static int __init of_numa_parse_memory_nodes(void)
>> break;
>>
>> r = of_address_to_resource(np, 0, &rsrc);
>> - if (r) {
>> - pr_err("NUMA: bad reg property in memory node\n");
>> - break;
>> - }
>> -
>> - pr_debug("NUMA: base = %llx len = %llx, node = %u\n",
>> - rsrc.start, rsrc.end - rsrc.start + 1, nid);
>> -
>> - r = numa_add_memblk(nid, rsrc.start,
>
> Missing declaration: int i;
>
> Calling of_address_to_resource() with index > 0 and then calling
> numa_add_memblk() with the resulting resource(s) is a change in
> functionality. This should be in a separate patch.
This shouldn't be here. This was my rewriting the NUMA patches under review...
>
>> + for (i = 0; !r; i++, r = of_address_to_resource(np, i, &rsrc)) {
>> + r = numa_add_memblk(nid, rsrc.start,
>> rsrc.end - rsrc.start + 1);
>> - if (r)
>> - break;
>> + }
>> }
>> of_node_put(np);
>>
>
> < snip >
>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 8225081..318dbb5 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -8,7 +8,9 @@
>> * modify it under the terms of the GNU General Public License
>> * version 2 as published by the Free Software Foundation.
>> */
>> -#undef DEBUG
>> +
>> +#define pr_fmt(fmt) "OF: overlay: " fmt
>> +
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> @@ -137,8 +139,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;
>> }
>> }
>> @@ -146,9 +148,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;
>> }
>> @@ -176,8 +177,7 @@ 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("apply failed '%s'\n", ovinfo->target->full_name);
>> return err;
>> }
>> }
>> @@ -208,7 +208,7 @@ static struct device_node *find_target_node(struct device_node *info_node)
>> if (ret == 0)
>> return of_find_node_by_path(path);
>>
>> - pr_err("%s: Failed to find target for node %p (%s)\n", __func__,
>> + pr_err("Failed to find target for node %p (%s)\n",
>> info_node, info_node->name);
>>
>> return NULL;
>> @@ -355,8 +355,6 @@ 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);
>
> Every other error in of_overlay_create() results in a pr_err().
> (The other cases of removing pr_err() in this file are fine, because
> the errors are already reported in the functions called from this
> function.)
>
> I would recommend leaving in the pr_err() for idr_alloc() failure.
I was thinking idr_alloc is going to call kmalloc which will print
errors on failure, but there may be some case it doesn't.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
