[PATCH 0/6] acpi: Fix and cleanup in acpi.

[PATCH 0/6] acpi: Fix and cleanup in acpi.

[Tang Chen]

This patch-set fix the following problems:

1. Kill useless function save_add_info() which will block us from using
   numa when MEMORY_HOTPLUG is not configured.
2. acpi_table_parse() didn't check if @id is NULL.
3. Fix incorrect comment in acpi_table_parse(), and return -ENOENT if a
   table is not found.

And also did some cleanup.

Tang Chen (6):
  acpi, numa, mem_hotplug: Kill save_add_info().
  acpi, numa, mem_hotplug: Print Hot-Pluggable Field in SRAT.
  acpi cleanup: Use pr_info() instead of printk() in arch/x86/mm/srat.c
  acpi cleanup: Use pr_err() instead of printk() in arch/x86/mm/srat.c
  acpi: Check if @id is NULL in acpi_table_parse()
  acpi: Return -ENOENT in acpi_table_parse() and fix wrong comment.

 arch/x86/mm/srat.c    |   38 +++++++++++++++++---------------------
 drivers/acpi/tables.c |    9 +++++----
 2 files changed, 22 insertions(+), 25 deletions(-)

[PATCH 1/6] acpi, numa, mem_hotplug: Kill save_add_info().

[Tang Chen]

save_add_info() is defined as:

	#ifdef CONFIG_MEMORY_HOTPLUG
	static inline int save_add_info(void) {return 1;}
	#else
	static inline int save_add_info(void) {return 0;}
	#endif

which means it is true when memory hotplug is configured.

In acpi_numa_memory_affinity_init(), it checks the memory hotplug
flag in SRAT memory affinity and save_add_info() like this:

	if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
		goto out_err;
	......
	node = setup_node(pxm);
	numa_add_memblk(node, start, end);
	......

which means if the memory range is hotpluggable, but memory hotplug is not
configured, it won't add these memory to numa_meminfo.

After this, numa_meminfo_cover_memory() will fail, which will finally cause
numa_init() to fail.

numa_init()
 |->numa_register_memblks()
     |->numa_meminfo_cover_memory()

When numa_init() fails, it will fallback to numa_init(dummy_numa_init), and
all numa architecture will not be setup.

This is nonsense. Even if memory hotplug is not configured, we can also use
numa architecture.

Actually, save_add_info() is added by commit 71efa8fdc55e70ec6687c897a30759f0a2c2ad7e
in 2006. And now it is useless.

So this patch kill save_add_info() and the nonsense checking.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 arch/x86/mm/srat.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index cdd0da9..1613c02 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -135,12 +135,6 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 	       pxm, apic_id, node);
 }
 
-#ifdef CONFIG_MEMORY_HOTPLUG
-static inline int save_add_info(void) {return 1;}
-#else
-static inline int save_add_info(void) {return 0;}
-#endif
-
 /* Callback for parsing of the Proximity Domain <-> Memory Area mappings */
 int __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
@@ -154,8 +148,6 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 		goto out_err_bad_srat;
 	if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
 		goto out_err;
-	if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
-		goto out_err;
 
 	start = ma->base_address;
 	end = start + ma->length;
-- 
1.7.1

Re: [PATCH 1/6] acpi, numa, mem_hotplug: Kill save_add_info().

[Toshi Kani]

