Patchwork pcie: aspm: add aspm option and detect if the "PCIe ASPM Controls" bit is set in FADT.

login
register
mail settings
Submitter Alex Hung
Date Dec. 6, 2011, 6:48 a.m.
Message ID <1323154087-14142-1-git-send-email-alex.hung@canonical.com>
Download mbox | patch
Permalink /patch/129571/
State Accepted
Headers show

Comments

Alex Hung - Dec. 6, 2011, 6:48 a.m.
Signed-off-by: Alex Hung <alex.hung@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
Colin King - Dec. 6, 2011, 9:45 a.m.
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@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
Colin King - Dec. 6, 2011, 10:04 a.m.
On 06/12/11 06:48, Alex Hung wrote:
> Signed-off-by: Alex Hung<alex.hung@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
> + *
> + * 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
> +
> +#define BIT0	0x01
> +#define BIT1	0x02
> +#define BIT2    0x04
> +#define BIT3    0x08
> +#define BIT4    0x10
> +#define BIT5    0x20
> +#define BIT6    0x40
> +#define BIT7    0x80
> +
> +
> +
> +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
> + *
> + * 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);
> +
> +	iapc_boot_arch_low_byte = facp[FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET];
> +	if ((iapc_boot_arch_low_byte&  BIT4) == 0)
> +	{
> +		*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;
> +	}
> +
> +	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 */
Come to think of it a little deeper,  I believe we need to add some more 
logic for the up and coming Precise kernel as I believe this also 
consults  _OSC as well as the boot arch flags before it enables PCIe 
ASPM.  I just got Matthew Garrett's PCIe ASPM patch ACK'd and included 
into the next Precise kernel, so I suspect you need to consult this 
patch too to double check exactly what the kernel is doing.  The patch 
in question is: http://lwn.net/Articles/467654/

..it's just some more food for thought.

Colin
Alex Hung - Dec. 6, 2011, 10:45 a.m.
Hi Colin,

Thanks for the quick feedback. I haven't finished reading the patch but 
just provide quick information.

Two thoughts pop up in my mind when I decide to add aspm option.

1. Smaller changes:
  * I intend to add incremental changes, and I think a simple check on 
FADT is a good starting point - BIOS tells kernel which Linux should 
control ASPM.
  * Next thing is the sanity check on PCI(E) hardware registers. This is 
the actual ASPM setting, whether it is set by BIOS or by kernel.
  * Next thing is _OSC evaluation (more below)

2. _OSC evaluation:
I thought about _OSC when I was adding the feature, and I want to 
limited the --aspm to BIOS-to-OS which includes two things - FADT and 
the PCI(E) registers, at least to now.

According to Section 4.5.1 in PCI Firmware Specification Rev 3.0, _OSC 
is an OS-to-BIOS call
==============================================
On input argument:

Active State Power Management supported
The operating system sets this bit to 1 if it natively supports 
configuration of Active
State Power Management registers in PCI Express devices. Otherwise, the
operating system sets this bit to 0

On return values, there is nothing associated to ASPM, which means that 
BIOS does not tell OSPM whether OS should have the control (reasonable, 
since FADT reports it).
==============================================
As it is not defined very clearly, I think OSPM (i.e. Linux, Windows, 
etc...) sets this bit and tells BIOS that it controls ASPM, and I think 
the action is based on FADT table as there is no other places that tells 
whether OSPM should control ASPM.

Summary of above:
  * I intend to implement the checks on FADT and PCI(E) registers first.
  * We can discuss the details of _OSC implementation.

What do you think?

P.S. Do you need the PCI firmware document?

Best Regards,
Alex Hung

