Patchwork [3/4] acpi: mcfg: more code tidying.

login
register
mail settings
Submitter Colin King
Date Oct. 20, 2012, 7:54 p.m.
Message ID <1350762841-5702-4-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/192946/
State Accepted
Headers show

Comments

Colin King - Oct. 20, 2012, 7:54 p.m.
From: Colin Ian King <colin.king@canonical.com>

This code really needs a bit of a clean up.  Clean up argument
types in compare_config_space() and break up some overly long
lines.  Also improve the pretty printing of the configuration
data.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/mcfg/mcfg.c |   86 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 33 deletions(-)
Alex Hung - Oct. 25, 2012, 2:57 a.m.
On 10/21/2012 03:54 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> This code really needs a bit of a clean up.  Clean up argument
> types in compare_config_space() and break up some overly long
> lines.  Also improve the pretty printing of the configuration
> data.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpi/mcfg/mcfg.c |   86 +++++++++++++++++++++++++++++++-------------------
>   1 file changed, 53 insertions(+), 33 deletions(-)
>
> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
> index adc5e3a..1b96b26 100644
> --- a/src/acpi/mcfg/mcfg.c
> +++ b/src/acpi/mcfg/mcfg.c
> @@ -27,17 +27,22 @@
>   #include <string.h>
>   #include <unistd.h>
>   #include <inttypes.h>
> +#include <stdbool.h>
>
>   static fwts_list *memory_map_list;
>   static fwts_acpi_table_info *mcfg_table;
>
> -static void compare_config_space(fwts_framework *fw, int segment, int device, unsigned char *space)
> +static void compare_config_space(
> +	fwts_framework *fw,
> +	const uint16_t segment,
> +	const uint32_t device,
> +	const uint8_t *space)
>   {
>   	fwts_list *lspci_output;
>   	fwts_list_link *item;
>
>   	char command[PATH_MAX];
> -	char compare_line[1024];
> +	char compare_line[50];
>
>   	snprintf(compare_line, sizeof(compare_line),
>   		"%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x",
> @@ -46,7 +51,7 @@ static void compare_config_space(fwts_framework *fw, int segment, int device, un
>   		space[8],  space[9],  space[10], space[11],
>   		space[12], space[13], space[14], space[15]);
>
> -	snprintf(command, sizeof(command), "%s -vxxx -s %i:%i",
> +	snprintf(command, sizeof(command), "%s -vxxx -s %" PRIu16 ":%" PRIu32,
>   		fw->lspci, segment, device);
>
>   	if (fwts_pipe_exec(command, &lspci_output) == FWTS_EXEC_ERROR) {
> @@ -54,22 +59,31 @@ static void compare_config_space(fwts_framework *fw, int segment, int device, un
>   		return;
>   	}
>
> +	if (lspci_output == NULL) {
> +		fwts_log_warning(fw,
> +			"Failed to get lspci info for %" PRIu16 ":%" PRIu32,
> +			segment, device);
> +		return;
> +	}
> +
>   	fwts_list_foreach(item, lspci_output) {
>   		char *line = fwts_text_list_text(item);
>
>   		fwts_chop_newline(line);
>
> -		if (strncmp(line, "00: ",4)==0) {
> +		if (strncmp(line, "00: ", 4) ==0) {
>   			if (strcmp(&line[4], compare_line)) {
> -				fwts_log_info(fw, "%s is read from MMCONFIG, but traditional gives :\n-%s-\n", &line[4], compare_line);
> +				fwts_log_info(fw,
> +					"%s is read from MMCONFIG, but config "
> +					"space gives :\n-%s-\n", &line[4], compare_line);
>   				fwts_failed(fw, LOG_LEVEL_MEDIUM,
>   					"PCIConfigSpaceBad",
> -					"PCI config space appears to not work");
> +					"PCI config space appears to not work.");
>   			} else
> -				fwts_passed(fw, "PCI config space verified");
> +				fwts_passed(fw, "PCI config space verified.");
>
>   			fwts_text_list_free(lspci_output);
> -			return ;
> +			return;
>   		}
>   	}
>   	fwts_text_list_free(lspci_output);
> @@ -86,7 +100,9 @@ static int mcfg_init(fwts_framework *fw)
>   		return FWTS_ERROR;
>   	}
>   	if (mcfg_table == NULL) {
> -		fwts_log_error(fw, "ACPI table MCFG not found. This table is required to check for PCI Express*");
> +		fwts_log_error(fw,
> +			"ACPI table MCFG not found. This table is "
> +			"required to check for PCI Express*");
>   		return FWTS_ERROR;
>   	}
>
> @@ -104,10 +120,10 @@ static int mcfg_deinit(fwts_framework *fw)
>   static int mcfg_test1(fwts_framework *fw)
>   {
>   	int nr, i;
> -	void *mapped_table_page;
> +	uint8_t *mapped_table_page;
>   	fwts_acpi_table_mcfg *mcfg = (fwts_acpi_table_mcfg*)mcfg_table->data;
>   	fwts_acpi_mcfg_configuration *config;
> -	int failed = 0;
> +	bool failed = false;
>   	ssize_t mcfg_size;
>   	const char *memory_map_name;
>   	int page_size;
> @@ -143,9 +159,10 @@ static int mcfg_test1(fwts_framework *fw)
>   			mcfg_size + sizeof(fwts_acpi_table_mcfg));
>   		fwts_tag_failed(fw, FWTS_TAG_ACPI_INVALID_TABLE);
>   		fwts_advice(fw,
> -			"MCFG table must be least %d bytes (header size) with "
> -			"multiples of %d bytes for each MCFG entry.",
> -			36+8, (int)sizeof(fwts_acpi_mcfg_configuration));
> +			"MCFG table must be least %zd bytes (header size) with "
> +			"multiples of %zd bytes for each MCFG entry.",
> +			sizeof(fwts_acpi_table_mcfg),
> +			sizeof(fwts_acpi_mcfg_configuration));
>   		return FWTS_ERROR;
>   	}
>   	nr = mcfg_size / sizeof(fwts_acpi_mcfg_configuration);
> @@ -163,8 +180,9 @@ static int mcfg_test1(fwts_framework *fw)
>   		return FWTS_ERROR;
>   	}
>
> -	fwts_log_info(fw, "MCFG table found, size is %zd bytes (excluding header) (%i entries).",
> -			mcfg_size, nr);
> +	fwts_log_info(fw,
> +		"MCFG table found, size is %zd bytes (excluding header) (%i entries).",
> +		mcfg_size, nr);
>
>   	if (mcfg == NULL) {
>   		fwts_failed(fw, LOG_LEVEL_HIGH, "MCFGInvalidTable",
> @@ -179,7 +197,11 @@ static int mcfg_test1(fwts_framework *fw)
>
>   	config = &mcfg->configuration[0];
>   	for (i = 0; i < nr; i++, config++) {
> -		fwts_log_info(fw, "Configuration Entry #%d address : 0x%" PRIx64, i, config->base_address);
> +		fwts_log_info_verbatum(fw, "Configuration Entry #%d:", i);
> +		fwts_log_info_verbatum(fw, "  Base Address  : 0x%" PRIx64, config->base_address);
> +		fwts_log_info_verbatum(fw, "  Segment       : %" PRIu8, config->pci_segment_group_number);
> +		fwts_log_info_verbatum(fw, "  Start bus     : %" PRIu8, config->start_bus_number);
> +		fwts_log_info_verbatum(fw, "  End bus       : %" PRIu8, config->end_bus_number);
>
>   		if ((memory_map_list != NULL) &&
>   		    (!fwts_memory_map_is_reserved(memory_map_list, config->base_address))) {
> @@ -189,21 +211,22 @@ static int mcfg_test1(fwts_framework *fw)
>   				" is not reserved in the memory map table",
>   				config->base_address);
>   			fwts_tag_failed(fw, FWTS_TAG_BIOS);
> +
>   			fwts_advice(fw,
> -				"The PCI Express specification states that the PCI Express configuration space should "
> -				"be defined in the MCFG table and *maybe* optionally defined in the %s "
> -				"if ACPI MCFG is present. "
> -				"Linux checks if the region is reserved in the memory map table and will reject the "
> -				"MMCONFIG if there is a discrepency between MCFG and the memory map table for the "
> -				"PCI Express region.  [See arch/x86/pci/mmconfig-shared.c pci_mmcfg_reject_broken()]. "
> -				"It is recommended that this is defined in the %s table for Linux.",
> +				"The PCI Express specification states that the "
> +				"PCI Express configuration space should "
> +				"be defined in the MCFG table and *maybe* "
> +				"optionally defined in the %s if ACPI MCFG is "
> +				"present. Linux checks if the region is reserved "
> +				"in the memory map table and will reject the "
> +				"MMCONFIG if there is a discrepency between MCFG "
> +				"and the memory map table for the PCI Express region. "
> +				"[See arch/x86/pci/mmconfig-shared.c pci_mmcfg_reject_broken()]. "
> +				"It is recommended that this is defined in the "
> +				"%s table for Linux.",
>   				memory_map_name, memory_map_name);
> -			failed++;
> +			failed = true;
>   		}
> -
> -		fwts_log_info_verbatum(fw, "Segment       : %" PRIu8, config->pci_segment_group_number);
> -		fwts_log_info_verbatum(fw, "Start bus     : %" PRIu8, config->start_bus_number);
> -		fwts_log_info_verbatum(fw, "End bus       : %" PRIu8, config->end_bus_number);
>   	}
>   	if (!failed)
>   		fwts_passed(fw, "MCFG mmio config space is reserved in memory map table.");
> @@ -215,10 +238,7 @@ static int mcfg_test1(fwts_framework *fw)
>   		fwts_log_error(fw, "Cannot mmap table at 0x%" PRIx64 ".", config->base_address);
>   		return FWTS_ERROR;
>   	}
> -
> -	compare_config_space(fw, config->pci_segment_group_number,
> -		0, (unsigned char *)mapped_table_page);
> -
> +	compare_config_space(fw, config->pci_segment_group_number, 0, mapped_table_page);
>   	fwts_munmap(mapped_table_page, page_size);
>
>   	return FWTS_OK;
>
Acked-by: Alex Hung <alex.hung@canonical.com>
Keng-Yu Lin - Oct. 25, 2012, 7:58 a.m.
On Sun, Oct 21, 2012 at 3:54 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> This code really needs a bit of a clean up.  Clean up argument
> types in compare_config_space() and break up some overly long
> lines.  Also improve the pretty printing of the configuration
> data.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/acpi/mcfg/mcfg.c |   86 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 33 deletions(-)
>
> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
> index adc5e3a..1b96b26 100644
> --- a/src/acpi/mcfg/mcfg.c
> +++ b/src/acpi/mcfg/mcfg.c
> @@ -27,17 +27,22 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <inttypes.h>
> +#include <stdbool.h>
>
>  static fwts_list *memory_map_list;
>  static fwts_acpi_table_info *mcfg_table;
>
> -static void compare_config_space(fwts_framework *fw, int segment, int device, unsigned char *space)
> +static void compare_config_space(
> +       fwts_framework *fw,
> +       const uint16_t segment,
> +       const uint32_t device,
> +       const uint8_t *space)
>  {
>         fwts_list *lspci_output;
>         fwts_list_link *item;
>
>         char command[PATH_MAX];
> -       char compare_line[1024];
> +       char compare_line[50];
>
>         snprintf(compare_line, sizeof(compare_line),
>                 "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x",
> @@ -46,7 +51,7 @@ static void compare_config_space(fwts_framework *fw, int segment, int device, un
>                 space[8],  space[9],  space[10], space[11],
>                 space[12], space[13], space[14], space[15]);
>
> -       snprintf(command, sizeof(command), "%s -vxxx -s %i:%i",
> +       snprintf(command, sizeof(command), "%s -vxxx -s %" PRIu16 ":%" PRIu32,
>                 fw->lspci, segment, device);
>
>         if (fwts_pipe_exec(command, &lspci_output) == FWTS_EXEC_ERROR) {
> @@ -54,22 +59,31 @@ static void compare_config_space(fwts_framework *fw, int segment, int device, un
>                 return;
>         }
>
> +       if (lspci_output == NULL) {
> +               fwts_log_warning(fw,
> +                       "Failed to get lspci info for %" PRIu16 ":%" PRIu32,
> +                       segment, device);
> +               return;
> +       }
> +
>         fwts_list_foreach(item, lspci_output) {
>                 char *line = fwts_text_list_text(item);
>
>                 fwts_chop_newline(line);
>
> -               if (strncmp(line, "00: ",4)==0) {
> +               if (strncmp(line, "00: ", 4) ==0) {
>                         if (strcmp(&line[4], compare_line)) {
> -                               fwts_log_info(fw, "%s is read from MMCONFIG, but traditional gives :\n-%s-\n", &line[4], compare_line);
> +                               fwts_log_info(fw,
> +                                       "%s is read from MMCONFIG, but config "
> +                                       "space gives :\n-%s-\n", &line[4], compare_line);
>                                 fwts_failed(fw, LOG_LEVEL_MEDIUM,
>                                         "PCIConfigSpaceBad",
> -                                       "PCI config space appears to not work");
> +                                       "PCI config space appears to not work.");
>                         } else
> -                               fwts_passed(fw, "PCI config space verified");
> +                               fwts_passed(fw, "PCI config space verified.");
>
>                         fwts_text_list_free(lspci_output);
> -                       return ;
> +                       return;
>                 }
>         }
>         fwts_text_list_free(lspci_output);
> @@ -86,7 +100,9 @@ static int mcfg_init(fwts_framework *fw)
>                 return FWTS_ERROR;
>         }
>         if (mcfg_table == NULL) {
> -               fwts_log_error(fw, "ACPI table MCFG not found. This table is required to check for PCI Express*");
> +               fwts_log_error(fw,
> +                       "ACPI table MCFG not found. This table is "
> +                       "required to check for PCI Express*");
>                 return FWTS_ERROR;
>         }
>
> @@ -104,10 +120,10 @@ static int mcfg_deinit(fwts_framework *fw)
>  static int mcfg_test1(fwts_framework *fw)
>  {
>         int nr, i;
> -       void *mapped_table_page;
> +       uint8_t *mapped_table_page;
>         fwts_acpi_table_mcfg *mcfg = (fwts_acpi_table_mcfg*)mcfg_table->data;
>         fwts_acpi_mcfg_configuration *config;
> -       int failed = 0;
> +       bool failed = false;
>         ssize_t mcfg_size;
>         const char *memory_map_name;
>         int page_size;
> @@ -143,9 +159,10 @@ static int mcfg_test1(fwts_framework *fw)
>                         mcfg_size + sizeof(fwts_acpi_table_mcfg));
>                 fwts_tag_failed(fw, FWTS_TAG_ACPI_INVALID_TABLE);
>                 fwts_advice(fw,
> -                       "MCFG table must be least %d bytes (header size) with "
> -                       "multiples of %d bytes for each MCFG entry.",
> -                       36+8, (int)sizeof(fwts_acpi_mcfg_configuration));
> +                       "MCFG table must be least %zd bytes (header size) with "
> +                       "multiples of %zd bytes for each MCFG entry.",
> +                       sizeof(fwts_acpi_table_mcfg),
> +                       sizeof(fwts_acpi_mcfg_configuration));
>                 return FWTS_ERROR;
>         }
>         nr = mcfg_size / sizeof(fwts_acpi_mcfg_configuration);
> @@ -163,8 +180,9 @@ static int mcfg_test1(fwts_framework *fw)
>                 return FWTS_ERROR;
>         }
>
> -       fwts_log_info(fw, "MCFG table found, size is %zd bytes (excluding header) (%i entries).",
> -                       mcfg_size, nr);
> +       fwts_log_info(fw,
> +               "MCFG table found, size is %zd bytes (excluding header) (%i entries).",
> +               mcfg_size, nr);
>
>         if (mcfg == NULL) {
>                 fwts_failed(fw, LOG_LEVEL_HIGH, "MCFGInvalidTable",
> @@ -179,7 +197,11 @@ static int mcfg_test1(fwts_framework *fw)
>
>         config = &mcfg->configuration[0];
>         for (i = 0; i < nr; i++, config++) {
> -               fwts_log_info(fw, "Configuration Entry #%d address : 0x%" PRIx64, i, config->base_address);
> +               fwts_log_info_verbatum(fw, "Configuration Entry #%d:", i);
> +               fwts_log_info_verbatum(fw, "  Base Address  : 0x%" PRIx64, config->base_address);
> +               fwts_log_info_verbatum(fw, "  Segment       : %" PRIu8, config->pci_segment_group_number);
> +               fwts_log_info_verbatum(fw, "  Start bus     : %" PRIu8, config->start_bus_number);
> +               fwts_log_info_verbatum(fw, "  End bus       : %" PRIu8, config->end_bus_number);
>
>                 if ((memory_map_list != NULL) &&
>                     (!fwts_memory_map_is_reserved(memory_map_list, config->base_address))) {
> @@ -189,21 +211,22 @@ static int mcfg_test1(fwts_framework *fw)
>                                 " is not reserved in the memory map table",
>                                 config->base_address);
>                         fwts_tag_failed(fw, FWTS_TAG_BIOS);
> +
>                         fwts_advice(fw,
> -                               "The PCI Express specification states that the PCI Express configuration space should "
> -                               "be defined in the MCFG table and *maybe* optionally defined in the %s "
> -                               "if ACPI MCFG is present. "
> -                               "Linux checks if the region is reserved in the memory map table and will reject the "
> -                               "MMCONFIG if there is a discrepency between MCFG and the memory map table for the "
> -                               "PCI Express region.  [See arch/x86/pci/mmconfig-shared.c pci_mmcfg_reject_broken()]. "
> -                               "It is recommended that this is defined in the %s table for Linux.",
> +                               "The PCI Express specification states that the "
> +                               "PCI Express configuration space should "
> +                               "be defined in the MCFG table and *maybe* "
> +                               "optionally defined in the %s if ACPI MCFG is "
> +                               "present. Linux checks if the region is reserved "
> +                               "in the memory map table and will reject the "
> +                               "MMCONFIG if there is a discrepency between MCFG "
> +                               "and the memory map table for the PCI Express region. "
> +                               "[See arch/x86/pci/mmconfig-shared.c pci_mmcfg_reject_broken()]. "
> +                               "It is recommended that this is defined in the "
> +                               "%s table for Linux.",
>                                 memory_map_name, memory_map_name);
> -                       failed++;
> +                       failed = true;
>                 }
> -
> -               fwts_log_info_verbatum(fw, "Segment       : %" PRIu8, config->pci_segment_group_number);
> -               fwts_log_info_verbatum(fw, "Start bus     : %" PRIu8, config->start_bus_number);
> -               fwts_log_info_verbatum(fw, "End bus       : %" PRIu8, config->end_bus_number);
>         }
>         if (!failed)
>                 fwts_passed(fw, "MCFG mmio config space is reserved in memory map table.");
> @@ -215,10 +238,7 @@ static int mcfg_test1(fwts_framework *fw)
>                 fwts_log_error(fw, "Cannot mmap table at 0x%" PRIx64 ".", config->base_address);
>                 return FWTS_ERROR;
>         }
> -
> -       compare_config_space(fw, config->pci_segment_group_number,
> -               0, (unsigned char *)mapped_table_page);
> -
> +       compare_config_space(fw, config->pci_segment_group_number, 0, mapped_table_page);
>         fwts_munmap(mapped_table_page, page_size);
>
>         return FWTS_OK;
> --
> 1.7.10.4
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>