On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
> save_add_info() is defined as:
> 
> 	#ifdef CONFIG_MEMORY_HOTPLUG
> 	static inline int save_add_info(void) {return 1;}
> 	#else
> 	static inline int save_add_info(void) {return 0;}
> 	#endif
> 
> which means it is true when memory hotplug is configured.
> 
> In acpi_numa_memory_affinity_init(), it checks the memory hotplug
> flag in SRAT memory affinity and save_add_info() like this:
> 
> 	if ((ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && !save_add_info())
> 		goto out_err;
> 	......
> 	node = setup_node(pxm);
> 	numa_add_memblk(node, start, end);
> 	......
> 
> which means if the memory range is hotpluggable, but memory hotplug is not
> configured, it won't add these memory to numa_meminfo.
> 
> After this, numa_meminfo_cover_memory() will fail, which will finally cause
> numa_init() to fail.
> 
> numa_init()
>  |->numa_register_memblks()
>      |->numa_meminfo_cover_memory()
> 
> When numa_init() fails, it will fallback to numa_init(dummy_numa_init), and
> all numa architecture will not be setup.
> 
> This is nonsense. Even if memory hotplug is not configured, we can also use
> numa architecture.
> 
> Actually, save_add_info() is added by commit 71efa8fdc55e70ec6687c897a30759f0a2c2ad7e
> in 2006. And now it is useless.
> 
> So this patch kill save_add_info() and the nonsense checking.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>

Good catch!

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

[PATCH 2/6] acpi, numa, mem_hotplug: Print Hot-Pluggable Field in SRAT.

[Tang Chen]

The Hot-Pluggable field in SRAT suggests if the memory could be
hotplugged while the system is running. Print it as well when
parsing SRAT will help users to know which memory is hotpluggable.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Reviewed-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 arch/x86/mm/srat.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 1613c02..71411aa 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -141,6 +141,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
 	u64 start, end;
 	int node, pxm;
+	u32 hotpluggable;
 
 	if (srat_disabled())
 		goto out_err;
@@ -152,6 +153,8 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 	start = ma->base_address;
 	end = start + ma->length;
 	pxm = ma->proximity_domain;
+	hotpluggable = ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE;
+
 	if (acpi_srat_revision <= 1)
 		pxm &= 0xff;
 
@@ -166,9 +169,10 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	node_set(node, numa_nodes_parsed);
 
-	printk(KERN_INFO "SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
-	       node, pxm,
-	       (unsigned long long) start, (unsigned long long) end - 1);
+	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n",
+		node, pxm,
+		(unsigned long long) start, (unsigned long long) end - 1,
+		hotpluggable ? " hotplug" : "");
 
 	return 0;
 out_err_bad_srat:
-- 
1.7.1

Re: [PATCH 2/6] acpi, numa, mem_hotplug: Print Hot-Pluggable Field in SRAT.

[Toshi Kani]

On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
> The Hot-Pluggable field in SRAT suggests if the memory could be
> hotplugged while the system is running. Print it as well when
> parsing SRAT will help users to know which memory is hotpluggable.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Reviewed-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
> Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

[PATCH 3/6] acpi cleanup: Use pr_info() instead of printk() in arch/x86/mm/srat.c

[Tang Chen]

Use pr_info() instead of printk() in arch/x86/mm/srat.c.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 arch/x86/mm/srat.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 71411aa..6286e89 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -71,8 +71,8 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	pxm = pa->proximity_domain;
 	apic_id = pa->apic_id;
 	if (!apic->apic_id_valid(apic_id)) {
-		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
-			 pxm, apic_id);
+		pr_info("SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
+			pxm, apic_id);
 		return;
 	}
 	node = setup_node(pxm);
@@ -83,13 +83,13 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	}
 
 	if (apic_id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		pr_info("SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
 		return;
 	}
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u\n",
+	pr_info("SRAT: PXM %u -> APIC 0x%04x -> Node %u\n",
 	       pxm, apic_id, node);
 }
 
@@ -124,14 +124,14 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		apic_id = pa->apic_id;
 
 	if (apic_id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		pr_info("SRAT: PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
 		return;
 	}
 
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u\n",
+	pr_info("SRAT: PXM %u -> APIC 0x%02x -> Node %u\n",
 	       pxm, apic_id, node);
 }
 
-- 
1.7.1

Re: [PATCH 3/6] acpi cleanup: Use pr_info() instead of printk() in arch/x86/mm/srat.c

