Thank you to both you and Woody for chasing this down!
On Wed, Jul 26, 2023 at 02:35:18PM +0200, Igor Mammedov wrote:
> Commit [1] switched acpiphp hotplug to use
> pci_assign_unassigned_bridge_resources()
> which depends on bridge being available, however in some cases
> when acpiphp is in use, enable_slot() can get a slot without
> bridge associated.
> 1. legitimate case of hotplug on root bus
> (likely not exiting on real hw, but widely used in virt world)
> 2. broken firmware, that sends 'Bus check' events to non
> existing root ports (Dell Inspiron 7352/0W6WV0), which somehow
> endup at acpiphp:enable_slot(..., bridge = 0) and with bus
> without bridge assigned to it.
Do we have evidence about the details of this non-existent root port?
If we do, I think it would be interesting to include a URL to them in
case there's some hole in the way we handle Bus Check events.
> Issue is easy to reproduce with QEMU's 'pc' machine provides
> PCI hotplug on hostbridge slots. to reproduce boot kernel at
> commit [1] in VM started with followin CLI and hotplug a device:
You mention CLI; did you mean to include a qemu command line here?
Maybe it's the same thing mentioned in the 40613da52b13 commit log?
I tried briefly to reproduce this using the 40613da52b13 command line
but haven't quite got it going yet. I think it would be very useful
to either include it here again or point to the 40613da52b13 commit
log.
> once guest OS is fully booted at qemu prompt:
>
> (qemu) device_add e1000
>
> it will cause NULL pointer dereference at
>
> void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
> {
> struct pci_bus *parent = bridge->subordinate;
>
> [ 612.277651] BUG: kernel NULL pointer dereference, address: 0000000000000018
> [...]
> [ 612.277798] ? pci_assign_unassigned_bridge_resources+0x1f/0x260
> [ 612.277804] ? pcibios_allocate_dev_resources+0x3c/0x2a0
> [ 612.277809] enable_slot+0x21f/0x3e0
> [ 612.277816] acpiphp_hotplug_notify+0x13d/0x260
> [ 612.277822] ? __pfx_acpiphp_hotplug_notify+0x10/0x10
> [ 612.277827] acpi_device_hotplug+0xbc/0x540
> [ 612.277834] acpi_hotplug_work_fn+0x15/0x20
> [ 612.277839] process_one_work+0x1f7/0x370
> [ 612.277845] worker_thread+0x45/0x3b0
> [ 612.277850] ? __pfx_worker_thread+0x10/0x10
> [ 612.277854] kthread+0xdc/0x110
> [ 612.277860] ? __pfx_kthread+0x10/0x10
> [ 612.277866] ret_from_fork+0x28/0x40
> [ 612.277871] ? __pfx_kthread+0x10/0x10
> [ 612.277876] ret_from_fork_asm+0x1b/0x30
>
> The issue was discovered on Dell Inspiron 7352/0W6WV0 laptop with
> following sequence:
> 1. suspend to RAM
> 2. wake up with the same backtrace being observed:
> 3. 2nd suspend to RAM attempt makes laptop freeze
>
> Fix it by using __pci_bus_assign_resources() instead of
> pci_assign_unassigned_bridge_resources()as we used to do
> but only in case when bus doesn't have a bridge associated
> with it.
>
> That let us keep hotplug on root bus working like it used to be
> but at the same time keeps resource reassignment usable on
> root ports (and other 1st level bridges) that was fixed by [1].
>
> 1)
> Fixes: 40613da52b13 ("PCI: acpiphp: Reassign resources on bridge if necessary")
> Link: https://lore.kernel.org/r/11fc981c-af49-ce64-6b43-3e282728bd1a@xxxxxxxxx
> Reported-by: Woody Suwalski <terraluna977@xxxxxxxxx>
> Signed-off-by: Igor Mammedov <imammedo@xxxxxxxxxx>
> ---
> drivers/pci/hotplug/acpiphp_glue.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 328d1e416014..3bc4e1f3efee 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -498,6 +498,7 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> acpiphp_native_scan_bridge(dev);
> }
> } else {
> + LIST_HEAD(add_list);
> int max, pass;
>
> acpiphp_rescan_slot(slot);
> @@ -511,10 +512,15 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> if (pass && dev->subordinate) {
> check_hotplug_bridge(slot, dev);
> pcibios_resource_survey_bus(dev->subordinate);
> + if (!bus->self)
> + __pci_bus_size_bridges(dev->subordinate, &add_list);
> }
> }
> }
> - pci_assign_unassigned_bridge_resources(bus->self);
> + if (bus->self)
> + pci_assign_unassigned_bridge_resources(bus->self);
> + else
> + __pci_bus_assign_resources(bus, &add_list, NULL);
> }
>
> acpiphp_sanitize_bus(bus);
> --
> 2.39.3
>
