[PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Tang Chen]

This patch-set does some trivial fix and improving in ACPI code
for memory hotplug.

Patch 1,3,4 have been acked.

Tang Chen (4):
  acpi: Print Hot-Pluggable Field in SRAT.
  earlycpio.c: Fix the confusing comment of find_cpio_data().
  acpi: Remove "continue" in macro INVALID_TABLE().
  acpi: Introduce acpi_verify_initrd() to check if a table is invalid.

 arch/x86/mm/srat.c |   11 ++++--
 drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
 lib/earlycpio.c    |   27 ++++++++--------
 3 files changed, 85 insertions(+), 37 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Tang Chen]

This patch-set does some trivial fix and improving in ACPI code
for memory hotplug.

Patch 1,3,4 have been acked.

Tang Chen (4):
  acpi: Print Hot-Pluggable Field in SRAT.
  earlycpio.c: Fix the confusing comment of find_cpio_data().
  acpi: Remove "continue" in macro INVALID_TABLE().
  acpi: Introduce acpi_verify_initrd() to check if a table is invalid.

 arch/x86/mm/srat.c |   11 ++++--
 drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
 lib/earlycpio.c    |   27 ++++++++--------
 3 files changed, 85 insertions(+), 37 deletions(-)

[PATCH part2 1/4] acpi: 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 |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index cdd0da9..d44c8a4 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -146,6 +146,7 @@ int __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
 	u64 start, end;
+	u32 hotpluggable;
 	int node, pxm;
 
 	if (srat_disabled())
@@ -154,7 +155,8 @@ 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())
+	hotpluggable = ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE;
+	if (hotpluggable && !save_add_info())
 		goto out_err;
 
 	start = ma->base_address;
@@ -174,9 +176,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 ? " Hot Pluggable" : "");
 
 	return 0;
 out_err_bad_srat:
-- 
1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH part2 1/4] acpi: 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 |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index cdd0da9..d44c8a4 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -146,6 +146,7 @@ int __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
 	u64 start, end;
+	u32 hotpluggable;
 	int node, pxm;
 
 	if (srat_disabled())
@@ -154,7 +155,8 @@ 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())
+	hotpluggable = ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE;
+	if (hotpluggable && !save_add_info())
 		goto out_err;
 
 	start = ma->base_address;
@@ -174,9 +176,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 ? " Hot Pluggable" : "");
 
 	return 0;
 out_err_bad_srat:
-- 
1.7.1

[PATCH part2 2/4] earlycpio.c: Fix the confusing comment of find_cpio_data().

[Tang Chen]

The comments of find_cpio_data() says:

  * @offset: When a matching file is found, this is the offset to the
  *          beginning of the cpio. ......

But according to the code,

  dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
  nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
  ....
  *offset = (long)nptr - (long)data;	/* data is the cpio file */

@offset is the offset of the next file, not the matching file itself.
This is confused and may cause unnecessary waste of time to debug.
So fix it.

