On Wed, 2 Aug 2017 13:18:24 -0400
Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
> An endpoint is allowed to issue Configuration Request Retry Status (CRS)
> following a Function Level Reset (FLR) request to indicate that it is
> not ready to accept new requests. CRS is defined in PCIe r3.1, sec 2.3.1.
> Request Handling Rules and CRS usage in FLR context is mentioned in
> PCIe r3.1, sec 6.6.2. Function-Level Reset.
>
> Adding a vendor ID read if this is a physical function before attempting
> to read any other registers on the endpoint. A CRS indication will only
> be given if the address to be read is vendor ID register.
> pci_bus_read_dev_vendor_id() knows how to deal with CRS returned values.
>
> If pci_bus_read_dev_vendor_id() fails, it prints a user visible warning
> after provided 1 second timeout is reached. pci_flr_wait() will keep
> calling this function 60 times to allow up to 60 seconds to be consistent
> with the rest of the kernel CRS timeout handling.
>
> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
> ---
> drivers/pci/pci.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 2ed604a..01b28ac 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3813,30 +3813,40 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
>
> /*
> * We should only need to wait 100ms after FLR for virtual functions.
> - * Wait for up to 1000ms for config space to return something other than -1.
> - * Intel IGD requires this when an LCD panel is attached. We read the 2nd
> - * dword because VFs don't implement the 1st dword.
> + * Wait for up to 60s for config space to return something other than -1.
> + * Intel IGD requires 300ms when an LCD panel is attached. We use
> + * pci_bus_read_dev_vendor_id() for reading the vendor ID as it handles
> + * CRS gracefully.
> */
> static void pci_flr_wait(struct pci_dev *dev)
> {
> - int i = 0;
> + u32 sleep = 1000, total = 0;
> u32 id;
> + bool ret;
>
> if (dev->is_virtfn) {
> msleep(100);
> return;
> }
>
> + /* don't touch the HW before waiting 100ms */
> + msleep(100);
> +
Wouldn't it be better as:
msleep(100);
if (dev->is_virtfn)
return;
Perhaps with a spec reference in a comment why we don't care about
checking config space for the vf.
> do {
> - msleep(100);
> - pci_read_config_dword(dev, PCI_COMMAND, &id);
> - } while (i++ < 10 && id == ~0);
> -
> - if (id == ~0)
> - dev_warn(&dev->dev, "Failed to return from FLR\n");
> - else if (i > 1)
> - dev_info(&dev->dev, "Required additional %dms to return from FLR\n",
> - (i - 1) * 100);
> + ret = pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
> + sleep);
> + if (ret)
> + break;
> + total += sleep;
> + sleep *= 2;
> + } while (total < 60000 && !ret);
> +
> + if (!ret)
> + dev_warn(&dev->dev, "Failed to return from FLR after %ds\n",
> + total);
> + else if (total)
> + dev_info(&dev->dev, "Required additional %ds to return from FLR\n",
> + total);
> }
I'm not a big fan. Nested exponential backoff is pretty nasty. Are
there users of pci_bus_read_dev_vendor_id() that don't want a "still
trying" message? It seems better to add that to the function than try
to wrap this bandage around it. Thanks,
Alex
