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.
change log v1 -> v2:
1. Use pr_fmt() to simply the SRAT message.
Suggested by Joe Perches <[email protected]>
2. Improve the log in patch 4, and change the return value of the stub of
acpi_table_parse() in linux/acpi.h
Suggested by Toshi Kani <[email protected]>
3. Merge the two pr_{info|err} patches into one.
4. Remove on of the patch that has been merged by Rafael.
Tang Chen (4):
acpi, numa, mem_hotplug: Kill save_add_info().
acpi cleanup: Use pr_{info|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 | 40 +++++++++++++++++-----------------------
drivers/acpi/tables.c | 9 +++++----
include/linux/acpi.h | 2 +-
3 files changed, 23 insertions(+), 28 deletions(-)
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 <[email protected]>
Acked-by: Toshi Kani <[email protected]>
---
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
The comment about return value of acpi_table_parse() is incorrect.
This patch fix it.
Since all callers only check if the function succeeded or not, this
patch simplifies the semantics by returning -errno for all failure
cases. This will also simply the comment.
As suggested by Toshi Kani <[email protected]>, also change the stub
in linux/acpi.h to return -ENODEV.
Signed-off-by: Tang Chen <[email protected]>
---
drivers/acpi/tables.c | 7 ++++---
include/linux/acpi.h | 2 +-
2 files changed, 5 insertions(+), 4 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;
}
/*
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 353ba25..39fd53a 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -443,7 +443,7 @@ struct acpi_table_header;
static inline int acpi_table_parse(char *id,
int (*handler)(struct acpi_table_header *))
{
- return -1;
+ return -ENODEV;
}
static inline int acpi_nvs_register(__u64 start, __u64 size)
--
1.7.1
Use pr_{info|err}() instead of printk() in arch/x86/mm/srat.c.
As suggested by Joe Perches <[email protected]>, use pr_fmt(fmt) to simplify
the output format.
Signed-off-by: Tang Chen <[email protected]>
Acked-by: Toshi Kani <[email protected]>
---
arch/x86/mm/srat.c | 32 +++++++++++++++++---------------
1 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 1613c02..5adfcfb 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)
@@ -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,26 @@ 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 +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;
}
@@ -124,15 +126,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);
}
/* Callback for parsing of the Proximity Domain <-> Memory Area mappings */
@@ -157,7 +159,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;
}
@@ -166,9 +168,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:
--
1.7.1
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 <[email protected]>
Acked-by: Toshi Kani <[email protected]>
---
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
On Tue, 2013-09-03 at 16:45 +0800, Tang Chen wrote:
> The comment about return value of acpi_table_parse() is incorrect.
> This patch fix it.
>
> Since all callers only check if the function succeeded or not, this
> patch simplifies the semantics by returning -errno for all failure
> cases. This will also simply the comment.
>
> As suggested by Toshi Kani <[email protected]>, also change the stub
> in linux/acpi.h to return -ENODEV.
>
> Signed-off-by: Tang Chen <[email protected]>
Acked-by: Toshi Kani <[email protected]>
Thanks,
-Toshi
On Tuesday, September 03, 2013 04:45:37 PM Tang Chen wrote:
> 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.
>
> change log v1 -> v2:
> 1. Use pr_fmt() to simply the SRAT message.
> Suggested by Joe Perches <[email protected]>
> 2. Improve the log in patch 4, and change the return value of the stub of
> acpi_table_parse() in linux/acpi.h
> Suggested by Toshi Kani <[email protected]>
> 3. Merge the two pr_{info|err} patches into one.
> 4. Remove on of the patch that has been merged by Rafael.
>
> Tang Chen (4):
> acpi, numa, mem_hotplug: Kill save_add_info().
> acpi cleanup: Use pr_{info|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.
Thanks for the patches!
Peter, any objections against [1-2/4]? If not, I'll queue them up for 3.13.
Thanks,
Rafael
(2013/09/03 17:45), Tang Chen wrote:
> Use pr_{info|err}() instead of printk() in arch/x86/mm/srat.c.
>
> As suggested by Joe Perches <[email protected]>, use pr_fmt(fmt) to simplify
> the output format.
>
> Signed-off-by: Tang Chen <[email protected]>
> Acked-by: Toshi Kani <[email protected]>
> ---
> arch/x86/mm/srat.c | 32 +++++++++++++++++---------------
> 1 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
> index 1613c02..5adfcfb 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)
> @@ -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,26 @@ 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);
It should be one line.
> 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);
ditto.
> }
>
> /* Callback for Proximity Domain -> LAPIC mapping */
> @@ -113,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;
> }
> @@ -124,15 +126,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);
ditto.
> }
>
> /* Callback for parsing of the Proximity Domain <-> Memory Area mappings */
> @@ -157,7 +159,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;
> }
>
> @@ -166,9 +168,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);
It should be two lines.
Thanks,
Yasuaki Ishimatsu
>
> return 0;
> out_err_bad_srat:
>
On 09/05/2013 07:50 AM, Rafael J. Wysocki wrote:
> On Tuesday, September 03, 2013 04:45:37 PM Tang Chen wrote:
>> 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.
>>
>> change log v1 -> v2:
>> 1. Use pr_fmt() to simply the SRAT message.
>> Suggested by Joe Perches<[email protected]>
>> 2. Improve the log in patch 4, and change the return value of the stub of
>> acpi_table_parse() in linux/acpi.h
>> Suggested by Toshi Kani<[email protected]>
>> 3. Merge the two pr_{info|err} patches into one.
>> 4. Remove on of the patch that has been merged by Rafael.
>>
>> Tang Chen (4):
>> acpi, numa, mem_hotplug: Kill save_add_info().
>> acpi cleanup: Use pr_{info|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.
>
> Thanks for the patches!
>
> Peter, any objections against [1-2/4]? If not, I'll queue them up for 3.13.
Hi Rafael,
Thanks for the checking. As Ishimatu has sent some comments for patch 2,
I'd like
to send a v3 patch-set soon. Please queue the coming v3 patches.
Thanks.
On Thursday, September 05, 2013 09:56:52 AM Tang Chen wrote:
> On 09/05/2013 07:50 AM, Rafael J. Wysocki wrote:
> > On Tuesday, September 03, 2013 04:45:37 PM Tang Chen wrote:
> >> 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.
> >>
> >> change log v1 -> v2:
> >> 1. Use pr_fmt() to simply the SRAT message.
> >> Suggested by Joe Perches<[email protected]>
> >> 2. Improve the log in patch 4, and change the return value of the stub of
> >> acpi_table_parse() in linux/acpi.h
> >> Suggested by Toshi Kani<[email protected]>
> >> 3. Merge the two pr_{info|err} patches into one.
> >> 4. Remove on of the patch that has been merged by Rafael.
> >>
> >> Tang Chen (4):
> >> acpi, numa, mem_hotplug: Kill save_add_info().
> >> acpi cleanup: Use pr_{info|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.
> >
> > Thanks for the patches!
> >
> > Peter, any objections against [1-2/4]? If not, I'll queue them up for 3.13.
>
> Hi Rafael,
>
> Thanks for the checking. As Ishimatu has sent some comments for patch 2,
> I'd like
> to send a v3 patch-set soon. Please queue the coming v3 patches.
Sure, thanks.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.