[Joe Perches]

On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
> arch/x86/mm/srat.c

I think it'd be better to use pr_fmt
with the conversions to pr_info and pr_err.

pr_fmt can prefix the appropriate srat: and
so the format strings do not need it.

Something like:
---
 arch/x86/mm/srat.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index cdd0da9..350b4c5 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -9,6 +9,8 @@
  * are in one chunk. Holes between them will be included in the node.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/acpi.h>
 #include <linux/mmzone.h>
@@ -33,7 +35,7 @@ static __init int setup_node(int pxm)
 
 static __init void bad_srat(void)
 {
-	printk(KERN_ERR "SRAT: SRAT not used.\n");
+	pr_err("SRAT not used\n");
 	acpi_numa = -1;
 }
 
@@ -71,26 +73,25 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	pxm = pa->proximity_domain;
 	apic_id = pa->apic_id;
 	if (!apic->apic_id_valid(apic_id)) {
-		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
-			 pxm, apic_id);
+		pr_info("PXM %u -> X2APIC 0x%04x ignored\n", pxm, apic_id);
 		return;
 	}
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
+		pr_err("Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
 	}
 
 	if (apic_id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		pr_info("PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n",
+			pxm, apic_id, node);
 		return;
 	}
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u\n",
-	       pxm, apic_id, node);
+	pr_info("PXM %u -> APIC 0x%04x -> Node %u\n", pxm, apic_id, node);
 }
 
 /* Callback for Proximity Domain -> LAPIC mapping */
@@ -113,7 +114,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		pxm |= *((unsigned int*)pa->proximity_domain_hi) << 8;
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
+		pr_err("Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
 	}
@@ -124,15 +125,15 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		apic_id = pa->apic_id;
 
 	if (apic_id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		pr_info("PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n",
+			pxm, apic_id, node);
 		return;
 	}
 
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u\n",
-	       pxm, apic_id, node);
+	pr_info("PXM %u -> APIC 0x%02x -> Node %u\n", pxm, apic_id, node);
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG
@@ -165,7 +166,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains.\n");
+		pr_err("Too many proximity domains\n");
 		goto out_err_bad_srat;
 	}
 
@@ -174,9 +175,9 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	node_set(node, numa_nodes_parsed);
 
-	printk(KERN_INFO "SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
-	       node, pxm,
-	       (unsigned long long) start, (unsigned long long) end - 1);
+	pr_info("Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
+		node, pxm,
+		(unsigned long long) start, (unsigned long long) end - 1);
 
 	return 0;
 out_err_bad_srat:

Re: [PATCH 3/6] acpi cleanup: Use pr_info() instead of printk() in arch/x86/mm/srat.c

[Tang Chen]

Hi Joe,

On 08/16/2013 03:25 PM, Joe Perches wrote:
> On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
>> arch/x86/mm/srat.c
>
> I think it'd be better to use pr_fmt
> with the conversions to pr_info and pr_err.
>
> pr_fmt can prefix the appropriate srat: and
> so the format strings do not need it.
>
> Something like:
> ---
>   arch/x86/mm/srat.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
> index cdd0da9..350b4c5 100644
> --- a/arch/x86/mm/srat.c
> +++ b/arch/x86/mm/srat.c
> @@ -9,6 +9,8 @@
>    * are in one chunk. Holes between them will be included in the node.
>    */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

OK, will update the patches.

Thanks. :)

[PATCH 3/6] acpi cleanup: Use pr_info() instead of printk() in arch/x86/mm/srat.c

[Tang Chen]

Use pr_info() instead of printk() in arch/x86/mm/srat.c.

As Joe suggested, define pr_fmt(fmt) as KBUILD_MODNAME ": " fmt to
prefix message with "srat: ".

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 arch/x86/mm/srat.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 71411aa..591f4bb 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -24,6 +24,8 @@
 #include <asm/apic.h>
 #include <asm/uv/uv.h>
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 int acpi_numa __initdata;
 
 static __init int setup_node(int pxm)