v1 -> v2:
As tj suggested, rename @offset to @nextoff which is more clear to
users. And also adjust the new comments.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 lib/earlycpio.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/earlycpio.c b/lib/earlycpio.c
index 7aa7ce2..c7ae5ed 100644
--- a/lib/earlycpio.c
+++ b/lib/earlycpio.c
@@ -49,22 +49,23 @@ enum cpio_fields {
 
 /**
  * cpio_data find_cpio_data - Search for files in an uncompressed cpio
- * @path:   The directory to search for, including a slash at the end
- * @data:   Pointer to the the cpio archive or a header inside
- * @len:    Remaining length of the cpio based on data pointer
- * @offset: When a matching file is found, this is the offset to the
- *          beginning of the cpio. It can be used to iterate through
- *          the cpio to find all files inside of a directory path
+ * @path:       The directory to search for, including a slash at the end
+ * @data:       Pointer to the the cpio archive or a header inside
+ * @len:        Remaining length of the cpio based on data pointer
+ * @nextoff:    When a matching file is found, this is the offset from the
+ *              beginning of the cpio to the beginning of the next file, not the
+ *              matching file itself. It can be used to iterate through the cpio
+ *              to find all files inside of a directory path 
  *
- * @return: struct cpio_data containing the address, length and
- *          filename (with the directory path cut off) of the found file.
- *          If you search for a filename and not for files in a directory,
- *          pass the absolute path of the filename in the cpio and make sure
- *          the match returned an empty filename string.
+ * @return:     struct cpio_data containing the address, length and
+ *              filename (with the directory path cut off) of the found file.
+ *              If you search for a filename and not for files in a directory,
+ *              pass the absolute path of the filename in the cpio and make sure
+ *              the match returned an empty filename string.
  */
 
 struct cpio_data find_cpio_data(const char *path, void *data,
-					  size_t len,  long *offset)
+				size_t len,  long *nextoff)
 {
 	const size_t cpio_header_len = 8*C_NFIELDS - 2;
 	struct cpio_data cd = { NULL, 0, "" };
@@ -124,7 +125,7 @@ struct cpio_data find_cpio_data(const char *path, void *data,
 		if ((ch[C_MODE] & 0170000) == 0100000 &&
 		    ch[C_NAMESIZE] >= mypathsize &&
 		    !memcmp(p, path, mypathsize)) {
-			*offset = (long)nptr - (long)data;
+			*nextoff = (long)nptr - (long)data;
 			if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
 				pr_warn(
 				"File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
-- 
1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH part2 2/4] earlycpio.c: Fix the confusing comment of find_cpio_data().

[Tang Chen]

The comments of find_cpio_data() says:

  * @offset: When a matching file is found, this is the offset to the
  *          beginning of the cpio. ......

But according to the code,

  dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
  nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
  ....
  *offset = (long)nptr - (long)data;	/* data is the cpio file */

@offset is the offset of the next file, not the matching file itself.
This is confused and may cause unnecessary waste of time to debug.
So fix it.

v1 -> v2:
As tj suggested, rename @offset to @nextoff which is more clear to
users. And also adjust the new comments.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 lib/earlycpio.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/earlycpio.c b/lib/earlycpio.c
index 7aa7ce2..c7ae5ed 100644
--- a/lib/earlycpio.c
+++ b/lib/earlycpio.c
@@ -49,22 +49,23 @@ enum cpio_fields {
 
 /**
  * cpio_data find_cpio_data - Search for files in an uncompressed cpio
- * @path:   The directory to search for, including a slash at the end
- * @data:   Pointer to the the cpio archive or a header inside
- * @len:    Remaining length of the cpio based on data pointer
- * @offset: When a matching file is found, this is the offset to the
- *          beginning of the cpio. It can be used to iterate through
- *          the cpio to find all files inside of a directory path
+ * @path:       The directory to search for, including a slash at the end
+ * @data:       Pointer to the the cpio archive or a header inside
+ * @len:        Remaining length of the cpio based on data pointer
+ * @nextoff:    When a matching file is found, this is the offset from the
+ *              beginning of the cpio to the beginning of the next file, not the
+ *              matching file itself. It can be used to iterate through the cpio
+ *              to find all files inside of a directory path 
  *
- * @return: struct cpio_data containing the address, length and
- *          filename (with the directory path cut off) of the found file.
- *          If you search for a filename and not for files in a directory,
- *          pass the absolute path of the filename in the cpio and make sure
- *          the match returned an empty filename string.
+ * @return:     struct cpio_data containing the address, length and
+ *              filename (with the directory path cut off) of the found file.
+ *              If you search for a filename and not for files in a directory,
+ *              pass the absolute path of the filename in the cpio and make sure
+ *              the match returned an empty filename string.
  */
 
 struct cpio_data find_cpio_data(const char *path, void *data,
-					  size_t len,  long *offset)
+				size_t len,  long *nextoff)
 {
 	const size_t cpio_header_len = 8*C_NFIELDS - 2;
 	struct cpio_data cd = { NULL, 0, "" };
@@ -124,7 +125,7 @@ struct cpio_data find_cpio_data(const char *path, void *data,
 		if ((ch[C_MODE] & 0170000) == 0100000 &&
 		    ch[C_NAMESIZE] >= mypathsize &&
 		    !memcmp(p, path, mypathsize)) {
-			*offset = (long)nptr - (long)data;
+			*nextoff = (long)nptr - (long)data;
 			if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
 				pr_warn(
 				"File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
-- 
1.7.1

[PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tang Chen]

The macro INVALID_TABLE() is defined like this:

 #define INVALID_TABLE(x, path, name)                                    \
         { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }

And it is used like this:

	for (...) {
		...
		if (...)
			INVALID_TABLE()
		...
	}

The "continue" in the macro makes the code hard to understand.
Change it to the style like other macros:

 #define INVALID_TABLE(x, path, name)                                    \
         do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

And also, INVALID_TABLE() is used to checkout acpi tables, so rename it to
ACPI_INVALID_TABLE(). This is suggested by Toshi Kani <toshi.kani@hp.com>.

So after this patch, this macro should be used like this:

	for (...) {
		...
		if (...) {
			ACPI_INVALID_TABLE()
			continue;
		}
		...
	}

Add the "continue" wherever the macro is called.
(For now, it is only called in acpi_initrd_override().)

The idea is from Yinghai Lu <yinghai@kernel.org>.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Toshi Kani <toshi.kani@hp.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 drivers/acpi/osl.c |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..3b8bab2 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
 	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
 
 /* Non-fatal errors: Affected tables/files are ignored */
-#define INVALID_TABLE(x, path, name)					\
-	{ pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
+#define ACPI_INVALID_TABLE(x, path, name)					\
+	do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
 
 #define ACPI_HEADER_SIZE sizeof(struct acpi_table_header)
 
@@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
 		data += offset;
 		size -= offset;
 
-		if (file.size < sizeof(struct acpi_table_header))
-			INVALID_TABLE("Table smaller than ACPI header",
+		if (file.size < sizeof(struct acpi_table_header)) {
+			ACPI_INVALID_TABLE("Table smaller than ACPI header",
 				      cpio_path, file.name);
+			continue;
+		}
 
 		table = file.data;
 
@@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)
 			if (!memcmp(table->signature, table_sigs[sig], 4))
 				break;
 
-		if (!table_sigs[sig])
-			INVALID_TABLE("Unknown signature",
+		if (!table_sigs[sig]) {
+			ACPI_INVALID_TABLE("Unknown signature",
 				      cpio_path, file.name);
-		if (file.size != table->length)
-			INVALID_TABLE("File length does not match table length",
+			continue;
+		}
+		if (file.size != table->length) {
+			ACPI_INVALID_TABLE("File length does not match table length",
 				      cpio_path, file.name);
-		if (acpi_table_checksum(file.data, table->length))
-			INVALID_TABLE("Bad table checksum",
+			continue;
+		}
+		if (acpi_table_checksum(file.data, table->length)) {
+			ACPI_INVALID_TABLE("Bad table checksum",
 				      cpio_path, file.name);
+			continue;
+		}
 
 		pr_info("%4.4s ACPI table found in initrd [%s%s][0x%x]\n",
 			table->signature, cpio_path, file.name, table->length);
-- 
1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tang Chen]

The macro INVALID_TABLE() is defined like this:

 #define INVALID_TABLE(x, path, name)                                    \
         { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }

And it is used like this:

	for (...) {
		...
		if (...)
			INVALID_TABLE()
		...
	}

The "continue" in the macro makes the code hard to understand.
Change it to the style like other macros:

 #define INVALID_TABLE(x, path, name)                                    \
         do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

And also, INVALID_TABLE() is used to checkout acpi tables, so rename it to
ACPI_INVALID_TABLE(). This is suggested by Toshi Kani <toshi.kani@hp.com>.

So after this patch, this macro should be used like this:

	for (...) {
		...
		if (...) {
			ACPI_INVALID_TABLE()
			continue;
		}
		...
	}

Add the "continue" wherever the macro is called.
(For now, it is only called in acpi_initrd_override().)

The idea is from Yinghai Lu <yinghai@kernel.org>.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Toshi Kani <toshi.kani@hp.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 drivers/acpi/osl.c |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..3b8bab2 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
 	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
 
 /* Non-fatal errors: Affected tables/files are ignored */
-#define INVALID_TABLE(x, path, name)					\
-	{ pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
+#define ACPI_INVALID_TABLE(x, path, name)					\
+	do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
 
 #define ACPI_HEADER_SIZE sizeof(struct acpi_table_header)
 
@@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
 		data += offset;
 		size -= offset;
 
-		if (file.size < sizeof(struct acpi_table_header))
-			INVALID_TABLE("Table smaller than ACPI header",
+		if (file.size < sizeof(struct acpi_table_header)) {
+			ACPI_INVALID_TABLE("Table smaller than ACPI header",
 				      cpio_path, file.name);
+			continue;
+		}
 
 		table = file.data;
 
@@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)
 			if (!memcmp(table->signature, table_sigs[sig], 4))
 				break;
 
-		if (!table_sigs[sig])
-			INVALID_TABLE("Unknown signature",
+		if (!table_sigs[sig]) {
+			ACPI_INVALID_TABLE("Unknown signature",
 				      cpio_path, file.name);
-		if (file.size != table->length)
-			INVALID_TABLE("File length does not match table length",
+			continue;
+		}
+		if (file.size != table->length) {
+			ACPI_INVALID_TABLE("File length does not match table length",
 				      cpio_path, file.name);
-		if (acpi_table_checksum(file.data, table->length))
-			INVALID_TABLE("Bad table checksum",
+			continue;
+		}
+		if (acpi_table_checksum(file.data, table->length)) {
+			ACPI_INVALID_TABLE("Bad table checksum",
 				      cpio_path, file.name);
+			continue;
+		}
 
 		pr_info("%4.4s ACPI table found in initrd [%s%s][0x%x]\n",
 			table->signature, cpio_path, file.name, table->length);
-- 
1.7.1

[PATCH part2 4/4] acpi: Introduce acpi_verify_initrd() to check if a table is invalid.

[Tang Chen]

In acpi_initrd_override(), it checks several things to ensure the
table it found is valid. In later patches, we need to do these check
somewhere else. So this patch introduces a common function
acpi_verify_table() to do all these checks, and reuse it in different
places. The function will be used in the subsequent patches.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
Acked-by: Toshi Kani <toshi.kani@hp.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |   86 +++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3b8bab2..0043e9f 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -572,9 +572,68 @@ static const char * const table_sigs[] = {
 /* Must not increase 10 or needs code modification below */
 #define ACPI_OVERRIDE_TABLES 10
 
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_verify_table
+ *
+ * PARAMETERS:  File               - The initrd file
+ *              Path               - Path to acpi overriding tables in cpio file
+ *              Signature          - Signature of the table
+ *
+ * RETURN:      0 if it passes all the checks, -EINVAL if any check fails.
+ *
+ * DESCRIPTION: Check if an acpi table found in initrd is invalid.
+ *              @signature can be NULL. If it is NULL, the function will check
+ *              if the table signature matches any signature in table_sigs[].
+ *
+ ******************************************************************************/
+int __init acpi_verify_table(struct cpio_data *file,
+			      const char *path, const char *signature)
+{
+	int idx;
+	struct acpi_table_header *table = file->data;
+
+	if (file->size < sizeof(struct acpi_table_header)) {
+		ACPI_INVALID_TABLE("Table smaller than ACPI header",
+			      path, file->name);
+		return -EINVAL;
+	}
+
+	if (signature) {
+		if (memcmp(table->signature, signature, 4)) {
+			ACPI_INVALID_TABLE("Table signature does not match",
+				      path, file->name);
+			return -EINVAL;
+		}
+	} else {
+		for (idx = 0; table_sigs[idx]; idx++)
+			if (!memcmp(table->signature, table_sigs[idx], 4))
+				break;
+
+		if (!table_sigs[idx]) {
+			ACPI_INVALID_TABLE("Unknown signature", path, file->name);
+			return -EINVAL;
+		}
+	}
+
+	if (file->size != table->length) {
+		ACPI_INVALID_TABLE("File length does not match table length",
+			      path, file->name);
+		return -EINVAL;
+	}
+
+	if (acpi_table_checksum(file->data, table->length)) {
+		ACPI_INVALID_TABLE("Bad table checksum",
+			      path, file->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 void __init acpi_initrd_override(void *data, size_t size)
 {
-	int sig, no, table_nr = 0, total_offset = 0;
+	int no, table_nr = 0, total_offset = 0;
 	long offset = 0;
 	struct acpi_table_header *table;
 	char cpio_path[32] = "kernel/firmware/acpi/";
@@ -593,33 +652,10 @@ void __init acpi_initrd_override(void *data, size_t size)
 		data += offset;
 		size -= offset;
 
-		if (file.size < sizeof(struct acpi_table_header)) {
-			ACPI_INVALID_TABLE("Table smaller than ACPI header",
-				      cpio_path, file.name);
-			continue;
-		}
-
 		table = file.data;
 
-		for (sig = 0; table_sigs[sig]; sig++)
-			if (!memcmp(table->signature, table_sigs[sig], 4))
-				break;
-
-		if (!table_sigs[sig]) {
-			ACPI_INVALID_TABLE("Unknown signature",
-				      cpio_path, file.name);
+		if (acpi_verify_table(&file, cpio_path, NULL))
 			continue;
-		}
-		if (file.size != table->length) {
-			ACPI_INVALID_TABLE("File length does not match table length",
-				      cpio_path, file.name);
-			continue;
-		}
-		if (acpi_table_checksum(file.data, table->length)) {
-			ACPI_INVALID_TABLE("Bad table checksum",
-				      cpio_path, file.name);
-			continue;
-		}
 
 		pr_info("%4.4s ACPI table found in initrd [%s%s][0x%x]\n",
 			table->signature, cpio_path, file.name, table->length);
-- 
1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH part2 4/4] acpi: Introduce acpi_verify_initrd() to check if a table is invalid.

[Tang Chen]

In acpi_initrd_override(), it checks several things to ensure the
table it found is valid. In later patches, we need to do these check
somewhere else. So this patch introduces a common function
acpi_verify_table() to do all these checks, and reuse it in different
places. The function will be used in the subsequent patches.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
Acked-by: Toshi Kani <toshi.kani@hp.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |   86 +++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3b8bab2..0043e9f 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -572,9 +572,68 @@ static const char * const table_sigs[] = {
 /* Must not increase 10 or needs code modification below */
 #define ACPI_OVERRIDE_TABLES 10
 
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_verify_table
+ *
+ * PARAMETERS:  File               - The initrd file
+ *              Path               - Path to acpi overriding tables in cpio file
+ *              Signature          - Signature of the table
+ *
+ * RETURN:      0 if it passes all the checks, -EINVAL if any check fails.
+ *
+ * DESCRIPTION: Check if an acpi table found in initrd is invalid.
+ *              @signature can be NULL. If it is NULL, the function will check
+ *              if the table signature matches any signature in table_sigs[].
+ *
+ ******************************************************************************/
+int __init acpi_verify_table(struct cpio_data *file,
+			      const char *path, const char *signature)
+{
+	int idx;
+	struct acpi_table_header *table = file->data;
+
+	if (file->size < sizeof(struct acpi_table_header)) {
+		ACPI_INVALID_TABLE("Table smaller than ACPI header",
+			      path, file->name);
+		return -EINVAL;
+	}
+
+	if (signature) {
+		if (memcmp(table->signature, signature, 4)) {
+			ACPI_INVALID_TABLE("Table signature does not match",
+				      path, file->name);
+			return -EINVAL;
+		}
+	} else {
+		for (idx = 0; table_sigs[idx]; idx++)
+			if (!memcmp(table->signature, table_sigs[idx], 4))
+				break;
+
+		if (!table_sigs[idx]) {
+			ACPI_INVALID_TABLE("Unknown signature", path, file->name);
+			return -EINVAL;
+		}
+	}
+
+	if (file->size != table->length) {
+		ACPI_INVALID_TABLE("File length does not match table length",
+			      path, file->name);
+		return -EINVAL;
+	}
+
+	if (acpi_table_checksum(file->data, table->length)) {
+		ACPI_INVALID_TABLE("Bad table checksum",
+			      path, file->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 void __init acpi_initrd_override(void *data, size_t size)
 {
-	int sig, no, table_nr = 0, total_offset = 0;
+	int no, table_nr = 0, total_offset = 0;
 	long offset = 0;
 	struct acpi_table_header *table;
 	char cpio_path[32] = "kernel/firmware/acpi/";
@@ -593,33 +652,10 @@ void __init acpi_initrd_override(void *data, size_t size)
 		data += offset;
 		size -= offset;
 
-		if (file.size < sizeof(struct acpi_table_header)) {
-			ACPI_INVALID_TABLE("Table smaller than ACPI header",
-				      cpio_path, file.name);
-			continue;
-		}
-
 		table = file.data;
 
-		for (sig = 0; table_sigs[sig]; sig++)
-			if (!memcmp(table->signature, table_sigs[sig], 4))
-				break;
-
-		if (!table_sigs[sig]) {
-			ACPI_INVALID_TABLE("Unknown signature",
-				      cpio_path, file.name);
+		if (acpi_verify_table(&file, cpio_path, NULL))
 			continue;
-		}
-		if (file.size != table->length) {
-			ACPI_INVALID_TABLE("File length does not match table length",
-				      cpio_path, file.name);
-			continue;
-		}
-		if (acpi_table_checksum(file.data, table->length)) {
-			ACPI_INVALID_TABLE("Bad table checksum",
-				      cpio_path, file.name);
-			continue;
-		}
 
 		pr_info("%4.4s ACPI table found in initrd [%s%s][0x%x]\n",
 			table->signature, cpio_path, file.name, table->length);
-- 
1.7.1

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Joe Perches]

On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:

> Change it to the style like other macros:
> 
>  #define INVALID_TABLE(x, path, name)                                    \
>          do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Single statement macros do _not_ need to use 
	"do { foo(); } while (0)"
and should be written as
	"foo()"

> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
[]
> @@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
>  	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
>  
>  /* Non-fatal errors: Affected tables/files are ignored */
> -#define INVALID_TABLE(x, path, name)					\
> -	{ pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
> +#define ACPI_INVALID_TABLE(x, path, name)					\
> +	do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Just remove the silly macro altogether

> @@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
[]
> -		if (file.size < sizeof(struct acpi_table_header))
> -			INVALID_TABLE("Table smaller than ACPI header",
> +		if (file.size < sizeof(struct acpi_table_header)) {
> +			ACPI_INVALID_TABLE("Table smaller than ACPI header",
>  				      cpio_path, file.name);

and use the normal style

			pr_err("ACPI OVERRIDE: Table smaller than ACPI header [%s%s]\n",
			       cpio_path, file.name);

> @@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)

etc...

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Joe Perches]

On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:

> Change it to the style like other macros:
> 
>  #define INVALID_TABLE(x, path, name)                                    \
>          do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Single statement macros do _not_ need to use 
	"do { foo(); } while (0)"
and should be written as
	"foo()"

> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
[]
> @@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
>  	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
>  
>  /* Non-fatal errors: Affected tables/files are ignored */
> -#define INVALID_TABLE(x, path, name)					\
> -	{ pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
> +#define ACPI_INVALID_TABLE(x, path, name)					\
> +	do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Just remove the silly macro altogether

> @@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
[]
> -		if (file.size < sizeof(struct acpi_table_header))
> -			INVALID_TABLE("Table smaller than ACPI header",
> +		if (file.size < sizeof(struct acpi_table_header)) {
> +			ACPI_INVALID_TABLE("Table smaller than ACPI header",
>  				      cpio_path, file.name);

and use the normal style

			pr_err("ACPI OVERRIDE: Table smaller than ACPI header [%s%s]\n",
			       cpio_path, file.name);

> @@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)

etc...

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tang Chen]

Hi Joe,


On 08/08/2013 01:27 PM, Joe Perches wrote:
> On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:
>
>> Change it to the style like other macros:
>>
>>   #define INVALID_TABLE(x, path, name)                                    \
>>           do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
>
> Single statement macros do _not_ need to use
> 	"do { foo(); } while (0)"
> and should be written as
> 	"foo()"

OK, will remove the do {} while (0).

But I think we'd better keep the macro, or rename it to something
more meaningful. At least we can use it to avoid adding "ACPI OVERRIDE:"
prefix every time. Maybe this is why it is defined.

Thanks. :)

>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> []
>> @@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
>>   	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
>>
>>   /* Non-fatal errors: Affected tables/files are ignored */
>> -#define INVALID_TABLE(x, path, name)					\
>> -	{ pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
>> +#define ACPI_INVALID_TABLE(x, path, name)					\
>> +	do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
>
> Just remove the silly macro altogether
>
>> @@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
> []
>> -		if (file.size<  sizeof(struct acpi_table_header))
>> -			INVALID_TABLE("Table smaller than ACPI header",
>> +		if (file.size<  sizeof(struct acpi_table_header)) {
>> +			ACPI_INVALID_TABLE("Table smaller than ACPI header",
>>   				      cpio_path, file.name);
>
> and use the normal style
>
> 			pr_err("ACPI OVERRIDE: Table smaller than ACPI header [%s%s]\n",
> 			       cpio_path, file.name);
>
>> @@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)
>
> etc...
>
>

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tang Chen]

Hi Joe,


On 08/08/2013 01:27 PM, Joe Perches wrote:
> On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:
>
>> Change it to the style like other macros:
>>
>>   #define INVALID_TABLE(x, path, name)                                    \
>>           do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
>
> Single statement macros do _not_ need to use
> 	"do { foo(); } while (0)"
> and should be written as
> 	"foo()"

OK, will remove the do {} while (0).

But I think we'd better keep the macro, or rename it to something
more meaningful. At least we can use it to avoid adding "ACPI OVERRIDE:"
prefix every time. Maybe this is why it is defined.

Thanks. :)

>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> []
>> @@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
>>   	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
>>
>>   /* Non-fatal errors: Affected tables/files are ignored */
>> -#define INVALID_TABLE(x, path, name)					\
>> -	{ pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
>> +#define ACPI_INVALID_TABLE(x, path, name)					\
>> +	do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
>
> Just remove the silly macro altogether
>
>> @@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
> []
>> -		if (file.size<  sizeof(struct acpi_table_header))
>> -			INVALID_TABLE("Table smaller than ACPI header",
>> +		if (file.size<  sizeof(struct acpi_table_header)) {
>> +			ACPI_INVALID_TABLE("Table smaller than ACPI header",
>>   				      cpio_path, file.name);
>
> and use the normal style
>
> 			pr_err("ACPI OVERRIDE: Table smaller than ACPI header [%s%s]\n",
> 			       cpio_path, file.name);
>
>> @@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)
>
> etc...
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Rafael J. Wysocki]

On Thursday, August 08, 2013 01:03:55 PM Tang Chen wrote:
> This patch-set does some trivial fix and improving in ACPI code
> for memory hotplug.
> 
> Patch 1,3,4 have been acked.
> 
> Tang Chen (4):
>   acpi: Print Hot-Pluggable Field in SRAT.
>   earlycpio.c: Fix the confusing comment of find_cpio_data().
>   acpi: Remove "continue" in macro INVALID_TABLE().
>   acpi: Introduce acpi_verify_initrd() to check if a table is invalid.
> 
>  arch/x86/mm/srat.c |   11 ++++--
>  drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
>  lib/earlycpio.c    |   27 ++++++++--------
>  3 files changed, 85 insertions(+), 37 deletions(-)

It looks like this part doesn't depend on the other parts, is that correct?

Rafael

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Rafael J. Wysocki]

On Thursday, August 08, 2013 01:03:55 PM Tang Chen wrote:
> This patch-set does some trivial fix and improving in ACPI code
> for memory hotplug.
> 
> Patch 1,3,4 have been acked.
> 
> Tang Chen (4):
>   acpi: Print Hot-Pluggable Field in SRAT.
>   earlycpio.c: Fix the confusing comment of find_cpio_data().
>   acpi: Remove "continue" in macro INVALID_TABLE().
>   acpi: Introduce acpi_verify_initrd() to check if a table is invalid.
> 
>  arch/x86/mm/srat.c |   11 ++++--
>  drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
>  lib/earlycpio.c    |   27 ++++++++--------
>  3 files changed, 85 insertions(+), 37 deletions(-)

It looks like this part doesn't depend on the other parts, is that correct?

Rafael

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Joe Perches]

On Thu, 2013-08-08 at 20:18 +0800, Tang Chen wrote:
> Hi Joe,

Hello Tang.

> On 08/08/2013 01:27 PM, Joe Perches wrote:
> > On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:
> >
> >> Change it to the style like other macros:
> >>
> >>   #define INVALID_TABLE(x, path, name)                                    \
> >>           do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
> >
> > Single statement macros do _not_ need to use
> > 	"do { foo(); } while (0)"
> > and should be written as
> > 	"foo()"
> 
> OK, will remove the do {} while (0).
> 
> But I think we'd better keep the macro, or rename it to something
> more meaningful. At least we can use it to avoid adding "ACPI OVERRIDE:"
> prefix every time. Maybe this is why it is defined.

No, it's just silly.

If you really think that the #define is better, use
something like HW_ERR does and embed that #define
in the pr_err.

#define ACPI_OVERRIDE	"ACPI OVERRIDE: "

	pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
	       cpio_path, file.name);

It's only used a few times by a single file so
I think it's unnecessary.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Joe Perches]

On Thu, 2013-08-08 at 20:18 +0800, Tang Chen wrote:
> Hi Joe,

Hello Tang.

> On 08/08/2013 01:27 PM, Joe Perches wrote:
> > On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:
> >
> >> Change it to the style like other macros:
> >>
> >>   #define INVALID_TABLE(x, path, name)                                    \
> >>           do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
> >
> > Single statement macros do _not_ need to use
> > 	"do { foo(); } while (0)"
> > and should be written as
> > 	"foo()"
> 
> OK, will remove the do {} while (0).
> 
> But I think we'd better keep the macro, or rename it to something
> more meaningful. At least we can use it to avoid adding "ACPI OVERRIDE:"
> prefix every time. Maybe this is why it is defined.

No, it's just silly.

If you really think that the #define is better, use
something like HW_ERR does and embed that #define
in the pr_err.

#define ACPI_OVERRIDE	"ACPI OVERRIDE: "

	pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
	       cpio_path, file.name);

It's only used a few times by a single file so
I think it's unnecessary.

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Tang Chen]

On 08/08/2013 10:09 PM, Rafael J. Wysocki wrote:
> On Thursday, August 08, 2013 01:03:55 PM Tang Chen wrote:
>> This patch-set does some trivial fix and improving in ACPI code
>> for memory hotplug.
>>
>> Patch 1,3,4 have been acked.
>>
>> Tang Chen (4):
>>    acpi: Print Hot-Pluggable Field in SRAT.
>>    earlycpio.c: Fix the confusing comment of find_cpio_data().
>>    acpi: Remove "continue" in macro INVALID_TABLE().
>>    acpi: Introduce acpi_verify_initrd() to check if a table is invalid.
>>
>>   arch/x86/mm/srat.c |   11 ++++--
>>   drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
>>   lib/earlycpio.c    |   27 ++++++++--------
>>   3 files changed, 85 insertions(+), 37 deletions(-)
>
> It looks like this part doesn't depend on the other parts, is that correct?

No, it doesn't. And this patch-set can be merged first.

Thanks.

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Tang Chen]

On 08/08/2013 10:09 PM, Rafael J. Wysocki wrote:
> On Thursday, August 08, 2013 01:03:55 PM Tang Chen wrote:
>> This patch-set does some trivial fix and improving in ACPI code
>> for memory hotplug.
>>
>> Patch 1,3,4 have been acked.
>>
>> Tang Chen (4):
>>    acpi: Print Hot-Pluggable Field in SRAT.
>>    earlycpio.c: Fix the confusing comment of find_cpio_data().
>>    acpi: Remove "continue" in macro INVALID_TABLE().
>>    acpi: Introduce acpi_verify_initrd() to check if a table is invalid.
>>
>>   arch/x86/mm/srat.c |   11 ++++--
>>   drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
>>   lib/earlycpio.c    |   27 ++++++++--------
>>   3 files changed, 85 insertions(+), 37 deletions(-)
>
> It looks like this part doesn't depend on the other parts, is that correct?

No, it doesn't. And this patch-set can be merged first.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Rafael J. Wysocki]

On Friday, August 09, 2013 08:37:29 AM Tang Chen wrote:
> On 08/08/2013 10:09 PM, Rafael J. Wysocki wrote:
> > On Thursday, August 08, 2013 01:03:55 PM Tang Chen wrote:
> >> This patch-set does some trivial fix and improving in ACPI code
> >> for memory hotplug.
> >>
> >> Patch 1,3,4 have been acked.
> >>
> >> Tang Chen (4):
> >>    acpi: Print Hot-Pluggable Field in SRAT.
> >>    earlycpio.c: Fix the confusing comment of find_cpio_data().
> >>    acpi: Remove "continue" in macro INVALID_TABLE().
> >>    acpi: Introduce acpi_verify_initrd() to check if a table is invalid.
> >>
> >>   arch/x86/mm/srat.c |   11 ++++--
> >>   drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
> >>   lib/earlycpio.c    |   27 ++++++++--------
> >>   3 files changed, 85 insertions(+), 37 deletions(-)
> >
> > It looks like this part doesn't depend on the other parts, is that correct?
> 
> No, it doesn't. And this patch-set can be merged first.

OK, so if nobody objects, I can take patches [1,3-4/4], but I don't think I'm
the right maintainer to handle [2/4].

Thanks,
Rafael

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Rafael J. Wysocki]

On Friday, August 09, 2013 08:37:29 AM Tang Chen wrote:
> On 08/08/2013 10:09 PM, Rafael J. Wysocki wrote:
> > On Thursday, August 08, 2013 01:03:55 PM Tang Chen wrote:
> >> This patch-set does some trivial fix and improving in ACPI code
> >> for memory hotplug.
> >>
> >> Patch 1,3,4 have been acked.
> >>
> >> Tang Chen (4):
> >>    acpi: Print Hot-Pluggable Field in SRAT.
> >>    earlycpio.c: Fix the confusing comment of find_cpio_data().
> >>    acpi: Remove "continue" in macro INVALID_TABLE().
> >>    acpi: Introduce acpi_verify_initrd() to check if a table is invalid.
> >>
> >>   arch/x86/mm/srat.c |   11 ++++--
> >>   drivers/acpi/osl.c |   84 +++++++++++++++++++++++++++++++++++++++------------
> >>   lib/earlycpio.c    |   27 ++++++++--------
> >>   3 files changed, 85 insertions(+), 37 deletions(-)
> >
> > It looks like this part doesn't depend on the other parts, is that correct?
> 
> No, it doesn't. And this patch-set can be merged first.

OK, so if nobody objects, I can take patches [1,3-4/4], but I don't think I'm
the right maintainer to handle [2/4].

Thanks,
Rafael

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Tejun Heo]

On Fri, Aug 09, 2013 at 03:36:16PM +0200, Rafael J. Wysocki wrote:
> > No, it doesn't. And this patch-set can be merged first.
> 
> OK, so if nobody objects, I can take patches [1,3-4/4], but I don't think I'm
> the right maintainer to handle [2/4].

Given the dependencies, we'll probably need some coordination among
trees.  It spans across ACPI, memblock and x86.  Maybe the best way to
do it is applying the ACPI part to your tree, pulling the rest into a
tip branch and then put everything else there.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 0/4] acpi: Trivial fix and improving for memory hotplug.

[Tejun Heo]

On Fri, Aug 09, 2013 at 03:36:16PM +0200, Rafael J. Wysocki wrote:
> > No, it doesn't. And this patch-set can be merged first.
> 
> OK, so if nobody objects, I can take patches [1,3-4/4], but I don't think I'm
> the right maintainer to handle [2/4].

Given the dependencies, we'll probably need some coordination among
trees.  It spans across ACPI, memblock and x86.  Maybe the best way to
do it is applying the ACPI part to your tree, pulling the rest into a
tip branch and then put everything else there.

Thanks.

-- 
tejun

Re: [PATCH part2 1/4] acpi: Print Hot-Pluggable Field in SRAT.

[Tejun Heo]

On Thu, Aug 08, 2013 at 01:03:56PM +0800, Tang Chen wrote:
> +	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n",
> +		node, pxm,
> +		(unsigned long long) start, (unsigned long long) end - 1,
> +		hotpluggable ? " Hot Pluggable" : "");

Wouldn't it be better to just print "hotplug"?

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 1/4] acpi: Print Hot-Pluggable Field in SRAT.

[Tejun Heo]

On Thu, Aug 08, 2013 at 01:03:56PM +0800, Tang Chen wrote:
> +	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n",
> +		node, pxm,
> +		(unsigned long long) start, (unsigned long long) end - 1,
> +		hotpluggable ? " Hot Pluggable" : "");

Wouldn't it be better to just print "hotplug"?

Thanks.

-- 
tejun

Re: [PATCH part2 2/4] earlycpio.c: Fix the confusing comment of find_cpio_data().

[Tejun Heo]

On Thu, Aug 08, 2013 at 01:03:57PM +0800, Tang Chen wrote:
> The comments of find_cpio_data() says:
> 
>   * @offset: When a matching file is found, this is the offset to the
>   *          beginning of the cpio. ......
> 
> But according to the code,
> 
>   dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
>   nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
>   ....
>   *offset = (long)nptr - (long)data;	/* data is the cpio file */
> 
> @offset is the offset of the next file, not the matching file itself.
> This is confused and may cause unnecessary waste of time to debug.
> So fix it.
> 
> v1 -> v2:
> As tj suggested, rename @offset to @nextoff which is more clear to
> users. And also adjust the new comments.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH part2 2/4] earlycpio.c: Fix the confusing comment of find_cpio_data().

[Tejun Heo]

On Thu, Aug 08, 2013 at 01:03:57PM +0800, Tang Chen wrote:
> The comments of find_cpio_data() says:
> 
>   * @offset: When a matching file is found, this is the offset to the
>   *          beginning of the cpio. ......
> 
> But according to the code,
> 
>   dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
>   nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
>   ....
>   *offset = (long)nptr - (long)data;	/* data is the cpio file */
> 
> @offset is the offset of the next file, not the matching file itself.
> This is confused and may cause unnecessary waste of time to debug.
> So fix it.
> 
> v1 -> v2:
> As tj suggested, rename @offset to @nextoff which is more clear to
> users. And also adjust the new comments.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tejun Heo]

On Thu, Aug 08, 2013 at 07:09:53AM -0700, Joe Perches wrote:
> If you really think that the #define is better, use
> something like HW_ERR does and embed that #define
> in the pr_err.
> 
> #define ACPI_OVERRIDE	"ACPI OVERRIDE: "
> 
> 	pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
> 	       cpio_path, file.name);
> 
> It's only used a few times by a single file so
> I think it's unnecessary.

I agree with Joe here.  Just doing normal pr_err() should be enough.
You can use pr_fmt() to add headers but given that we aren't talking
about huge number of printks, that probably is an overkill too.

Thanks.

-- 
tejun

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tejun Heo]

On Thu, Aug 08, 2013 at 07:09:53AM -0700, Joe Perches wrote:
> If you really think that the #define is better, use
> something like HW_ERR does and embed that #define
> in the pr_err.
> 
> #define ACPI_OVERRIDE	"ACPI OVERRIDE: "
> 
> 	pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
> 	       cpio_path, file.name);
> 
> It's only used a few times by a single file so
> I think it's unnecessary.

I agree with Joe here.  Just doing normal pr_err() should be enough.
You can use pr_fmt() to add headers but given that we aren't talking
about huge number of printks, that probably is an overkill too.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 1/4] acpi: Print Hot-Pluggable Field in SRAT.

[Tang Chen]

On 08/12/2013 10:15 PM, Tejun Heo wrote:
> On Thu, Aug 08, 2013 at 01:03:56PM +0800, Tang Chen wrote:
>> +	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n",
>> +		node, pxm,
>> +		(unsigned long long) start, (unsigned long long) end - 1,
>> +		hotpluggable ? " Hot Pluggable" : "");
>
> Wouldn't it be better to just print "hotplug"?

Sure, followed.

Thanks.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 1/4] acpi: Print Hot-Pluggable Field in SRAT.

[Tang Chen]

On 08/12/2013 10:15 PM, Tejun Heo wrote:
> On Thu, Aug 08, 2013 at 01:03:56PM +0800, Tang Chen wrote:
>> +	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s\n",
>> +		node, pxm,
>> +		(unsigned long long) start, (unsigned long long) end - 1,
>> +		hotpluggable ? " Hot Pluggable" : "");
>
> Wouldn't it be better to just print "hotplug"?

Sure, followed.

Thanks.

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tang Chen]

On 08/12/2013 10:21 PM, Tejun Heo wrote:
> On Thu, Aug 08, 2013 at 07:09:53AM -0700, Joe Perches wrote:
>> If you really think that the #define is better, use
>> something like HW_ERR does and embed that #define
>> in the pr_err.
>>
>> #define ACPI_OVERRIDE	"ACPI OVERRIDE: "
>>
>> 	pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
>> 	       cpio_path, file.name);
>>
>> It's only used a few times by a single file so
>> I think it's unnecessary.
>
> I agree with Joe here.  Just doing normal pr_err() should be enough.
> You can use pr_fmt() to add headers but given that we aren't talking
> about huge number of printks, that probably is an overkill too.

OK, followed.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

[Tang Chen]

On 08/12/2013 10:21 PM, Tejun Heo wrote:
> On Thu, Aug 08, 2013 at 07:09:53AM -0700, Joe Perches wrote:
>> If you really think that the #define is better, use
>> something like HW_ERR does and embed that #define
>> in the pr_err.
>>
>> #define ACPI_OVERRIDE	"ACPI OVERRIDE: "
>>
>> 	pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
>> 	       cpio_path, file.name);
>>
>> It's only used a few times by a single file so
>> I think it's unnecessary.
>
> I agree with Joe here.  Just doing normal pr_err() should be enough.
> You can use pr_fmt() to add headers but given that we aren't talking
> about huge number of printks, that probably is an overkill too.

OK, followed.

Thanks.