[PATCH 3/4] acpi: mcfg: more code tidying.
Keng-Yu Lin
kengyu at canonical.com
Thu Oct 25 07:58:47 UTC 2012
On Sun, Oct 21, 2012 at 3:54 AM, Colin King <colin.king at canonical.com> wrote:
> From: Colin Ian King <colin.king at 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 at 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 at canonical.com>
More information about the fwts-devel
mailing list