bus/pci: nvme on Windows requires class id and bus

  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>
---
 drivers/bus/pci/windows/pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
  

> Subject: [PATCH] bus/pci: nvme on Windows requires class id and bus
> 
> External email: Use caution opening links or attachments
> 
> 
> 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>
> ---
>  drivers/bus/pci/windows/pci.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index f66258452..3d11444c2 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -280,17 +280,24 @@ 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++)

How about 
for (cp = buf; cp[0] || cp[1]; cp++)

> +               if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32, &class_id) == 1)

Could there be a case where PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2) exist but PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2)p(2) doesn't? In that case the parsing would be incorrect.

> +                       break;
> +
>         pci_id->vendor_id = vendor_id;
>         pci_id->device_id = device_id;
>         pci_id->subsystem_device_id = subvendor_id >> 16;
>         pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
> +       pci_id->class_id = class_id;
>         return 0;
>  }
> 
> @@ -339,6 +346,7 @@ pci_scan_one(HDEVINFO dev_info,
> PSP_DEVINFO_DATA device_info_data)
>         if (ret != 0)
>                 goto end;
> 
> +       dev->device.bus = &rte_pci_bus.bus;
>         dev->addr = addr;
>         dev->id = pci_id;
>         dev->max_vfs = 0; /* TODO: get max_vfs */
> --
> 2.25.1
  

Hi Tal,

Thanks for the comments.

>> +       /* Try and find PCI class ID */
>> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> How about
> for (cp = buf; cp[0] || cp[1]; cp++)
That would be my preferred idiom, but the DPDK coding style (1.9.1) says 
'do not use ! for tests unless it is a boolean' (but somewhat 
confusingly does so in a section on NULL pointers).  I interpreted it as 
a general prohibition on conditionals without an explicit operator 
(except for booleans).  I'd love to be corrected here!

>> +               if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32, &class_id) == 1)
> Could there be a case where PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2) exist but PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2)p(2) doesn't? In that case the parsing would be incorrect.
The MSDN documentation says that the most specific string will be 
returned first, in this case &CC_c(2)s(2)p(2), so if there is a 6-digit 
class ID we will return it.  That left me wondering what to do if we 
only encounter a 4-digit class ID.  If we ignore it, then we won't be 
able to match on the class ID.  We could parse it as 4-digits and supply 
zero for the p(2) field, but that left me wondering why Windows wouldn't 
have already done that for us.  What I see on my own systems is that 
there is always a 6-digit class ID defined, even if the pci-ids 
repository doesn't list any values for p(2) (e.g. 
https://pci-ids.ucw.cz/read/PD/07/80 reports as &CC_078000).

My conclusion was that was probably best just to return the first class 
ID that Windows reports even if it's only 4-digits.  That way the device 
can still be found and will match the CC_ info displayed in device manager.

Thanks,
Nick
  

26/01/2021 19:18, Nick Connolly:
> Hi Tal,
> 
> Thanks for the comments.
> 
> >> +       /* Try and find PCI class ID */
> >> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> > How about
> > for (cp = buf; cp[0] || cp[1]; cp++)
> That would be my preferred idiom, but the DPDK coding style (1.9.1) says 
> 'do not use ! for tests unless it is a boolean' (but somewhat 
> confusingly does so in a section on NULL pointers).  I interpreted it as 
> a general prohibition on conditionals without an explicit operator 
> (except for booleans).  I'd love to be corrected here!

That's true. Comparisons should be explicit.
  

On 26/01/2021 22:41, Thomas Monjalon wrote:
> That's true. Comparisons should be explicit.

Thanks Thomas,
Nick
  

> Subject: Re: [PATCH] bus/pci: nvme on Windows requires class id and bus
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Tal,
> 
> Thanks for the comments.
> 
> >> +       /* Try and find PCI class ID */
> >> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> > How about
> > for (cp = buf; cp[0] || cp[1]; cp++)
> That would be my preferred idiom, but the DPDK coding style (1.9.1) says 'do
> not use ! for tests unless it is a boolean' (but somewhat confusingly does so
> in a section on NULL pointers).  I interpreted it as a general prohibition on
> conditionals without an explicit operator (except for booleans).  I'd love to be
> corrected here!
> 
> >> +               if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32,
> >> + &class_id) == 1)
> > Could there be a case where PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2) exist
> but PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2)p(2) doesn't? In that case the
> parsing would be incorrect.
> The MSDN documentation says that the most specific string will be returned
> first, in this case &CC_c(2)s(2)p(2), so if there is a 6-digit class ID we will
> return it.  That left me wondering what to do if we only encounter a 4-digit
> class ID.  If we ignore it, then we won't be able to match on the class ID.  We
> could parse it as 4-digits and supply zero for the p(2) field, but that left me
> wondering why Windows wouldn't have already done that for us.  What I see
> on my own systems is that there is always a 6-digit class ID defined, even if
> the pci-ids repository doesn't list any values for p(2) (e.g.
> https://pci-ids.ucw.cz/read/PD/07/80 reports as &CC_078000).
> 
> My conclusion was that was probably best just to return the first class ID that
> Windows reports even if it's only 4-digits.  That way the device can still be
> found and will match the CC_ info displayed in device manager.

I'd shift the class_id value to the right in case the returned value is 4 digit, just to verify the result would be "c(2)s(2) 0" and not "0 s(2)p(2)", however when testing your change I couldn't find a case where only 4 digits matched.

Either than that LGTM.
> 
> Thanks,
> Nick