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
