On 8/17/2017 11:00 PM, Bjorn Helgaas wrote:
> On Fri, Aug 11, 2017 at 12:56:34PM -0400, Sinan Kaya wrote:
>> Kernel is hiding Configuration Request Retry Status (CRS) inside
>> pci_bus_read_dev_vendor_id() function. We are looking to add support for
>> Function Level Reset (FLR) where vendor id read returns ~0.
>>
>> Move CRS handling into its own function so that it can be called from other
>> places as well.
>
> I think this is a much better idea than what I proposed. I still have
> a few questions proposals. I'll post a v11 to show what I'm thinking.
>
Sure, let me know. I can test it.
>> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
>> ---
>> drivers/pci/pci.h | 2 ++
>> drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++--------------
>> 2 files changed, 32 insertions(+), 14 deletions(-)
>>
>> msleep(delay);
>> delay *= 2;
>> if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
>> @@ -1858,6 +1846,34 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>> PCI_FUNC(devfn));
>> return false;
>
> While staring at this, I think I found a pre-existing bug in
> pci_bus_read_dev_vendor_id(). It looks like this:
>
> while ((*l & 0xffff) == 0x0001) {
> pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l);
> if (delay > crs_timeout)
> return false;
> }
>
> The problem is that the config read may have *succeeded* that last
> time before we time out. I think the correct sequence is:
>
> - check for timeout
> - read PCI_VENDOR_ID
> - check for CRS
>
Yeah, it makes sense.
>> }
>> + } while ((*l & 0xffff) == 0x0001);
>> +
>> + return true;
>> +}
>> +EXPORT_SYMBOL(pci_bus_wait_crs);
>
> I don't think we need EXPORT_SYMBOL here, do we?
copy/paste mistake.
>
>> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>> + int crs_timeout)
>> +{
>> + if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
>> + return false;
>> +
>> + /* some broken boards return 0 or ~0 if a slot is empty: */
>> + if (*l == 0xffffffff || *l == 0x00000000 ||
>> + *l == 0x0000ffff || *l == 0xffff0000)
>> + return false;
>> +
>> + /*
>> + * Configuration Request Retry Status. Some root ports return the
>> + * actual device ID instead of the synthetic ID (0xFFFF) required
>> + * by the PCIe spec. Ignore the device ID and only check for
>> + * (vendor id == 1).
>> + */
>> + if ((*l & 0xffff) == 0x0001) {
>> + if (!crs_timeout)
>> + return false;
>
> One thing I don't like is that every caller of pci_bus_wait_crs() has
> to know about the 0x0001 value.
Another helper function? I was trying to poll for CRS completion only
if we know that we are observing CRS.
>
>> + return pci_bus_wait_crs(bus, devfn, l, crs_timeout);
>> }
>>
>> return true;
>> --
>> 1.9.1
>>
>
--
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.
