On Thu, 27 Jul 2023 12:41:02 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> 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.
it's scattered over logs Woody has provided, here are links to
emails with
1: lspci output
https://lore.kernel.org/r/92150d8d-8a3a-d600-a996-f60a8e4c876c@xxxxxxxxx/
according to lscpi and dmesg there is only one root-port at 1c.0
which is occupied by wifi card
while DSTD table has more ports described, which is fine as long as
missing/disabled are not reported as present.
2: last round of logs with debug patch /before 40613da5, with 40613da5, and after/
https://lore.kernel.org/r/46437825-3bd0-2f8a-12d8-98a2b54d7c22@xxxxxxxxx/
here dmesg shows 1st correct port
ACPI: \_SB_.PCI0.RP03: acpiphp_glue: Bus check in hotplug_event(): bridge: 000000000dad0b34
and then later on
ACPI: \_SB_.PCI0.RP07: acpiphp_glue: Bus check in hotplug_event(): bridge: 0000000000000000
ACPI: \_SB_.PCI0.RP08: acpiphp_glue: Bus check in hotplug_event(): bridge: 0000000000000000
which aren't recognized as bridge
I don't know ACPICA code enough to guesstimate where we might miss
a check that device is actually exists to do further debug
over mail list within reasonable timeframe.
> > 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.
my bad, I didn't realize that saying 'pc' machine is not sufficient.
minimal CLI can be (important part '-M pc -monitor stdio',
the rest is for making guest boot and run at tolerable speed):
$QEMU -M pc -m 4G -monitor stdio -cpu host --enable-kvm vm_disk_image
Will you amend commit message or shall I repost with changes/Acks?
> > 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
> >
>
