On Mon, 31 Jul 2023 16:42:51 -0500 Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: Hi Bjorn, Is there anything else for me to do to make this merged? (just checked recent branches in pci tree, and 'Revert "PCI: acpiphp: Reassign resources on bridge if necessary' is still there) > On Mon, Jul 31, 2023 at 02:44:18PM +0200, Igor Mammedov wrote: > > On Sat, 29 Jul 2023 16:50:09 -0500 Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > On Fri, Jul 28, 2023 at 11:32:16AM +0200, Igor Mammedov wrote: > > > > On Thu, 27 Jul 2023 12:41:02 -0500 Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > > 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. > > > > > 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 > > > > > > Thanks, that does seem a little suspect. ACPI r6.5 sec 5.6.6 says > > > that when OSPM handles a Bus Check, it should "perform a Plug and Play > > > re-enumeration operation on the device tree starting from the point > > > where it has been notified." > > > > > > PCI devices are enumerated by doing PCI config reads. It would make > > > sense to re-enumerate a PCI hierarchy starting with a PCI device > > > that's already known to the OS, e.g., by scanning the secondary bus of > > > a PCI-to-PCI bridge. > > > > > > I think there are two problems here: > > > > > > 1) The platform shouldn't send a Bus Check notification to a PCI > > > device that doesn't exist. How could the OS re-enumerate > > > starting there? > > > > in case of reported laptop, DSDT provides Device Descriptors > > for not existing root-ports. > > > > OSPM can't do anything with it but to pass Notify event to > > PCI bus-specific enumeration mechanism, and it's upto PCI subsystem > > to discard/ignore Notify() on this ACPI node. > > That seems backwards to me, but we have a fair bit of ACPI and PCI > stuff that's backwards. > > > > 2) Linux runs acpiphp_hotplug_notify() for Bus Checks to > > > non-existent PCI devices when it ignore them; reasoning below. > > > > > > We call acpiphp_enumerate_slots() in this path, which happens before > > > any of the PCI devices on the root bus have been enumerated: > > > > > > pci_register_host_bridge > > > pcibios_add_bus(root bus) > > > acpi_pci_add_bus > > > acpiphp_enumerate_slots(pci_bus *bus) > > > acpi_walk_namespace(acpiphp_add_context) > > > acpiphp_add_context(struct acpiphp_bridge *) > > > acpi_evaluate_integer("_ADR") > > > acpiphp_init_context > > > context->hp.notify = acpiphp_hotplug_notify > > > > > > So now we've already looked at RP03, RP07, and RP08, and set up the > > > .notify() handler for all of them. Since we haven't scanned the bus > > > yet, we don't know that RP03 exists and RP07 and RP08 do not. > > > > While ACPI doesn't forbid firmware to describe non-existing RP, > > the PCIe hostbridge can't hotplug extra root ports. (and QEMU > > follows PCIe design in this respect on 'q35' machine). > > > > However when it comes to hotplug on QEMU's 'pc' machine > > (hotplug on root bus), each slot has "Augmented Device > > Descriptors", that includes un-populated slots leading to > > the presence of .notify() handler on such slots. > > > > Then later on when device is hotplugged, a Notify(,1/*DeviceCheck*/) > > is sent to previously empty slot and from there on PCI subsystem > > re-enumerates either a single device or a bridge hierarchy > > (from the parent context). > > > > So I'd assume that we need to have .notify() handler for all slots > > that are described in DSDT (present and non present). > > Just from a "beautiful design" perspective, it seems suboptimal for > the ACPI device tree to have to include Device objects for all > possible hot-added devices. > > I would expect hot-add to be handled via a Bus Check to the *parent* > of a new device, so the device tree would only need to describe > hardware that's present at boot. That would mean pci_root.c would > have some .notify() handler, but I don't see anything there. > > I don't know if platforms really implement Root Port hot-add (maybe > qemu?), but if they do, I could believe they might do it via Notify to > an ACPI Device for a non-present hardware device. I wouldn't know > whether that's the intent of the spec, or just a reaction to something > that happened to work with existing OSes. > > > > $ qemu-system-x86_64 -M pc -m 512M -monitor stdio -cpu host --enable-kvm -kernel arch/x86/boot/bzImage -drive format=raw,file=ubuntu.img -append "root=/dev/sda1" > > > (qemu) device_add e1000 > > > > > > (For posterity, replacing "-monitor stdio" with "-nographic -monitor > > > telnet:localhost:7001,server,nowait,nodelay" and adding > > > "console=ttyS0,115200n8" to the -append made it easier to see the > > > crash details.) > > > > I've not put extra arguments, because there is a lot of ways > > one can configure/use monitor/serial options. > > > > But specifying full command line like yours will be useful > > for anyone who doesn't have any experience with QEMU CLI. > > Yep, that's the audience :) I want to make it as easy as reasonably > possible for non-qemu experts to repro things. > > > > I really wish we didn't have such different resource assignment paths > > > depending on whether the device is on a root bus or deeper in the > > > hierarchy. But we can't fix that now, so this seems like the right > > > thing. > > > > I've looked at possibility of making > > pci_assign_unassigned_bridge_resources() > > work without bridge pointer, but it looks not viable as it's > > a bridge dedicated function which on top of rearranging > > resources, also disables/reprograms/enables bridge. > > > > If there are ideas how to make it better, > > I can pick it up and try to implement. > > > > Testing shows that pci_assign_unassigned_bridge_resources() > > isn't ideal since it releases all resources before reassigning > > and then if the later fails bridge stays in mis-configured > > state (attempt to recover results in failing BAR assignment > > to children devices). > > It's not issue in case of > > root-port -> 1 child device hotplug > > since root port hadn't any working device[s] behind it. > > But in case of hotplug into PCI bridge, that leaves > > pre-existing devices behind the bridge broken (SHPC and acpiphp case). > > Yeah, it's a complicated mess. That's why I didn't think this would > be a viable fix in the short term. > > Bjorn >
