| Message ID | 1328693362-31128-1-git-send-email-alex.hung@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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 } };
Signed-off-by: Alex Hung <alex.hung@canonical.com> --- src/pci/aspm/aspm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 216 insertions(+), 3 deletions(-)