[RFC PATCH 0/2] x86/boot: early ACPI MADT processing cleanup

[RFC PATCH 0/2] x86/boot: early ACPI MADT processing cleanup

[Cao jin]

Logic in early_acpi_process_madt() & acpi_process_madt() is really hard to
follow now. Clean them up.

Done basic boot test on my x86-64 PC.

CCed linux-acpi@vger.kernel.org

Cao jin (2):
  x86/acpi: Improve code readablity of early madt processing
  x86/acpi: Cleanup acpi_process_madt()

 arch/x86/kernel/acpi/boot.c | 72 +++++++++++++++----------------------
 1 file changed, 28 insertions(+), 44 deletions(-)

-- 
2.21.0

[RFC PATCH 1/2] x86/acpi: Improve code readablity of early madt processing

[Cao jin]

Current processing logic is confusing.

Return value of early_acpi_parse_madt_lapic_addr_ovr() indicates error(< 0),
parsed entry number(>= 0). So, it makes no sense to initialize acpi_lapic &
smp_found_config seeing no override entry, instead, initialize them seeing
MADT.

Put register_lapic_address() into override entry processing doesn't make
sense either.

Improved all the related comments too.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/acpi/boot.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 04205ce127a1..2131035bba98 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1003,11 +1003,7 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
 }
 
 #ifdef	CONFIG_X86_LOCAL_APIC
-/*
- * Parse LAPIC entries in MADT
- * returns 0 on success, < 0 on error
- */
-
+/* Returns >= 0 on success, indicates parsed entry number; < 0 on error. */
 static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 {
 	int count;
@@ -1025,11 +1021,8 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 	if (count < 0) {
 		printk(KERN_ERR PREFIX
 		       "Error parsing LAPIC address override entry\n");
-		return count;
 	}
 
-	register_lapic_address(acpi_lapic_addr);
-
 	return count;
 }
 
@@ -1234,19 +1227,16 @@ static inline int acpi_parse_madt_ioapic_entries(void)
 static void __init early_acpi_process_madt(void)
 {
 #ifdef CONFIG_X86_LOCAL_APIC
-	int error;
+	int ret;
 
 	if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) {
+		/* Found MADT */
+		acpi_lapic = 1;
+		smp_found_config = 1;
 
-		/*
-		 * Parse MADT LAPIC entries
-		 */
-		error = early_acpi_parse_madt_lapic_addr_ovr();
-		if (!error) {
-			acpi_lapic = 1;
-			smp_found_config = 1;
-		}
-		if (error == -EINVAL) {
+		/* See if override entry exists. */
+		ret = early_acpi_parse_madt_lapic_addr_ovr();
+		if (ret == -EINVAL) {
 			/*
 			 * Dell Precision Workstation 410, 610 come here.
 			 */
@@ -1254,6 +1244,8 @@ static void __init early_acpi_process_madt(void)
 			       "Invalid BIOS MADT, disabling ACPI\n");
 			disable_acpi();
 		}
+
+		register_lapic_address(acpi_lapic_addr);
 	}
 #endif
 }
-- 
2.21.0

[RFC PATCH 2/2] x86/acpi: Cleanup acpi_process_madt()

[Cao jin]

After many duct-taped patches, the readablity of these code chunks are
really hard. This patch does following improvements:

  1. Drop unnecessary comment that is self-documented by function name,
     while supply with necessary comments.
  2. Drop duplicated code: acpi_process_madt() has already been called
     in early madt processing, acpi_lapic & smp_found_config is also
     initialized there.
  3. Fix code logic: variable count's usage is quite confusing now, and
     some code logic is wrong, like count & x2count will never be negative
     after assigned from madt_proc[n].count

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/acpi/boot.c | 44 +++++++++++++++----------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 2131035bba98..844fc9f26064 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1026,9 +1026,10 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 	return count;
 }
 
+/* Returns 0 on success, < 0 on error. */
 static int __init acpi_parse_madt_lapic_entries(void)
 {
-	int count;
+	int count = 0;
 	int x2count = 0;
 	int ret;
 	struct acpi_subtable_proc madt_proc[2];
@@ -1036,10 +1037,13 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (!boot_cpu_has(X86_FEATURE_APIC))
 		return -ENODEV;
 
-	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
-				      acpi_parse_sapic, MAX_LOCAL_APIC);
+	/* SAPIC is commonly found on Intel Itanium processor. */
+	ret = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
+				    acpi_parse_sapic, MAX_LOCAL_APIC);
 