@@ -71,8 +73,8 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	pxm = pa->proximity_domain;
 	apic_id = pa->apic_id;
 	if (!apic->apic_id_valid(apic_id)) {
-		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
-			 pxm, apic_id);
+		pr_info("PXM %u -> X2APIC 0x%04x ignored\n",
+			pxm, apic_id);
 		return;
 	}
 	node = setup_node(pxm);
@@ -83,13 +85,13 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	}
 
 	if (apic_id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		pr_info("PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
 		return;
 	}
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u\n",
+	pr_info("PXM %u -> APIC 0x%04x -> Node %u\n",
 	       pxm, apic_id, node);
 }
 
@@ -124,14 +126,14 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		apic_id = pa->apic_id;
 
 	if (apic_id >= MAX_LOCAL_APIC) {
-		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		pr_info("PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
 		return;
 	}
 
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u\n",
+	pr_info("PXM %u -> APIC 0x%02x -> Node %u\n",
 	       pxm, apic_id, node);
 }
 
@@ -169,8 +171,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	node_set(node, numa_nodes_parsed);
 
-	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n",
-		node, pxm,
+	pr_info("Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n", node, pxm,
 		(unsigned long long) start, (unsigned long long) end - 1,
 		hotpluggable ? " hotplug" : "");
 
-- 
1.7.1

[PATCH 4/6] acpi cleanup: Use pr_err() instead of printk() in arch/x86/mm/srat.c

[Tang Chen]

Use pr_err() instead of printk() in arch/x86/mm/srat.c

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 arch/x86/mm/srat.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 591f4bb..d1e3b95 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -35,7 +35,7 @@ static __init int setup_node(int pxm)
 
 static __init void bad_srat(void)
 {
-	printk(KERN_ERR "SRAT: SRAT not used.\n");
+	pr_err("SRAT not used.\n");
 	acpi_numa = -1;
 }
 
@@ -79,7 +79,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	}
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
+		pr_err("Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
 	}
@@ -115,7 +115,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		pxm |= *((unsigned int*)pa->proximity_domain_hi) << 8;
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
+		pr_err("Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
 	}
@@ -162,7 +162,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains.\n");
+		pr_err("Too many proximity domains.\n");
 		goto out_err_bad_srat;
 	}
 
-- 
1.7.1

Re: [PATCH 3/6] acpi cleanup: Use pr_info() instead of printk() in arch/x86/mm/srat.c

[Toshi Kani]

On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
> Use pr_info() instead of printk() in arch/x86/mm/srat.c.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>

Please fold patch 4/6 into this patch 3/6 since they both are printk()
cleanup on the same file.  I do not see why they need to be separated.
With that change,

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

[PATCH 4/6] acpi cleanup: Use pr_err() instead of printk() in arch/x86/mm/srat.c

[Tang Chen]

Use pr_err() instead of printk() in arch/x86/mm/srat.c

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 arch/x86/mm/srat.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 6286e89..32b9597 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -33,7 +33,7 @@ static __init int setup_node(int pxm)
 
 static __init void bad_srat(void)
 {
-	printk(KERN_ERR "SRAT: SRAT not used.\n");
+	pr_err("SRAT: SRAT not used.\n");
 	acpi_numa = -1;
 }
 
@@ -77,7 +77,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	}
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
+		pr_err("SRAT: Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
 	}
@@ -113,7 +113,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		pxm |= *((unsigned int*)pa->proximity_domain_hi) << 8;
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
+		pr_err("SRAT: Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
 	}
@@ -160,7 +160,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	node = setup_node(pxm);
 	if (node < 0) {
-		printk(KERN_ERR "SRAT: Too many proximity domains.\n");
+		pr_err("SRAT: Too many proximity domains.\n");
 		goto out_err_bad_srat;
 	}
 
