| Message ID | 1323154087-14142-1-git-send-email-alex.hung@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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 > >
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 >> >> >
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 */
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