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

[Tal Shnaiderman <talshn@mellanox.com>]

> Subject: Re: [dpdk-dev] [PATCH 7/7] bus/pci: support Windows with
> bifurcated drivers
> 
> On Wed, Apr 22, 2020 at 10:27:47AM +0300, talshn@mellanox.com wrote:
> > From: Tal Shnaiderman <talshn@mellanox.com>
> >
> > Uses SetupAPI.h functions to scan PCI tree.
> > Uses DEVPKEY_Device_Numa_Node to get the PCI Numa node.
> > scanning currently supports types RTE_KDRV_NONE.
> >
> > Signed-off-by: Tal Shnaiderman <talshn@mellanox.com>
> > ---
> >  drivers/bus/pci/windows/pci.c                | 342
> ++++++++++++++++++++++++++-
> >  lib/librte_eal/rte_eal_exports.def           |   1 +
> >  lib/librte_eal/windows/include/rte_windows.h |   1 +
> >  3 files changed, 342 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/pci/windows/pci.c
> > b/drivers/bus/pci/windows/pci.c index 7eff39173..d5ee938fa 100644
> > --- a/drivers/bus/pci/windows/pci.c
> > +++ b/drivers/bus/pci/windows/pci.c
> > @@ -1,14 +1,24 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> >   * Copyright 2020 Mellanox Technologies, Ltd
> >   */
> > +
> > +static
> > +int get_device_resource_info(HDEVINFO hDevInfo,
> > +	PSP_DEVINFO_DATA pDeviceInfoData , struct rte_pci_device *dev) {
> > +	int  ret = -1;
> > +	DEVPROPTYPE uPropertyType;
> > +	DWORD uNumaNode;
> > +	BOOL  bResult;
> > +
> > +	switch (dev->kdrv) {
> > +	case RTE_KDRV_NONE:
> > +		/* Get NUMA node using DEVPKEY_Device_Numa_Node */
> > +		bResult = SetupDiGetDevicePropertyW(hDevInfo,
> pDeviceInfoData,
> > +			&DEVPKEY_Device_Numa_Node, &uPropertyType,
> > +			(BYTE *)&uNumaNode, sizeof(uNumaNode), NULL,
> 0);
> > +		if (!bResult) {
> > +			ret = GetLastError();
> > +			break;
> 
> Here 'ret' is correctly set to an error code, but after breaking out of the
> switch, it is overwritten to ERROR_SUCCESS.
> Maybe 'goto end' instead of 'break'.
> 
> > +		}
> > +		dev->device.numa_node = uNumaNode;
> > +		/* mem_resource - Uneeded 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:
> > +		ret = -1;
> 
> Same thing here, ret is overwritten after breaking from the switch.

Agreed, I'll fix both in v2, thank you.

> 
> > +		break;
> > +	}
> > +
> > +	ret = ERROR_SUCCESS;
> > +end:
> > +	return ret;
> > +}
> > +
> > +
> > +/*