Re: [PATCH V3 2/2] PCI: handle CRS returned by device after FLR

[Sinan Kaya]

On 2/21/2017 3:51 PM, Alex Williamson wrote:
> On Tue, 21 Feb 2017 12:04:24 -0500
> Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
> 
>> Hi Alex,
>>
>> I'm coming back to work on this. 
>>
>> On 10/3/2016 1:37 AM, Sinan Kaya wrote:
>>> An endpoint is allowed to issue CRS following an FLR request to indicate
>>> that it is not ready to accept new requests. Changing the polling mechanism
>>> in FLR wait function to go read the vendor ID instead of the command/status
>>> register. A CRS indication will only be given if the address to be read is
>>> vendor ID.
>>>
>>> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
>>> ---
>>>  drivers/pci/pci.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index c8749b9..7580b00 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -3725,7 +3725,8 @@ static void pci_flr_wait(struct pci_dev *dev)
>>>  
>>>  	do {
>>>  		msleep(100);
>>> -		pci_read_config_dword(dev, PCI_COMMAND, &id);  
>>
>> Your comment here puzzled me. 
>>
>> https://patchwork.kernel.org/patch/8331851/
>>
>> "Self nak on this one, didn't account for VFs not implementing the first
>> dword.  Thanks,"
>>
>> I'm trying to add Configuration Request Retry Status (CRS) support to FLR
>> with this patch. 
>>
>> Basically, the root port will return 0xFFFF0001 only when a config read
>> request is sent to the vendor ID register and CRS visibility is set.
>> The SW needs to poll until this special read ID disappears. See the
>> implementation note on Configuration Request Retry Status in PCIE
>> specification for details.
>>
>> pci_bus_read_dev_vendor_id implements this loop for us.
>>
>> You are saying that there are VFs that do not implement vendor ID register.
>> Can you give some history on this?
> 
> SR-IOV spec rev 1.1, 3.4.1.1 & 3.4.1.2, Vendor ID and Device ID fields
> for the VF return 0xFFFF when read.  The "Virtualization Intermediary"
> is supposed to use the vendor ID from the PF and the device ID defined
> in the PF SR-IOV capability.


Interesting. Since lspci was showing the correct vendor id and device id, I
assumed that it is coming from offset 0.

Maybe, the right thing is to figure out if this is a virtual function or not.
If it is a physical function, check the CRS first before reading the command
register in the existing loop.


> 
>>
>>> +		pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
>>> +					   60 * 1000);
>>>  	} while (i++ < 10 && id == ~0);
> 
> pci_bus_read_dev_vendor_id() seems like it will return false with an
> id value of ~0 for a functional VF, so this loop will spin longer than
> necessary and report an invalid error.  Patch 1/2 from this series
> would cause pci_dev_restore() to be a no-op on VFs.  Thanks,
> 
> Alex
> 
> 


-- 
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.