ACK: [PATCH] pci: aspm: Add segment support

[ivanhu]

On 2016年09月07日 03:42, Jeffrey Hugo wrote:
> PCI segement groups are the first level of relationship between PCI devices
> in a system.  PCI segments allow bus numbers to be reused as a particular
> bus ID must be unique only within the segment upon which it resides.
>
> Segment support is important as a system may have multiple root ports and
> attached devices across multiple segements, with different ASPM settings
> for each port/device set.  If the test is not aware of segments, it may
> incorrectly match a root port from segment A with a device from segment B
> and cause a test failure if that port and device happen to not have
> matching ASPM settings.
>
> Change-Id: I2aea72d12ca1dc945a4f73cf5cc652c6ffc62654
> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
> ---
>  src/pci/aspm/aspm.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index 9c3c143..72bee44 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -108,29 +108,29 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>
>  	if (((rp_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L0_FIELD) >> 10) !=
>  		(rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L0_FIELD)) {
> -		fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L0s not enabled.",
> -			 rp->bus, rp->dev, rp->func);
> +		fwts_warning(fw, "RP %04Xh:%02Xh:%02Xh.%02Xh L0s not enabled.",
> +			 rp->segment, rp->bus, rp->dev, rp->func);
>  		l0s_disabled = true;
>  	}
>
>  	if (((rp_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L1_FIELD) >> 10) !=
>  		(rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L1_FIELD)) {
> -		fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L1 not enabled.",
> -			rp->bus, rp->dev, rp->func);
> +		fwts_warning(fw, "RP %04Xh:%02Xh:%02Xh.%02Xh L1 not enabled.",
> +			rp->segment, rp->bus, rp->dev, rp->func);
>  		l1_disabled = true;
>  	}
>
>  	if (((device_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L0_FIELD) >> 10) !=
>  		(device_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L0_FIELD)) {
> -		fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L0s not enabled.",
> -			dev->bus, dev->dev, dev->func);
> +		fwts_warning(fw, "Device %04Xh:%02Xh:%02Xh.%02Xh L0s not enabled.",
> +			dev->segment, dev->bus, dev->dev, dev->func);
>  		l0s_disabled = true;
>  	}
>
>  	if (((device_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L1_FIELD) >> 10) !=
>  		(device_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L1_FIELD)) {
> -		fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L1 not enabled.",
> -			dev->bus, dev->dev, dev->func);
> +		fwts_warning(fw, "Device %04Xh:%02Xh:%02Xh.%02Xh L1 not enabled.",
> +			dev->segment, dev->bus, dev->dev, dev->func);
>  		l1_disabled = true;
>  	}
>
> @@ -157,10 +157,10 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>  	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);
> +		fwts_log_error(fw, "RP %04Xh:%02Xh:%02Xh.%02Xh has ASPM = %02Xh."
> +			, rp->segment, rp->bus, rp->dev, rp->func, rp_aspm_cntrl);
> +		fwts_log_error(fw, "Device %04Xh:%02Xh:%02Xh.%02Xh has ASPM = %02Xh.",
> +			dev->segment, dev->bus, dev->dev, dev->func, device_aspm_cntrl);
>  		fwts_advice(fw,
>  			"ASPM control registers between root port and device "
>  			"must match in order for ASPM to be active. Unmatched "
> @@ -189,11 +189,12 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
>  	}
>  	while ((entry = readdir(dirp)) != NULL) {
>  		uint8_t bus, dev, func;
> +		uint16_t segment;
>
>  		if (entry->d_name[0] == '.')
>  			continue;
>
> -		if (sscanf(entry->d_name, "%*x:%" SCNx8 ":%" SCNx8 ".%" SCNx8, &bus, &dev, &func) == 3) {
> +		if (sscanf(entry->d_name, "%" SCNx16 ":%" SCNx8 ":%" SCNx8 ".%" SCNx8, &segment, &bus, &dev, &func) == 4) {
>  			int fd;
>  			char path[PATH_MAX];
>  			struct pci_device *device;
> @@ -204,6 +205,7 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
>  				closedir(dirp);
>  				return FWTS_ERROR;
>  			}
> +			device->segment = segment;
>  			device->bus = bus;
>  			device->dev = dev;
>  			device->func = func;
> @@ -235,9 +237,11 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
>  		if (cur->config[FWTS_PCI_CONFIG_HEADER_TYPE] & 0x01) {
>  			for (ltarget = dev_list.head; ltarget; ltarget = ltarget->next) {
>  				struct pci_device *target = (struct pci_device *)ltarget->data;
> -				if (target->bus == cur->config[FWTS_PCI_CONFIG_TYPE1_SECONDARY_BUS_NUMBER]) {
> -					pcie_compare_rp_dev_aspm_registers(fw, cur, target);
> -					break;
> +				if (target->segment == cur->segment) {
> +					if (target->bus == cur->config[FWTS_PCI_CONFIG_TYPE1_SECONDARY_BUS_NUMBER]) {
> +						pcie_compare_rp_dev_aspm_registers(fw, cur, target);
> +						break;
> +					}
>  				}
>  			}
>  		}
>
Thanks Jeffrey,

Acked-by: Ivan Hu <ivan.hu at canonical.com>