Re: [PATCH v2 2/2] PCI: pciehp: Abort hot-plug if pci_hp_add_bridge() fails

[Nam Cao]

On Sun, May 05, 2024 at 07:45:13AM +0200, Lukas Wunner wrote:
> 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?

Nope. I thought this is the kernel's coding style, since checkpatch.pl
didn't complain. But checkpatch also doesn't complain after I remove it,
so no I guess that's not 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?

Yes.

> Reviewed-by: Lukas Wunner <lukas@xxxxxxxxx>

Thanks for the review. I will send v3 with the brackets removed.

Best regards,
Nam