Patch

diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
index adc5e3a..1b96b26 100644
--- a/src/acpi/mcfg/mcfg.c
+++ b/src/acpi/mcfg/mcfg.c
@@ -27,17 +27,22 @@ 
 #include <string.h>
 #include <unistd.h>
 #include <inttypes.h>
+#include <stdbool.h>
 
 static fwts_list *memory_map_list;
 static fwts_acpi_table_info *mcfg_table;
 
-static void compare_config_space(fwts_framework *fw, int segment, int device, unsigned char *space)
+static void compare_config_space(
+	fwts_framework *fw,
+	const uint16_t segment,
+	const uint32_t device,
+	const uint8_t *space)
 {
 	fwts_list *lspci_output;
 	fwts_list_link *item;
 
 	char command[PATH_MAX];
-	char compare_line[1024];
+	char compare_line[50];
 
 	snprintf(compare_line, sizeof(compare_line),
 		"%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x",
@@ -46,7 +51,7 @@  static void compare_config_space(fwts_framework *fw, int segment, int device, un
 		space[8],  space[9],  space[10], space[11],
 		space[12], space[13], space[14], space[15]);
 
-	snprintf(command, sizeof(command), "%s -vxxx -s %i:%i",
+	snprintf(command, sizeof(command), "%s -vxxx -s %" PRIu16 ":%" PRIu32,
 		fw->lspci, segment, device);
 
 	if (fwts_pipe_exec(command, &lspci_output) == FWTS_EXEC_ERROR) {
@@ -54,22 +59,31 @@  static void compare_config_space(fwts_framework *fw, int segment, int device, un
 		return;
 	}
 
+	if (lspci_output == NULL) {
+		fwts_log_warning(fw,
+			"Failed to get lspci info for %" PRIu16 ":%" PRIu32,
+			segment, device);
+		return;
+	}
+
 	fwts_list_foreach(item, lspci_output) {
 		char *line = fwts_text_list_text(item);
 
 		fwts_chop_newline(line);
 
-		if (strncmp(line, "00: ",4)==0) {
+		if (strncmp(line, "00: ", 4) ==0) {
 			if (strcmp(&line[4], compare_line)) {
-				fwts_log_info(fw, "%s is read from MMCONFIG, but traditional gives :\n-%s-\n", &line[4], compare_line);
+				fwts_log_info(fw,
+					"%s is read from MMCONFIG, but config "
+					"space gives :\n-%s-\n", &line[4], compare_line);
 				fwts_failed(fw, LOG_LEVEL_MEDIUM,
 					"PCIConfigSpaceBad",
-					"PCI config space appears to not work");
+					"PCI config space appears to not work.");
 			} else
-				fwts_passed(fw, "PCI config space verified");
+				fwts_passed(fw, "PCI config space verified.");
 
 			fwts_text_list_free(lspci_output);
-			return ;
+			return;
 		}
 	}
 	fwts_text_list_free(lspci_output);
@@ -86,7 +100,9 @@  static int mcfg_init(fwts_framework *fw)
 		return FWTS_ERROR;
 	}
 	if (mcfg_table == NULL) {
-		fwts_log_error(fw, "ACPI table MCFG not found. This table is required to check for PCI Express*");
+		fwts_log_error(fw,
+			"ACPI table MCFG not found. This table is "
+			"required to check for PCI Express*");
 		return FWTS_ERROR;
 	}
 
@@ -104,10 +120,10 @@  static int mcfg_deinit(fwts_framework *fw)
 static int mcfg_test1(fwts_framework *fw)
 {
 	int nr, i;
-	void *mapped_table_page;
+	uint8_t *mapped_table_page;
 	fwts_acpi_table_mcfg *mcfg = (fwts_acpi_table_mcfg*)mcfg_table->data;
 	fwts_acpi_mcfg_configuration *config;
-	int failed = 0;
+	bool failed = false;
 	ssize_t mcfg_size;
 	const char *memory_map_name;
 	int page_size;
@@ -143,9 +159,10 @@  static int mcfg_test1(fwts_framework *fw)
 			mcfg_size + sizeof(fwts_acpi_table_mcfg));
 		fwts_tag_failed(fw, FWTS_TAG_ACPI_INVALID_TABLE);
 		fwts_advice(fw,
-			"MCFG table must be least %d bytes (header size) with "
-			"multiples of %d bytes for each MCFG entry.",
-			36+8, (int)sizeof(fwts_acpi_mcfg_configuration));
+			"MCFG table must be least %zd bytes (header size) with "
+			"multiples of %zd bytes for each MCFG entry.",
+			sizeof(fwts_acpi_table_mcfg),
+			sizeof(fwts_acpi_mcfg_configuration));
 		return FWTS_ERROR;
 	}
 	nr = mcfg_size / sizeof(fwts_acpi_mcfg_configuration);
@@ -163,8 +180,9 @@  static int mcfg_test1(fwts_framework *fw)
 		return FWTS_ERROR;
 	}
 
-	fwts_log_info(fw, "MCFG table found, size is %zd bytes (excluding header) (%i entries).",
-			mcfg_size, nr);
+	fwts_log_info(fw,
+		"MCFG table found, size is %zd bytes (excluding header) (%i entries).",
+		mcfg_size, nr);
 
 	if (mcfg == NULL) {
 		fwts_failed(fw, LOG_LEVEL_HIGH, "MCFGInvalidTable",
@@ -179,7 +197,11 @@  static int mcfg_test1(fwts_framework *fw)
 
 	config = &mcfg->configuration[0];
 	for (i = 0; i < nr; i++, config++) {
-		fwts_log_info(fw, "Configuration Entry #%d address : 0x%" PRIx64, i, config->base_address);
+		fwts_log_info_verbatum(fw, "Configuration Entry #%d:", i);
+		fwts_log_info_verbatum(fw, "  Base Address  : 0x%" PRIx64, config->base_address);
+		fwts_log_info_verbatum(fw, "  Segment       : %" PRIu8, config->pci_segment_group_number);
+		fwts_log_info_verbatum(fw, "  Start bus     : %" PRIu8, config->start_bus_number);
+		fwts_log_info_verbatum(fw, "  End bus       : %" PRIu8, config->end_bus_number);
 
 		if ((memory_map_list != NULL) &&
 		    (!fwts_memory_map_is_reserved(memory_map_list, config->base_address))) {
@@ -189,21 +211,22 @@  static int mcfg_test1(fwts_framework *fw)
 				" is not reserved in the memory map table",
 				config->base_address);
 			fwts_tag_failed(fw, FWTS_TAG_BIOS);
+
 			fwts_advice(fw,
-				"The PCI Express specification states that the PCI Express configuration space should "
-				"be defined in the MCFG table and *maybe* optionally defined in the %s "
-				"if ACPI MCFG is present. "
-				"Linux checks if the region is reserved in the memory map table and will reject the "
-				"MMCONFIG if there is a discrepency between MCFG and the memory map table for the "
-				"PCI Express region.  [See arch/x86/pci/mmconfig-shared.c pci_mmcfg_reject_broken()]. "
-				"It is recommended that this is defined in the %s table for Linux.",
+				"The PCI Express specification states that the "
+				"PCI Express configuration space should "
+				"be defined in the MCFG table and *maybe* "
+				"optionally defined in the %s if ACPI MCFG is "
+				"present. Linux checks if the region is reserved "
+				"in the memory map table and will reject the "
+				"MMCONFIG if there is a discrepency between MCFG "
+				"and the memory map table for the PCI Express region. "
+				"[See arch/x86/pci/mmconfig-shared.c pci_mmcfg_reject_broken()]. "
+				"It is recommended that this is defined in the "
+				"%s table for Linux.",
 				memory_map_name, memory_map_name);
-			failed++;
+			failed = true;
 		}
-
-		fwts_log_info_verbatum(fw, "Segment       : %" PRIu8, config->pci_segment_group_number);
-		fwts_log_info_verbatum(fw, "Start bus     : %" PRIu8, config->start_bus_number);
-		fwts_log_info_verbatum(fw, "End bus       : %" PRIu8, config->end_bus_number);
 	}
 	if (!failed)
 		fwts_passed(fw, "MCFG mmio config space is reserved in memory map table.");
@@ -215,10 +238,7 @@  static int mcfg_test1(fwts_framework *fw)
 		fwts_log_error(fw, "Cannot mmap table at 0x%" PRIx64 ".", config->base_address);
 		return FWTS_ERROR;
 	}
-
-	compare_config_space(fw, config->pci_segment_group_number,
-		0, (unsigned char *)mapped_table_page);
-
+	compare_config_space(fw, config->pci_segment_group_number, 0, mapped_table_page);
 	fwts_munmap(mapped_table_page, page_size);
 
 	return FWTS_OK;