linux-kernel - Re: [PATCH V5 2/2] PCI: handle CRS returned by device after FLR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170801224440.3f677a8d@w520.home>
Date:   Tue, 1 Aug 2017 22:44:40 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Sinan Kaya <okaya@...eaurora.org>
Cc:     linux-pci@...r.kernel.org, timur@...eaurora.org,
        linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V5 2/2] PCI: handle CRS returned by device after FLR

On Tue,  1 Aug 2017 23:44:13 -0400
Sinan Kaya <okaya@...eaurora.org> 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@...eaurora.org>
> ---
>  drivers/pci/pci.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 2ed604a..25c7a83 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3813,14 +3813,16 @@ 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 1s when an LCD panel is attached.  We use
> + * pci_bus_read_dev_vendor_id() for reading the vendor ID as it handles
> + * CRS gracefully.

nit, stating that IGD requires 1s with an LCD panel is a
misinterpretation of the previous comment.  In fact the original commit
only mentions 300ms.  I think perhaps 1s was simply a nice round
interval.

>   */
>  static void pci_flr_wait(struct pci_dev *dev)
>  {
>  	int i = 0;
>  	u32 id;
> +	bool ret;
>  
>  	if (dev->is_virtfn) {
>  		msleep(100);
> @@ -3828,15 +3830,15 @@ static void pci_flr_wait(struct pci_dev *dev)
>  	}
>  
>  	do {
> -		msleep(100);
> -		pci_read_config_dword(dev, PCI_COMMAND, &id);
> -	} while (i++ < 10 && id == ~0);
> +		ret = pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
> +						 1000);

Is it a problem that there's now zero delay between the FLR and first
attempt to read config space?  Seems like there should be a 100ms
delay before we start trying.  This is also going to print a kernel
warning 60 times in the course of getting to a 60s timeout, why not let
pci_bus_read_dev_vendor_id() manage the entire timeout?  Are we only
trying to preserve the dev_info() below?  Thanks,

Alex

> +	} while (i++ < 60 && !ret);
>  
> -	if (id == ~0)
> +	if (!ret)
>  		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);
> +		dev_info(&dev->dev, "Required additional %ds to
> return from FLR\n",
> +			 (i - 1));
>  }
>  
>  /**