Re: [PATCHv3 2/5] pci: Add is_removed state

[Lukas Wunner <lukas@wunner.de>]

On Tue, Sep 27, 2016 at 04:23:32PM -0400, Keith Busch wrote:
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -109,6 +109,8 @@ int pciehp_unconfigure_device(struct slot *p_slot)
>  				break;
>  			}
>  		}
> +		if (!presence)
> +			dev->is_removed = 1;
>  		pci_stop_and_remove_bus_device(dev);
>  		/*
>  		 * Ensure that no new Requests will be generated from

Sorry for the delay Keith, I finally got around to test v3 of your
series with hot-removed Thunderbolt devices on Apple Macs.

I've found that the above isn't sufficient, it's necessary to also
set the is_removed bit on any child devices.  E.g. on my system
when an Apple Gigabit Ethernet adapter is plugged in, the topology
looks like this:

0000:06:04.0 --- 0000:09:00.0 --- 0000:0a:00.0 --- 0000:0b:00.0

Hotplug port     Upstream bridge  Downstream br.   Broadcom 57762
of TB host       of TB switch in
                 Ethernet adapter

With your patch above, the is_removed bit is only set on 0000:09:00.0
but not on its children.  Consequently the "tg3" driver tries to
access the hot-removed Broadcom 57762 Ethernet chip as before,
causing a soft lockup.

The diff below fixes this for me, could you fold that into your patch?
The same change might also be necessary in pcie-dpc.c. Feel free to
rename "set_is_removed_cb()" if you don't like it.

Thanks,

Lukas

-- >8 --
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 299ea5e..ec26eb7 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -73,6 +73,12 @@ int pciehp_configure_device(struct slot *p_slot)
 	return ret;
 }
 
+static int set_is_removed_cb(struct pci_dev *pdev, void *unused)
+{
+	pdev->is_removed = 1;
+	return 0;
+}
+
 int pciehp_unconfigure_device(struct slot *p_slot)
 {
 	int rc = 0;
@@ -109,8 +115,11 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 				break;
 			}
 		}
-		if (!presence)
+		if (!presence) {
 			dev->is_removed = 1;
+			if (pci_has_subordinate(dev))
+				pci_walk_bus(dev->subordinate, set_is_removed_cb, NULL);
+		}
 		pci_stop_and_remove_bus_device(dev);
 		/*
 		 * Ensure that no new Requests will be generated from