Patchwork pcie: add pcie aspm registers check on root port and device.

login
register
mail settings
Submitter Alex Hung
Date Feb. 8, 2012, 9:29 a.m.
Message ID <1328693362-31128-1-git-send-email-alex.hung@canonical.com>
Download mbox | patch
Permalink /patch/140082/
State Rejected
Headers show

Comments

Alex Hung - Feb. 8, 2012, 9:29 a.m.
Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/pci/aspm/aspm.c |  219 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 216 insertions(+), 3 deletions(-)
Colin King - Feb. 9, 2012, 11:31 a.m.
On 08/02/12 09:29, Alex Hung wrote:
> Signed-off-by: Alex Hung<alex.hung@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
Alex Hung - Feb. 10, 2012, 7:14 a.m.
On 02/09/2012 07:31 PM, Colin Ian King wrote:
> On 08/02/12 09:29, Alex Hung wrote:
>> Signed-off-by: Alex Hung<alex.hung@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.
Very good point, and I think I don't need to find the root device; 
instead, all I need is the first device output from lspci (just need a 
head of the for linked list). I will change the name of the variable and 
modify the code accordingly.
>
>> +
>> +        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[]
PCI configuration space will always be exact 256 bytes long (the 
addition pci extended space is exactly 4096 byte long, and it is from 
lspci -xxxx).
Assuming lspci -xxx will not be changed to display extended 
configuration space, this should be safe.
>
>> +                }
>> +            }
>> +        }
>> +        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
>
Thanks a lot, I will re-work the patch according to the feedback.

Best Regards,
Alex Hung
Colin King - Feb. 10, 2012, 8:47 a.m.
On 10/02/12 07:14, Alex Hung wrote:
> On 02/09/2012 07:31 PM, Colin Ian King wrote:
>
>>> +
>>> + 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[]
> PCI configuration space will always be exact 256 bytes long (the
> addition pci extended space is exactly 4096 byte long, and it is from
> lspci -xxxx).
> Assuming lspci -xxx will not be changed to display extended
> configuration space, this should be safe.

How about "assume nothing" - stuff breaks and I like to avoid segfaults, 
so how about something paranoid like:

	for (i = 0; reg_loc < 256 && i < 16; i++)
		cur->config[reg_loc++] = strtol(pEnd + 1,&pEnd, 16);


Colin

Patch

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;
+};
+
 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];
+	rp_cap = (struct pcie_capability *) &rp->config[next_cap];
+	while (rp_cap->pcie_cap_id != 0x10) {
+		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);
+	}
+
+
+	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;
+	}
+
+	/* 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));
+		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;
+
+		cur = device;
+	}
+
+	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;
+		}
+
+		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++;
+				}
+			}
+		}
+		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) {
+
+			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;
+}
+
 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 }
 };