[PATCH] pcie: add pcie aspm registers check on root port and device.
Colin Ian King
colin.king at canonical.com
Thu Feb 9 11:31:01 UTC 2012
On 08/02/12 09:29, Alex Hung wrote:
> Signed-off-by: Alex Hung<alex.hung at canonical.com>
> ---
> src/pci/aspm/aspm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 216 insertions(+), 3 deletions(-)
>
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index db82718..9ed7fc0 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -26,10 +26,45 @@
> #include<string.h>
> #include<fcntl.h>
> #include<limits.h>
> -#include<linux/pci.h>
> -
> #include "fwts.h"
>
> +struct pci_device {
> + uint8_t segment;
> + uint8_t bus;
> + uint8_t dev;
> + uint8_t func;
> + uint8_t config[256];
> + struct pci_device *next;
> +};
> +
> +
> +struct pcie_capability {
> + uint8_t pcie_cap_id;
> + uint8_t next_cap_point;
> + uint16_t pcie_cap_reg;
> + uint32_t device_cap;
> + uint16_t device_contrl;
> + uint16_t device_status;
> + uint32_t link_cap;
> + uint16_t link_contrl;
> + uint16_t link_status;
> + uint32_t slot_cap;
> + uint16_t slot_contrl;
> + uint16_t slot_status;
> + uint16_t root_contrl;
> + uint16_t root_cap;
> + uint32_t root_status;
> + uint32_t device_cap2;
> + uint16_t device_contrl2;
> + uint16_t device_status2;
> + uint32_t link_cap2;
> + uint16_t link_contrl2;
> + uint16_t link_status2;
> + uint32_t slot_cap2;
> + uint16_t slot_contrl2;
> + uint16_t slot_status2;
> +};
> +
Can you put a comment in referencing which specification I can find a
description of these data structures?
> static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
> {
> fwts_acpi_table_info *table;
> @@ -51,6 +86,174 @@ static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
> return FWTS_OK;
> }
>
> +int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
> + struct pci_device *rp,
> + struct pci_device *dev)
> +{
> + struct pcie_capability *rp_cap, *device_cap;
> + uint8_t rp_aspm_support, rp_aspm_cntrl;
> + uint8_t device_aspm_support, device_aspm_cntrl;
> + uint8_t next_cap;
> + int ret = FWTS_OK;
> +
> + next_cap = rp->config[0x34];
#define of the 0x34 would be useful as I'm lazy and don't want to look
it up in the PCI spec :-)
> + rp_cap = (struct pcie_capability *)&rp->config[next_cap];
> + while (rp_cap->pcie_cap_id != 0x10) {
same here for 0x10 - what is that?
> + if (rp_cap->next_cap_point == 0x00)
> + break;
> + next_cap = rp_cap->next_cap_point;
> + rp_cap = (struct pcie_capability *)&rp->config[next_cap];
> + }
> +
> + next_cap = dev->config[0x34];
> + device_cap = (struct pcie_capability *) &dev->config[next_cap];
> + while (device_cap->pcie_cap_id != 0x10) {
> + if (device_cap->next_cap_point == 0x00)
> + break;
> + next_cap = device_cap->next_cap_point;
> + device_cap = (struct pcie_capability *) &dev->config[next_cap];
> + }
> +
> + rp_aspm_support = (rp_cap->link_cap& 0x0c00)>> 10;
> + rp_aspm_cntrl = (rp_cap->link_contrl& 0x03);
> + device_aspm_support = (device_cap->link_cap& 0x0c00)>> 10;
> + device_aspm_cntrl = (device_cap->link_contrl& 0x03);
> +
> + if ((rp_aspm_support& 0x01) != (rp_aspm_cntrl& 0x01)) {
> + fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L0s not enabled.",
> + rp->bus, rp->dev, rp->func);
> + }
> +
> + if ((rp_aspm_support& 0x02) != (rp_aspm_cntrl& 0x02)) {
> + fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L1 not enabled.",
> + rp->bus, rp->dev, rp->func);
> + }
> +
bunch more #defines on the magic values would be handy too..
> +
> + if ((device_aspm_support& 0x01) != (device_aspm_cntrl& 0x01)) {
> + fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L0s not enabled.",
> + dev->bus, dev->dev, dev->func);
> + }
> +
> + if ((device_aspm_support& 0x02) != (device_aspm_cntrl& 0x02)) {
> + fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L1 not enabled.",
> + dev->bus, dev->dev, dev->func);
> + }
> +
> + if (rp_aspm_cntrl != device_aspm_cntrl) {
> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "PCIEASPM_UNMATCHED",
> + "PCIE aspm setting was not matched.");
> + fwts_log_error(fw, "RP %02Xh:%02Xh.%02Xh has aspm = %02Xh."
> + , rp->bus, rp->dev, rp->func, rp_aspm_cntrl);
> + fwts_log_error(fw, "Device %02Xh:%02Xh.%02Xh has aspm = %02Xh.",
> + dev->bus, dev->dev, dev->func, device_aspm_cntrl);
> + } else {
> + fwts_passed(fw, "PCIE aspm setting matched was matched.");
> + }
> +
> + return ret;
> +}
> +
> +int pcie_check_aspm_registers(fwts_framework *fw,
> + const fwts_log_level level)
> +{
> + fwts_list *lspci_output;
> + fwts_list_link *item;
> + struct pci_device *root = NULL, *cur = NULL, *device = NULL;
> + char command[PATH_MAX];
> + int ret = FWTS_OK;
> +
> + snprintf(command, sizeof(command), "%s", fw->lspci);
> +
> + if (fwts_pipe_exec(command,&lspci_output) == FWTS_EXEC_ERROR) {
> + fwts_log_warning(fw, "Could not execute %s", command);
> + return FWTS_EXEC_ERROR;
> + }
> +
fwts_pipe_exec may return a NULL list (e.g. out of of memory), so we
should check for that. It's a poor API :-(
if (lspci_output == NULL)
return FWTS_ERROR;
> + /* Get the list of pci devices and their configuration */
> + fwts_list_foreach(item, lspci_output) {
> + char *line = fwts_text_list_text(item);
> + char *pEnd;
> +
> + device = (struct pci_device *) malloc(sizeof(*device));
I try to avoid malloc in fwts and use calloc instead since it also
ensures the memory is zero'd.
> + if (device == NULL)
> + return FWTS_ERROR;
> +
> + device->bus = strtol(line,&pEnd, 16);
> + device->dev = strtol(pEnd + 1,&pEnd, 16);
> + device->func = strtol(pEnd + 1,&pEnd, 16);
> +
> + if (device->bus == 0&& device->dev == 0&& device->func == 0)
> + root = device;
> + else
> + cur->next = device;
I suspect we are assuming that lspci will always output in device order,
with the root at the first item. My fear is that if this does not happen
the cur->next will segfault if cur is NULL. Perhaps we should
check for that to bullet-proof the code.
> +
> + cur = device;
> + }
> +
I'd prefer:
for (cur = root; cur; cur = cur->next)
instead of:
cur = root;
while (cur != NULL) {
...
...
cur = cur->next;
}
..several times in the code below.
> + cur = root;
> + while (cur != NULL) {
> + int reg_loc = 0, reg_val = 0;
> + int i;
> +
> + snprintf(command, sizeof(command), "%s -s %02X:%02X.%02X -xxx",
> + fw->lspci, cur->bus, cur->dev, cur->func);
> + if (fwts_pipe_exec(command,&lspci_output) == FWTS_EXEC_ERROR) {
> + fwts_log_warning(fw, "Could not execute %s", command);
> + return FWTS_EXEC_ERROR;
Should be return FWTS_ERROR so that the test framework can handle this
appropriately (FWTS_ERROR, FWTS_SKIP, FWTS_OK are the only return codes
that should be returned up to the fwts framework).
> + }
Again, null list check required:
if (lspci_output == NULL)
return FWTS_ERROR;
OK, so we're parsing something like:
00:1b.0 Audio device: Intel Corporation 82801H (ICH8 Family) HD Audio
Controller (rev 03)
00: 86 80 4b 28 06 05 10 00 03 00 03 04 10 00 00 00
10: 04 00 30 fc 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 aa 17 4e 38
30: 00 00 00 00 50 00 00 00 00 00 00 00 0b 01 00 00
> + fwts_list_foreach(item, lspci_output) {
> + char *line = fwts_text_list_text(item);
> + char *pEnd;
> +
> + if (line[3] == ' ') {
> + reg_val = strtol(line,&pEnd, 16);
> + for (i = 0; i< 16; i++) {
> + reg_val = strtol(pEnd + 1,&pEnd, 16);
> + cur->config[reg_loc] = reg_val;
> + reg_loc++;
Hopefully we won't have > 256 bytes in PCI config space. Maybe we should
guard against thsi overflowing cur->config[]
> + }
> + }
> + }
> + cur = cur->next;
> + }
> +
> + /* Check aspm registers from the list of pci devices */
> + cur = root;
> + while (cur != NULL) {
> + struct pci_device *target;
> +
> + if (cur->config[0x0E]& 0x01) {
These magic numbers mean nothing to me. Any chance of #defining them?
> +
Again, I'd prefer:
for (target = root; target; target = target->next)
> + target = root;
> + while (target != NULL) {
> + if (target->bus == cur->config[0x19])
> + break;
> + target = target->next;
> + }
> + if (target == NULL) {
> + cur = cur->next;
> + continue;
> + }
> +
> + pcie_compare_rp_dev_aspm_registers(fw, cur, target);
> + }
> + cur = cur->next;
> + }
> +
> + cur = root;
> + while (cur != NULL) {
> + device = cur->next;
> + free(cur);
> + cur = device;
> + }
> +
> + fwts_text_list_free(lspci_output);
> +
> + return ret;
> +}
> +
> static int aspm_check_configuration(fwts_framework *fw)
> {
> int ret;
> @@ -65,8 +268,18 @@ static int aspm_check_configuration(fwts_framework *fw)
> return ret;
> }
>
> +static int aspm_pcie_register_configuration(fwts_framework *fw)
> +{
> + int ret;
> +
> + ret = pcie_check_aspm_registers(fw, LOG_LEVEL_HIGH);
> +
> + return ret;
> +}
how about:
{
return pcie_check_aspm_registers(fw, LOG_LEVEL_HIGH);
}
> +
> static fwts_framework_minor_test aspm_tests[] = {
> - { aspm_check_configuration, "PCIe ASPM configuration test." },
> + { aspm_check_configuration, "PCIe ASPM ACPI test." },
> + { aspm_pcie_register_configuration, "PCIe ASPM registers test." },
> { NULL, NULL }
> };
>
Conceptually this is great work, just needs a little more re-working.
Thanks for the contribution Alex!
Colin
More information about the fwts-devel
mailing list