Re: [PATCH] pci: fix broadcom secondary bus reset handling

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Lukas since you mentioned pciehp]

On Wed, May 01, 2024 at 07:51:18AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> After a link reset, the Broadcom / LSI PEX890xx PCIe Gen 5 Switch in synth
> mode will temporarily insert a fake place-holder device, 1000 02b2, before
> the link is actually active for the expected downstream device. Confirm
> the device's identifier matches what we expect before moving forward.
> Otherwise, the pciehp driver may unmask hotplug notifications before
> the link is actually active, which triggers an undesired device removal.

Is this a Switch that doesn't conform to the PCIe spec, or is there
something wrong with the way we're looking for a CRS completion?

In the absence of a device defect, I would not expect to need a
Broacom-specific comment in this code.

> Cc: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
> Cc: Peter Delevoryas <pdel@meta.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e5f243dd42884..4dc00f7411a94 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1255,6 +1255,7 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  	int delay = 1;
>  	bool retrain = false;
>  	struct pci_dev *bridge;
> +	u32 vid = dev->vendor | dev->device << 16;
>  
>  	if (pci_is_pcie(dev)) {
>  		bridge = pci_upstream_bridge(dev);
> @@ -1268,17 +1269,22 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  	 * responding to them with CRS completions.  The Root Port will
>  	 * generally synthesize ~0 (PCI_ERROR_RESPONSE) data to complete
>  	 * the read (except when CRS SV is enabled and the read was for the
> -	 * Vendor ID; in that case it synthesizes 0x0001 data).
> +	 * Vendor ID; in that case it synthesizes 0x0001 data, or if the device
> +	 * is downstream a Broadcom switch, which syntesizes a fake device)

s/syntesizes/synthesizes/

>  	 * Wait for the device to return a non-CRS completion.  Read the
> -	 * Command register instead of Vendor ID so we don't have to
> -	 * contend with the CRS SV value.
> +	 * Command register instead of Vendor ID so we don't have to contend
> +	 * with the CRS SV value. But, also read the Vendor and Device ID's
> +	 * to defeat Broadcom switch's placeholder device.

s/ID's/IDs/

>  	 */
>  	for (;;) {
> -		u32 id;
> +		u32 id, l;
>  
> +		pci_read_config_dword(dev, PCI_VENDOR_ID, &l);
>  		pci_read_config_dword(dev, PCI_COMMAND, &id);
> -		if (!PCI_POSSIBLE_ERROR(id))
> +
> +		if (!PCI_POSSIBLE_ERROR(id) && !PCI_POSSIBLE_ERROR(l) &&
> +		    l == vid)
>  			break;
>  
>  		if (delay > timeout) {
> -- 
> 2.43.0
>