-	if (!count) {
+
+	/* No SAPIC entries implies it is not Itanium processor. */
+	if (!ret) {
 		memset(madt_proc, 0, sizeof(madt_proc));
 		madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
 		madt_proc[0].handler = acpi_parse_lapic;
@@ -1056,15 +1060,14 @@ static int __init acpi_parse_madt_lapic_entries(void)
 
 		count = madt_proc[0].count;
 		x2count = madt_proc[1].count;
-	}
-	if (!count && !x2count) {
-		printk(KERN_ERR PREFIX "No LAPIC entries present\n");
-		/* TBD: Cleanup to allow fallback to MPS */
-		return -ENODEV;
-	} else if (count < 0 || x2count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing LAPIC entry\n");
-		/* TBD: Cleanup to allow fallback to MPS */
-		return count;
+		if (!count && !x2count) {
+			printk(KERN_ERR PREFIX "No LAPIC entries present\n");
+			/* TBD: Cleanup to allow fallback to MPS */
+			return -ENODEV;
+		}
+	} else if (ret < 0) {
+		printk(KERN_ERR PREFIX "Error parsing SAPIC entries\n");
+		return ret;
 	}
 
 	x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI,
@@ -1074,7 +1077,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (count < 0 || x2count < 0) {
 		printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
-		return count;
+		return (count < 0) ? count : x2count;
 	}
 	return 0;
 }
@@ -1255,25 +1258,14 @@ static void __init acpi_process_madt(void)
 #ifdef CONFIG_X86_LOCAL_APIC
 	int error;
 
-	if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) {
-
-		/*
-		 * Parse MADT LAPIC entries
-		 */
+	if (acpi_lapic) {
 		error = acpi_parse_madt_lapic_entries();
 		if (!error) {
-			acpi_lapic = 1;
-
-			/*
-			 * Parse MADT IO-APIC entries
-			 */
 			mutex_lock(&acpi_ioapic_lock);
 			error = acpi_parse_madt_ioapic_entries();
 			mutex_unlock(&acpi_ioapic_lock);
 			if (!error) {
 				acpi_set_irq_model_ioapic();
-
-				smp_found_config = 1;
 			}
 		}
 		if (error == -EINVAL) {
-- 
2.21.0

Re: [RFC PATCH 0/2] x86/boot: early ACPI MADT processing cleanup

[Cao jin]

On 1/23/20 9:32 AM, Cao jin wrote:
> Logic in early_acpi_process_madt() & acpi_process_madt() is really hard to
> follow now. Clean them up.
> 
> Done basic boot test on my x86-64 PC.
> 
> CCed linux-acpi@vger.kernel.org
> 
I am sorry, I still missed that...
-- 
Sincerely,
Cao jin

[RFC PATCH 2/2] x86/acpi: Cleanup acpi_process_madt()

[Cao jin]

After many duct-taped patches, the readablity of these code chunks are
really hard. This patch does following improvements:

  1. Drop unnecessary comment that is self-documented by function name,
     while supply with necessary comments.
  2. Drop duplicated code: acpi_process_madt() has already been called
     in early madt processing, acpi_lapic & smp_found_config is also
     initialized there.
  3. Fix code logic: variable count's usage is quite confusing now, and
     some code logic is wrong, like count & x2count will never be negative
     after assigned from madt_proc[n].count

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/acpi/boot.c | 44 +++++++++++++++----------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 2131035bba98..844fc9f26064 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1026,9 +1026,10 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 	return count;
 }
 
+/* Returns 0 on success, < 0 on error. */
 static int __init acpi_parse_madt_lapic_entries(void)
 {
-	int count;
+	int count = 0;
 	int x2count = 0;
 	int ret;
 	struct acpi_subtable_proc madt_proc[2];
@@ -1036,10 +1037,13 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (!boot_cpu_has(X86_FEATURE_APIC))
 		return -ENODEV;
 
-	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
-				      acpi_parse_sapic, MAX_LOCAL_APIC);
+	/* SAPIC is commonly found on Intel Itanium processor. */
+	ret = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
+				    acpi_parse_sapic, MAX_LOCAL_APIC);
 
-	if (!count) {
+
+	/* No SAPIC entries implies it is not Itanium processor. */
+	if (!ret) {
 		memset(madt_proc, 0, sizeof(madt_proc));
 		madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
 		madt_proc[0].handler = acpi_parse_lapic;
@@ -1056,15 +1060,14 @@ static int __init acpi_parse_madt_lapic_entries(void)
 
 		count = madt_proc[0].count;
 		x2count = madt_proc[1].count;
-	}
-	if (!count && !x2count) {
-		printk(KERN_ERR PREFIX "No LAPIC entries present\n");
-		/* TBD: Cleanup to allow fallback to MPS */
-		return -ENODEV;
-	} else if (count < 0 || x2count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing LAPIC entry\n");
-		/* TBD: Cleanup to allow fallback to MPS */
-		return count;
+		if (!count && !x2count) {
+			printk(KERN_ERR PREFIX "No LAPIC entries present\n");
+			/* TBD: Cleanup to allow fallback to MPS */
+			return -ENODEV;
+		}
+	} else if (ret < 0) {
+		printk(KERN_ERR PREFIX "Error parsing SAPIC entries\n");
+		return ret;
 	}
 
 	x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI,
@@ -1074,7 +1077,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (count < 0 || x2count < 0) {
 		printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
-		return count;
+		return (count < 0) ? count : x2count;
 	}
 	return 0;
 }
@@ -1255,25 +1258,14 @@ static void __init acpi_process_madt(void)
 #ifdef CONFIG_X86_LOCAL_APIC
 	int error;
 
-	if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) {
-
-		/*
-		 * Parse MADT LAPIC entries
-		 */
+	if (acpi_lapic) {
 		error = acpi_parse_madt_lapic_entries();
 		if (!error) {
-			acpi_lapic = 1;
-
-			/*
-			 * Parse MADT IO-APIC entries
-			 */
 			mutex_lock(&acpi_ioapic_lock);
 			error = acpi_parse_madt_ioapic_entries();
 			mutex_unlock(&acpi_ioapic_lock);
 			if (!error) {
 				acpi_set_irq_model_ioapic();
-
-				smp_found_config = 1;
 			}
 		}
 		if (error == -EINVAL) {
-- 
2.21.0

[RFC PATCH 2/2] x86/acpi: Cleanup acpi_process_madt()

[Cao jin]

After many duct-taped patches, the readablity of these code chunks are
really hard. This patch does following improvements:

  1. Drop unnecessary comment that is self-documented by function name,
     while supply with necessary comments.
  2. Drop duplicated code: acpi_process_madt() has already been called
     in early madt processing, acpi_lapic & smp_found_config is also
     initialized there.
  3. Fix code logic: variable count's usage is quite confusing now, and
     some code logic is wrong, like count & x2count will never be negative
     after assigned from madt_proc[n].count

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/acpi/boot.c | 44 +++++++++++++++----------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 2131035bba98..844fc9f26064 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1026,9 +1026,10 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void)
 	return count;
 }
 
+/* Returns 0 on success, < 0 on error. */
 static int __init acpi_parse_madt_lapic_entries(void)
 {
-	int count;
+	int count = 0;
 	int x2count = 0;
 	int ret;
 	struct acpi_subtable_proc madt_proc[2];
@@ -1036,10 +1037,13 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (!boot_cpu_has(X86_FEATURE_APIC))
 		return -ENODEV;
 
-	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
-				      acpi_parse_sapic, MAX_LOCAL_APIC);
+	/* SAPIC is commonly found on Intel Itanium processor. */
+	ret = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
+				    acpi_parse_sapic, MAX_LOCAL_APIC);
 
