Re: [dpdk-dev] [PATCH v6] bus/pci: nvme on Windows requires class id and bus

From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Nick Connolly <nick.connolly@mayadata.io>
Cc: talshn@nvidia.com, pallavi.kadam@intel.com, thomas@monjalon.net,
	dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v6] bus/pci: nvme on Windows requires class id and bus
Date: Sun, 28 Feb 2021 17:38:22 +0300	[thread overview]
Message-ID: <20210228173822.69a09b0f@sovereign> (raw)
In-Reply-To: <20210223181857.2516-1-nick.connolly@mayadata.io>

2021-02-23 18:18, Nick Connolly:
> Attaching to an NVMe disk on Windows using SPDK requires the
> PCI class ID and device.bus fields. Decode the class ID from the PCI
> device info strings if it is present and set device.bus.
> 
> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
> Acked-by: Tal Shnaiderman <talshn@nvidia.com>
> ---
> v6:
> * no changes - resending to resolve spurious iol-testing failure
> 
> v5:
> * Add missing version history
> 
> v4:
> * Use #define to determine length of Class ID
> 
> v3:
> * Put version history at top - v2 mistakenly had it after the diffs
> 
> v2:
> * If only a 4-digit class ID is available, convert it to 6-digit format
> 
>  drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index f66258452..dceb0f4b2 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
>   * the registry hive for PCI devices.
>   */
>  
> +/* Class ID consists of hexadecimal digits */
> +#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
> +
>  /* The functions below are not implemented on Windows,
>   * but need to be defined for compilation purposes
>   */
> @@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
>  {
>  	int ids = 0;
>  	uint16_t vendor_id, device_id;
> -	uint32_t subvendor_id = 0;
> +	uint32_t subvendor_id = 0, class_id = 0;
> +	const char *cp;
>  
>  	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
>  		PRIx32, &vendor_id, &device_id, &subvendor_id);
>  	if (ids != 3)
>  		return -1;
>  
> +	/* Try and find PCI class ID */
> +	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> +		if (*cp == '&' && sscanf_s(cp,
> +				"&CC_%" PRIx32, &class_id) == 1) {
> +			/* Convert 4-digit class IDs to 6-digit format */
> +			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
> +				class_id <<= 8;
> +			break;
> +		}
> +

Is "4/6-digit format" used commonly for class ID, subclass ID, and optional
programming interface code? If not, I suggest sticking to official
terminology, something like "Assume zero programming interface code if
unspecified".

In general, a link to format reference would be useful in commit message or
function comment, for readers to understand what's being parsed:

https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices

With above nits,
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>