Received-SPF: pass (google.com: domain of linux-kernel+bounces-168896-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 5 May 2024 07:45:13 +0200
From: Lukas Wunner <lukas@wunner.de>
To: Nam Cao <namcao@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>, Yinghai Lu <yinghai@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: Re: [PATCH v2 2/2] PCI: pciehp: Abort hot-plug if
 pci_hp_add_bridge() fails
Message-ID: <Zjcc6Suf5HmmZVM9@wunner.de>
References: <cover.1714838173.git.namcao@linutronix.de>
 <f3db713f4a737756782be6e94fcea3eda352e39f.1714838173.git.namcao@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f3db713f4a737756782be6e94fcea3eda352e39f.1714838173.git.namcao@linutronix.de>

[cc += Ilpo, Mika]

On Sat, May 04, 2024 at 06:15:22PM +0200, Nam Cao wrote:
> If a bridge is hot-added without any bus number available for its
> downstream bus, pci_hp_add_bridge() will fail. However, the driver
> proceeds regardless, and the kernel crashes.
[...]
> Fix this by aborting the hot-plug if pci_hp_add_bridge() fails.
[...]
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -58,8 +58,10 @@ 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) {
> +		if (pci_hp_add_bridge(dev))
> +			goto out;
> +	}
>  
>  	pci_assign_unassigned_bridge_resources(bridge);
>  	pcie_bus_configure_settings(parent);

Are the curly braces even necessary?

FWIW, the rationale for returning 0 (success) in this case is that
pciehp has done its job by bringing up the slot and enumerating the
bridge in the slot.  It's not pciehp's fault that the hierarchy
cannot be extended further below the hot-added bridge.

Have you gone through the testing steps you spoke of earlier
(replacing the hot-added bridge with an Ethernet card) and do
they work correctly with this patch?

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas