Re: [dpdk-dev] [PATCH v5 8/8] bus/pci: support Windows with bifurcated drivers

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

[snip]
> +#ifdef RTE_TOOLCHAIN_GCC
> +#include <Devpropdef.h>

This breaks cross-compilation because devpropdef.h is all-lowercase on Linux.

> +DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
> +	0xa8, 0xa2, 0x6a, 0x0b, 0x89, 0x4c, 0xbd, 0xa2, 3);
> +#endif


[snip]
> +static int
> +get_device_resource_info(HDEVINFO dev_info,
> +	PSP_DEVINFO_DATA dev_info_data, struct rte_pci_device *dev)
> +{
> +	DEVPROPTYPE property_type;
> +	DWORD numa_node;
> +	BOOL  res;
> +
> +	switch (dev->kdrv) {
> +	case RTE_KDRV_NONE:
> +		/* Get NUMA node using DEVPKEY_Device_Numa_Node */
> +		res = SetupDiGetDevicePropertyW(dev_info, dev_info_data,
> +			&DEVPKEY_Device_Numa_Node, &property_type,
> +			(BYTE *)&numa_node, sizeof(numa_node), NULL, 0);
> +		if (!res) {
> +			RTE_LOG_WIN32_ERR(
> +				"SetupDiGetDevicePropertyW"
> +				"(DEVPKEY_Device_Numa_Node)");
> +			return -1;
> +		}
> +		dev->device.numa_node = numa_node;
> +		/* mem_resource - Unneeded for RTE_KDRV_NONE */
> +		dev->mem_resource[0].phys_addr = 0;
> +		dev->mem_resource[0].len = 0;
> +		dev->mem_resource[0].addr = NULL;
> +		break;
> +	default:
> +		return ERROR_NOT_SUPPORTED;
> +	}
> +
> +	return 0;
> +}

Why return (-1) in one case and ERROR_NOT_SUPPORTED in another if these cases
are not distinguished by the caller? Also, it returns 0 on success, but
caller checks for ERROR_SUCCESS (which is 0, but this is inconsistent).

[snip]
> @@ -165,5 +360,40 @@ pci_uio_remap_resource(struct rte_pci_device *dev __rte_unused)
>  int
>  rte_pci_scan(void)
>  {
> +	DWORD			device_index = 0, found_device = 0;
> +	HDEVINFO		dev_info;
> +	SP_DEVINFO_DATA	device_info_data;
> +	int				ret = -1;

Sorry for nitpicking, but such alignment hurts readability, with 8 spaces
per tab, at least. Please consider using spaces in the middle of line.

> +
> +	dev_info = SetupDiGetClassDevs(&GUID_DEVCLASS_NET, TEXT("PCI"), NULL,
> +				DIGCF_PRESENT);
> +	if (dev_info == INVALID_HANDLE_VALUE) {
> +		RTE_LOG(ERR, EAL, "Unable to enumerate PCI devices.\n");
> +		RTE_LOG_WIN32_ERR("SetupDiGetClassDevs(pci_scan)");

RTE_LOG() does output, which may rewrite GetLastError(), consider swapping
these two lines.

> +		goto end;
> +	}
> +
[snip]

-- 
Dmitry Kozlyuk