On 12/06/2011 06:04 PM, Colin Ian King wrote:
> On 06/12/11 06:48, Alex Hung wrote:
>> Signed-off-by: Alex Hung<alex.hung@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
>> + *
>> + * 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
>> +
>> +#define BIT0    0x01
>> +#define BIT1    0x02
>> +#define BIT2    0x04
>> +#define BIT3    0x08
>> +#define BIT4    0x10
>> +#define BIT5    0x20
>> +#define BIT6    0x40
>> +#define BIT7    0x80
>> +
>> +
>> +
>> +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
>> + *
>> + * 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);
>> +
>> +    iapc_boot_arch_low_byte = 
>> facp[FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET];
>> +    if ((iapc_boot_arch_low_byte&  BIT4) == 0)
>> +    {
>> +        *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;
>> +    }
>> +
>> +    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 */
> Come to think of it a little deeper,  I believe we need to add some 
> more logic for the up and coming Precise kernel as I believe this also 
> consults  _OSC as well as the boot arch flags before it enables PCIe 
> ASPM.  I just got Matthew Garrett's PCIe ASPM patch ACK'd and included 
> into the next Precise kernel, so I suspect you need to consult this 
> patch too to double check exactly what the kernel is doing.  The patch 
> in question is: http://lwn.net/Articles/467654/
>
> ..it's just some more food for thought.
>
> Colin
>
>
Colin King - Dec. 6, 2011, 10:52 a.m.
On 06/12/11 10:45, Alex Hung wrote:
> Hi Colin,
>
> Thanks for the quick feedback. I haven't finished reading the patch 
> but just provide quick information.
>
> Two thoughts pop up in my mind when I decide to add aspm option.
>
> 1. Smaller changes:
>  * I intend to add incremental changes, and I think a simple check on 
> FADT is a good starting point - BIOS tells kernel which Linux should 
> control ASPM.
>  * Next thing is the sanity check on PCI(E) hardware registers. This 
> is the actual ASPM setting, whether it is set by BIOS or by kernel.
>  * Next thing is _OSC evaluation (more below)
OK - that makes sense.
>
> 2. _OSC evaluation:
> I thought about _OSC when I was adding the feature, and I want to 
> limited the --aspm to BIOS-to-OS which includes two things - FADT and 
> the PCI(E) registers, at least to now.
>
> According to Section 4.5.1 in PCI Firmware Specification Rev 3.0, _OSC 
> is an OS-to-BIOS call
> ==============================================
> On input argument:
>
> Active State Power Management supported
> The operating system sets this bit to 1 if it natively supports 
> configuration of Active
> State Power Management registers in PCI Express devices. Otherwise, the
> operating system sets this bit to 0
>
> On return values, there is nothing associated to ASPM, which means 
> that BIOS does not tell OSPM whether OS should have the control 
> (reasonable, since FADT reports it).
> ==============================================
> As it is not defined very clearly, I think OSPM (i.e. Linux, Windows, 
> etc...) sets this bit and tells BIOS that it controls ASPM, and I 
> think the action is based on FADT table as there is no other places 
> that tells whether OSPM should control ASPM.
>
> Summary of above:
>  * I intend to implement the checks on FADT and PCI(E) registers first.
Good plan. Let's run with that.
>
>  * We can discuss the details of _OSC implementation.
Yep, if you have a look at what the kernel is currently doing and also 
what linux-next is doing (as this has Matthew Garrett's ASPM fix) and 
then we can discuss this further.
>
> What do you think?
>
> P.S. Do you need the PCI firmware document?
Yes please do.

>
> Best Regards,
> Alex Hung
>
> On 12/06/2011 06:04 PM, Colin Ian King wrote:
>> On 06/12/11 06:48, Alex Hung wrote:
>>> Signed-off-by: Alex Hung<alex.hung@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
>>> + *
>>> + * 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
>>> +
>>> +#define BIT0    0x01
>>> +#define BIT1    0x02
>>> +#define BIT2    0x04
>>> +#define BIT3    0x08
>>> +#define BIT4    0x10
>>> +#define BIT5    0x20
>>> +#define BIT6    0x40
>>> +#define BIT7    0x80
>>> +
>>> +
>>> +
>>> +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
>>> + *
>>> + * 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);
>>> +
>>> +    iapc_boot_arch_low_byte = 
>>> facp[FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET];
>>> +    if ((iapc_boot_arch_low_byte&  BIT4) == 0)
>>> +    {
>>> +        *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;
>>> +    }
>>> +
>>> +    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 */
>> Come to think of it a little deeper,  I believe we need to add some 
>> more logic for the up and coming Precise kernel as I believe this 
>> also consults  _OSC as well as the boot arch flags before it enables 
>> PCIe ASPM.  I just got Matthew Garrett's PCIe ASPM patch ACK'd and 
>> included into the next Precise kernel, so I suspect you need to 
>> consult this patch too to double check exactly what the kernel is 
>> doing.  The patch in question is: http://lwn.net/Articles/467654/
>>
>> ..it's just some more food for thought.
>>
>> Colin
>>
>>
>

Patch

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
+ *
+ * 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
+
+#define BIT0	0x01
+#define BIT1	0x02
+#define BIT2    0x04
+#define BIT3    0x08
+#define BIT4    0x10
+#define BIT5    0x20
+#define BIT6    0x40
+#define BIT7    0x80
+
+
+
+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
+ *
+ * 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);
+
+	iapc_boot_arch_low_byte = facp[FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET];
+	if ((iapc_boot_arch_low_byte & BIT4) == 0)
+	{
+		*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;
+	}
+
+	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 */