-	if (!count) {
+
+	/* No SAPIC entries implies it is not Itanium processor. */
+	if (!ret) {
 		memset(madt_proc, 0, sizeof(madt_proc));
 		madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
 		madt_proc[0].handler = acpi_parse_lapic;
@@ -1056,15 +1060,14 @@ static int __init acpi_parse_madt_lapic_entries(void)
 
 		count = madt_proc[0].count;
 		x2count = madt_proc[1].count;
-	}
-	if (!count && !x2count) {
-		printk(KERN_ERR PREFIX "No LAPIC entries present\n");
-		/* TBD: Cleanup to allow fallback to MPS */
-		return -ENODEV;
-	} else if (count < 0 || x2count < 0) {
-		printk(KERN_ERR PREFIX "Error parsing LAPIC entry\n");
-		/* TBD: Cleanup to allow fallback to MPS */
-		return count;
+		if (!count && !x2count) {
+			printk(KERN_ERR PREFIX "No LAPIC entries present\n");
+			/* TBD: Cleanup to allow fallback to MPS */
+			return -ENODEV;
+		}
+	} else if (ret < 0) {
+		printk(KERN_ERR PREFIX "Error parsing SAPIC entries\n");
+		return ret;
 	}
 
 	x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI,
@@ -1074,7 +1077,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (count < 0 || x2count < 0) {
 		printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
-		return count;
+		return (count < 0) ? count : x2count;
 	}
 	return 0;
 }
@@ -1255,25 +1258,14 @@ static void __init acpi_process_madt(void)
 #ifdef CONFIG_X86_LOCAL_APIC
 	int error;
 
-	if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) {
-
-		/*
-		 * Parse MADT LAPIC entries
-		 */
+	if (acpi_lapic) {
 		error = acpi_parse_madt_lapic_entries();
 		if (!error) {
-			acpi_lapic = 1;
-
-			/*
-			 * Parse MADT IO-APIC entries
-			 */
 			mutex_lock(&acpi_ioapic_lock);
 			error = acpi_parse_madt_ioapic_entries();
 			mutex_unlock(&acpi_ioapic_lock);
 			if (!error) {
 				acpi_set_irq_model_ioapic();
-
-				smp_found_config = 1;
 			}
 		}
 		if (error == -EINVAL) {
-- 
2.21.0