-- 
1.7.1

[PATCH 5/6] acpi: Check if @id is NULL in acpi_table_parse()

[Tang Chen]

strncmp() does not check if the params are NULL. In acpi_table_parse(),
if @id is NULL, the kernel will panic.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/tables.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index d67a1fe..5a5263b 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -293,7 +293,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 	if (acpi_disabled)
 		return -ENODEV;
 
-	if (!handler)
+	if (!id || !handler)
 		return -EINVAL;
 
 	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
-- 
1.7.1

Re: [PATCH 5/6] acpi: Check if @id is NULL in acpi_table_parse()

[Toshi Kani]

On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
> strncmp() does not check if the params are NULL. In acpi_table_parse(),
> if @id is NULL, the kernel will panic.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>

Acked-by: Toshi Kani <toshi.kani@hp.com>

-Toshi


> ---
>  drivers/acpi/tables.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index d67a1fe..5a5263b 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -293,7 +293,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
>  	if (acpi_disabled)
>  		return -ENODEV;
>  
> -	if (!handler)
> +	if (!id || !handler)
>  		return -EINVAL;
>  
>  	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)

[PATCH 6/6] acpi: Return -ENOENT in acpi_table_parse() and fix wrong comment.

[Tang Chen]

The comment about return value of acpi_table_parse() is incorrect.
This patch fix it.

Furthermore, if the table is not found, return 1 means nothing, and
make it difficult to write the comment. So return -ENOENT when the
table is not found, and correct the comment.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/tables.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 5a5263b..e6de24f 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -278,12 +278,13 @@ acpi_table_parse_madt(enum acpi_madt_type id,
 
 /**
  * acpi_table_parse - find table with @id, run @handler on it
- *
  * @id: table id to find
  * @handler: handler to run
  *
  * Scan the ACPI System Descriptor Table (STD) for a table matching @id,
- * run @handler on it.  Return 0 if table found, return on if not.
+ * run @handler on it.
+ *
+ * Return 0 on success, -errno on failure.
  */
 int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 {
@@ -306,7 +307,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 		early_acpi_os_unmap_memory(table, tbl_size);
 		return 0;
 	} else
-		return 1;
+		return -ENOENT;
 }
 
 /* 
-- 
1.7.1

Re: [PATCH 6/6] acpi: Return -ENOENT in acpi_table_parse() and fix wrong comment.

[Toshi Kani]

On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
> The comment about return value of acpi_table_parse() is incorrect.
> This patch fix it.
> 
> Furthermore, if the table is not found, return 1 means nothing, and
> make it difficult to write the comment. So return -ENOENT when the
> table is not found, and correct the comment.

I am OK with the change, but the above description is not very clear.
You should state that all callers only check if the function succeeded
or not.  So, you are simplifying the semantics by returning -errno for
all failure cases.

Since you are making this change, I'd suggest you also update the stub
function in linux/acpi.h to return -ENODEV as well.

Thanks,
-Toshi

Re: [PATCH 6/6] acpi: Return -ENOENT in acpi_table_parse() and fix wrong comment.

[Tang Chen]

On 08/20/2013 03:29 AM, Toshi Kani wrote:
> On Fri, 2013-08-16 at 15:06 +0800, Tang Chen wrote:
>> The comment about return value of acpi_table_parse() is incorrect.
>> This patch fix it.
>>
>> Furthermore, if the table is not found, return 1 means nothing, and
>> make it difficult to write the comment. So return -ENOENT when the
>> table is not found, and correct the comment.
>
> I am OK with the change, but the above description is not very clear.
> You should state that all callers only check if the function succeeded
> or not.  So, you are simplifying the semantics by returning -errno for
> all failure cases.
>
> Since you are making this change, I'd suggest you also update the stub
> function in linux/acpi.h to return -ENODEV as well.

OK, followed. And will merge patch 3 and 4 and resend them later.

Thanks for reviewing.