[PATCH] pcie: aspm: add aspm option and detect if the "PCIe ASPM Controls" bit is set in FADT.
Colin Ian King
colin.king at canonical.com
Tue Dec 6 09:45:23 UTC 2011
Thanks Alex for your contribution, great idea, just needs a little bit
of re-working before I ACK it.
On 06/12/11 06:48, Alex Hung wrote:
> Signed-off-by: Alex Hung<alex.hung at canonical.com>
> ---
> src/lib/include/fwts.h | 1 +
> src/lib/include/fwts_aspm.h | 42 +++++++++++++++++++++
> src/lib/src/Makefile.am | 3 +-
> src/lib/src/fwts_aspm.c | 84 ++++++++++++++++++++++++++++++++++++++++++
> src/lib/src/fwts_framework.c | 4 ++
> 5 files changed, 133 insertions(+), 1 deletions(-)
> create mode 100644 src/lib/include/fwts_aspm.h
> create mode 100644 src/lib/src/fwts_aspm.c
>
> diff --git a/src/lib/include/fwts.h b/src/lib/include/fwts.h
> index f5602ea..abea55a 100644
> --- a/src/lib/include/fwts.h
> +++ b/src/lib/include/fwts.h
> @@ -75,5 +75,6 @@
> #include "fwts_ac_adapter.h"
> #include "fwts_battery.h"
> #include "fwts_button.h"
> +#include "fwts_aspm.h"
>
> #endif
> diff --git a/src/lib/include/fwts_aspm.h b/src/lib/include/fwts_aspm.h
> new file mode 100644
> index 0000000..9695fe9
> --- /dev/null
> +++ b/src/lib/include/fwts_aspm.h
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical
^ I'd make this 2011 rather than 2010-2011 as this pertains to this
specific source.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +
> +#ifndef __FWTS_ASPM_H__
> +#define __FWTS_ASPM_H__
> +
> +#define FWTS_SYS_FIRMWARE_ACPI_TABLE_FACP "/sys/firmware/acpi/tables/FACP"
> +
> +#define FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET 109
> +#define FACP_IAPC_BOOT_ARCH_HIGH_BYTE_OFFSET 110
> +#define FACP_TABLE_MAX 1024
Generally speaking, any constants that get pulled into the fwts lib
headers should be
prefixed by FWTS_
With the use of the fwts table handling functions mentioned later on,
you won't need any of these
#define constants and no need for creating this include file.
> +
> +#define BIT0 0x01
> +#define BIT1 0x02
> +#define BIT2 0x04
> +#define BIT3 0x08
> +#define BIT4 0x10
> +#define BIT5 0x20
> +#define BIT6 0x40
> +#define BIT7 0x80
See my notes below about the FWTS_FADT_BOOT_ARCH_* #defines
> +
> +
> +
> +int fwts_aspm_check_configuration(fwts_framework *fw);
> +
> +#endif
> diff --git a/src/lib/src/Makefile.am b/src/lib/src/Makefile.am
> index 2aac13e..d76d4cd 100644
> --- a/src/lib/src/Makefile.am
> +++ b/src/lib/src/Makefile.am
> @@ -56,4 +56,5 @@ libfwts_la_SOURCES = \
> fwts_wakealarm.c \
> fwts_ac_adapter.c \
> fwts_battery.c \
> - fwts_button.c
> + fwts_button.c \
> + fwts_aspm.c
> diff --git a/src/lib/src/fwts_aspm.c b/src/lib/src/fwts_aspm.c
> new file mode 100644
> index 0000000..397dfc8
> --- /dev/null
> +++ b/src/lib/src/fwts_aspm.c
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical
^ likewise, (C) 2011 Canonical
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +#include<stdlib.h>
> +#include<stdio.h>
> +#include<errno.h>
> +#include<sys/types.h>
> +#include<sys/stat.h>
> +#include<sys/wait.h>
> +#include<unistd.h>
> +#include<string.h>
> +#include<fcntl.h>
> +#include<limits.h>
> +#include<linux/pci.h>
> +
> +#include "fwts.h"
> +
> +int fwts_facp_get_aspm_control(fwts_framework *fw, int *aspm)
> +{
> + FILE *fp;
> + char path[PATH_MAX], facp[FACP_TABLE_MAX];
> + char c;
> + int i = 0;
> + uint16_t iapc_boot_arch_low_byte;
> +
> + snprintf(path, sizeof(path), "%s", FWTS_SYS_FIRMWARE_ACPI_TABLE_FACP);
> + if ((fp = fopen(path, "rb")) == NULL) {
> + fwts_log_info(fw, "FACP is not present in %s.", path);
> + return FWTS_ERROR;
> + }
> +
> + c = fgetc(fp);
> + do {
> + facp[i] = c;
> + c = fgetc(fp);
> + i++;
> + } while(c != EOF);
> + fclose(fp);
> +
Instead of manually fetching the table and parsing it, one can use
something like:
fwts_acpi_table_info *table;
fwts_acpi_table_fadt *fadt;
if (fwts_acpi_find_table(fw, "FACP", 0, &table) != FWTS_OK) {
/* handle error*/
return FWTS_ERROR;
}
fadt = (fwts_acpi_table_fadt*)table->data;
and the field you want is in fadt->iapc_boot_arch
the fwts support libraries do all the magic of fetching the tables and
freeing the memory for you, so it is relatively
painless.
> + iapc_boot_arch_low_byte = facp[FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET];
> + if ((iapc_boot_arch_low_byte& BIT4) == 0)
Instead of using BIT4, you should probably be using names that make
sense from section 5.2.9.3 IA-PC Boot Architecture Flags, e.g.
#define FWTS_FADT_BOOT_ARCH_LEGACY_DEVICES (0x0001)
#define FWTS_FADT_BOOT_ARCH_8042 (0x0002)
#define FWTS_FADT_BOOT_ARCH_VGA_NOT_PRESENT (0x0004)
#define FWTS_FADT_BOOT_ARCH_MSI_NOT_SUPPORTED (0x0008)
#define FWTS_FADT_BOOT_ARCH_PCI_ASPM_CONTROLS (0x0010)
..and put these into src/lib/include/fwts_acpi.h somewhere near the
start near the FWTS_ACPI_TABLES_PATH #define.
Never entirely sure if we should name this table FADT or FACP really...
> + {
> + *aspm = 1;
> + fwts_log_info(fw, "PCIE ASPM is controlled by Linux kernel.");
> + } else
> + {
> + *aspm = 0;
> + fwts_log_info(fw, "PCIE ASPM is not controlled by Linux kernel.");
> + }
> +
> + return FWTS_OK;
> +}
> +
> +int fwts_aspm_check_configuration(fwts_framework *fw)
> +{
> + int ret;
> + int aspm_facp;
> +
> + ret = fwts_facp_get_aspm_control(fw,&aspm_facp);
> + if (ret == FWTS_ERROR)
> + {
> + fwts_log_info(fw, "No valid FACP information present: cannot test aspm.");
> + return FWTS_ERROR;
> + }
I'd prefer it if we use the kernel style of braces, e.g.
if (ret == FWTS_ERROR) {
...
}
..one can get more lines on the screen like that and it's part of the
fwts style. I've written up some guidelines about fwts source style in
README_SOURCE.txt in the fwts root directory.
> +
> + return ret;
> +
> +
> +
> diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c
> index 9898537..7064c44 100644
> --- a/src/lib/src/fwts_framework.c
> +++ b/src/lib/src/fwts_framework.c
> @@ -76,6 +76,7 @@ static fwts_option fwts_framework_options[] = {
> { "json-data-path", "j:", 1, "Specify path to fwts json data files - default is /usr/share/fwts." },
> { "lp-tags-log", "", 0, "Output LaunchPad bug tags in results log." },
> { "disassemble-aml", "", 0, "Disassemble AML from DSDT and SSDT tables." },
> + { "aspm", "", 0, "Test ASPM configuration." },
> { NULL, NULL, 0, NULL }
> };
>
> @@ -967,6 +968,9 @@ int fwts_framework_options_handler(fwts_framework *fw, int argc, char * const ar
> case 31: /* --disassemble-aml */
> fwts_iasl_disassemble_all_to_file(fw);
> return FWTS_COMPLETE;
> + case 32: /* --aspm */
> + fwts_aspm_check_configuration(fw);
> + return FWTS_COMPLETE;
> }
> break;
> case 'a': /* --all */
This arg handling is good. As it is a new option it also needs to be
put into the man page, in doc/fwts.1
Again, you patch is great, but I'd appreciate it if you re-work it a
little to match the points mentioned above.
Thanks!
Colin
More information about the fwts-devel
mailing list