[PATCH] pcie: add pcie aspm registers check on root port and device.
Alex Hung
alex.hung at canonical.com
Fri Feb 10 07:14:10 UTC 2012
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 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.
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
More information about the fwts-devel
mailing list