Re: [PATCH] PCI: hotplug: check the return of hotplug bridge

Re: [PATCH] PCI: hotplug: check the return of hotplug bridge

[Lukas Wunner]

[shorten subject, cc += Nam Cao, start of thread:
https://lore.kernel.org/all/20240817032228.6844-1-trintaeoitogc@gmail.com/
]

On Sat, Aug 17, 2024 at 12:22:27AM -0300, Guilherme Giacomo Simoes wrote:
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>

Hm, the body of the commit message ended up in the subject
and the patch was submitted twice.


> --- a/drivers/pci/hotplug/shpchp_pci.c
> +++ b/drivers/pci/hotplug/shpchp_pci.c
> @@ -48,8 +48,11 @@ int shpchp_configure_device(struct slot *p_slot)
>  	}
>  
>  	for_each_pci_bridge(dev, parent) {
> -		if (PCI_SLOT(dev->devfn) == p_slot->device)
> -			pci_hp_add_bridge(dev);
> +		if (PCI_SLOT(dev->devfn) == p_slot->device) {
> +			ret = pci_hp_add_bridge(dev);
> +			if (ret)
> +				goto out;
> +		}
>  	}
>  
>  	pci_assign_unassigned_bridge_resources(bridge);

Nam Cao worked on this back in May:

v1:
https://lore.kernel.org/all/cover.1714762038.git.namcao@linutronix.de/

v2:
https://lore.kernel.org/all/cover.1714838173.git.namcao@linutronix.de/

v3:
https://lore.kernel.org/all/cover.1715609848.git.namcao@linutronix.de/

Note that there was discussion on v2 after v3 had been submitted,
i.e. the last messages in the discussion are in the v2 thread.

Nam Cao's patches didn't get applied, I think we hadn't reached
consensus or were waiting for a v4.

Nam Cao's v2 uses the exact same approach that you're proposing
and they subsequently found a way to crash the kernel despite the
newly introduced error handling:

https://lore.kernel.org/all/20240506083701.NZNifFGn@linutronix.de/

So I'm afraid your patch may not work in every scenario.

Thanks,

Lukas

[PATCH] PCI: hotplug: check the return of hotplug bridge

[Guilherme Giacomo Simoes]

In some pci drivers, when the pci bridge is added if the process return an error,
the drivers don't check this and continue your execute normally.
Then, this patch change this drivers for check return of pci_hp_add_bridge(), and
if has an error, then the drivers call goto lable , free your mutex and return the
error for your caller.

Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 drivers/pci/hotplug/cpci_hotplug_pci.c | 7 +++++--
 drivers/pci/hotplug/cpqphp_pci.c       | 9 ++++++---
 drivers/pci/hotplug/ibmphp_core.c      | 8 ++++++--
 drivers/pci/hotplug/pciehp_pci.c       | 7 +++++--
 drivers/pci/hotplug/shpchp_pci.c       | 7 +++++--
 drivers/pci/probe.c                    | 9 ++++++---
 6 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
index 6c48066acb44..2d3256795eab 100644
--- a/drivers/pci/hotplug/cpci_hotplug_pci.c
+++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
@@ -269,8 +269,11 @@ int cpci_configure_slot(struct slot *slot)
 	parent = slot->dev->bus;
 
 	for_each_pci_bridge(dev, parent) {
-		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn))
-			pci_hp_add_bridge(dev);
+		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn)) {
+			ret = pci_hp_add_bridge(dev);
+			if (ret)
+				goto out;
+		}
 	}
 
 	pci_assign_unassigned_bridge_resources(parent->self);
diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index e9f1fb333a71..5f6ce2c6385a 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -70,7 +70,7 @@ static void __iomem *detect_HRT_floating_pointer(void __iomem *begin, void __iom
 int cpqhp_configure_device(struct controller *ctrl, struct pci_func *func)
 {
 	struct pci_bus *child;
-	int num;
+	int num, ret = 0;
 
 	pci_lock_rescan_remove();
 
@@ -97,7 +97,10 @@ int cpqhp_configure_device(struct controller *ctrl, struct pci_func *func)
 	}
 
 	if (func->pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
-		pci_hp_add_bridge(func->pci_dev);
+		ret = pci_hp_add_bridge(func->pci_dev);
+		if (ret)
+			goto out;
+
 		child = func->pci_dev->subordinate;
 		if (child)
 			pci_bus_add_devices(child);
@@ -107,7 +110,7 @@ int cpqhp_configure_device(struct controller *ctrl, struct pci_func *func)
 
  out:
 	pci_unlock_rescan_remove();
-	return 0;
+	return ret;
 }
 
 
diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
index 197997e264a2..73a593c2993b 100644
--- a/drivers/pci/hotplug/ibmphp_core.c
+++ b/drivers/pci/hotplug/ibmphp_core.c
@@ -663,6 +663,7 @@ static int ibm_configure_device(struct pci_func *func)
 	int num;
 	int flag = 0;	/* this is to make sure we don't double scan the bus,
 					for bridged devices primarily */
+	int ret = 0;
 
 	pci_lock_rescan_remove();
 
@@ -690,7 +691,10 @@ static int ibm_configure_device(struct pci_func *func)
 		}
 	}
 	if (!(flag) && (func->dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
-		pci_hp_add_bridge(func->dev);
+		ret = pci_hp_add_bridge(func->dev);
+		if (ret)
+			goto out;
+
 		child = func->dev->subordinate;
 		if (child)
 			pci_bus_add_devices(child);
@@ -698,7 +702,7 @@ static int ibm_configure_device(struct pci_func *func)
 
  out:
 	pci_unlock_rescan_remove();
-	return 0;
+	return ret;
 }
 
 /*******************************************************
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 65e50bee1a8c..0c4873c2ef3c 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -58,8 +58,11 @@ int pciehp_configure_device(struct controller *ctrl)
 		goto out;
 	}
 
-	for_each_pci_bridge(dev, parent)
-		pci_hp_add_bridge(dev);
+	for_each_pci_bridge(dev, parent) {
+		ret = pci_hp_add_bridge(dev);
+		if (ret)
+			goto out;
+	}
 
 	pci_assign_unassigned_bridge_resources(bridge);
 	pcie_bus_configure_settings(parent);
diff --git a/drivers/pci/hotplug/shpchp_pci.c b/drivers/pci/hotplug/shpchp_pci.c
index 36db0c3c4ea6..7db0ce966f1d 100644
--- a/drivers/pci/hotplug/shpchp_pci.c
+++ b/drivers/pci/hotplug/shpchp_pci.c
@@ -48,8 +48,11 @@ int shpchp_configure_device(struct slot *p_slot)
 	}
 
 	for_each_pci_bridge(dev, parent) {
-		if (PCI_SLOT(dev->devfn) == p_slot->device)
-			pci_hp_add_bridge(dev);
+		if (PCI_SLOT(dev->devfn) == p_slot->device) {
+			ret = pci_hp_add_bridge(dev);
+			if (ret)
+				goto out;
+		}
 	}
 
 	pci_assign_unassigned_bridge_resources(bridge);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b14b9876c030..2418998820dc 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3363,9 +3363,10 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 		if (!pci_find_bus(pci_domain_nr(parent), busnr))
 			break;
 	}
+
 	if (busnr-- > end) {
 		pci_err(dev, "No bus number available for hot-added bridge\n");
-		return -1;
+		return -ENODEV;
 	}
 
 	/* Scan bridges that are already configured */
@@ -3380,8 +3381,10 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 	/* Scan bridges that need to be reconfigured */
 	pci_scan_bridge_extend(parent, dev, busnr, available_buses, 1);
 
-	if (!dev->subordinate)
-		return -1;
+	if (!dev->subordinate) {
+		pci_err(dev, "No dev subordinate\n");
+		return -ENODEV;
+	}
 
 	return 0;
 }
-- 
2.46.0