On 8/10/2017 6:32 PM, Sinan Kaya wrote:
>> Alex was concerned that pci_bus_read_dev_vendor_id() could return
>> false ("no device here") with an ID value of ~0 for a functional VF
>> [1].
>>
>> I think that is indeed possible:
>>
>> - a VF that is ready will return 0xffff as both Vendor ID and Device
>> ID (SR-IOV r1.1, sec 3.4.1.1, .2), so the very first read in
>> pci_bus_read_dev_vendor_id() would see 0xffffffff and return
>> "false".
>>
>> - a VF that needs more time will return CRS and we'll loop in
>> pci_bus_read_dev_vendor_id() until it becomes ready, and we'll
>> return "true".
>>
>> Falling into the code below for the "false" case probably will work,
>> but it's a little bit ugly because (a) we're using two mechanisms to
>> figure out when the device is ready for config requests, and (b) we
>> have to worry about VFs both in pci_bus_read_dev_vendor_id() and here
>> in the caller.
> OK, I'm open to improving the code.
>
>> Here's an idea to make pci_bus_read_dev_vendor_id() work for both VFs
>> and PFs. It can't distinguish the 0xffffffff from a VF vs one from a
>> non-existent device, but the caller might be able to pass that
>> information in, e.g., when we're enumerating and don't know whether
>> the device exists, we don't have a pci_dev and would use this:
> How about creating a pci_bus_wait_crs() function with the loop in
> pci_bus_read_dev_vendor_id() and calling it from both places?
>
I prototyped both of the options. I found pci_bus_wait_crs() to be cleaner
due to this.
is_vf looked like another hack like mine to tap into the CRS handling
inside the pci_bus_read_dev_vendor_id().
I think the right thing is to make CRS handling a first class citizen rather
than hiding it and overriding functions with unnecessary parameters.
I'll post V10 in a minute. It passed my testing.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
