| Message ID | 20231214165102.1093961-1-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9ad2ae63cdcad59b243e9ef548398c73e733c8df |
| Headers | show |
| Series | Revert "PCI: acpiphp: Reassign resources on bridge if necessary" | expand |
On Thu, Dec 14, 2023 at 10:51:02AM -0600, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > This reverts commit 40613da52b13fb21c5566f10b287e0ca8c12c4e9 and the > subsequent fix to it: > > cc22522fd55e ("PCI: acpiphp: Use pci_assign_unassigned_bridge_resources() only for non-root bus") > > 40613da52b13 fixed a problem where hot-adding a device with large BARs > failed if the bridge windows programmed by firmware were not large enough. > > cc22522fd55e ("PCI: acpiphp: Use pci_assign_unassigned_bridge_resources() > only for non-root bus") fixed a problem with 40613da52b13: an ACPI hot-add > of a device on a PCI root bus (common in the virt world) or firmware > sending ACPI Bus Check to non-existent Root Ports (e.g., on Dell Inspiron > 7352/0W6WV0) caused a NULL pointer dereference and suspend/resume hangs. > > Unfortunately the combination of 40613da52b13 and cc22522fd55e caused other > problems: > > - Fiona reported that hot-add of SCSI disks in QEMU virtual machine fails > sometimes. > > - Dongli reported a similar problem with hot-add of SCSI disks. > > - Jonathan reported a console freeze during boot on bare metal due to an > error in radeon GPU initialization. > > Revert both patches to avoid adding these problems. This means we will > again see the problems with hot-adding devices with large BARs and the NULL > pointer dereferences and suspend/resume issues that 40613da52b13 and > cc22522fd55e were intended to fix. > > Fixes: 40613da52b13 ("PCI: acpiphp: Reassign resources on bridge if necessary") > Fixes: cc22522fd55e ("PCI: acpiphp: Use pci_assign_unassigned_bridge_resources() only for non-root bus") > Reported-by: Fiona Ebner <f.ebner@proxmox.com> > Closes: https://lore.kernel.org/r/9eb669c0-d8f2-431d-a700-6da13053ae54@proxmox.com > Reported-by: Dongli Zhang <dongli.zhang@oracle.com> > Closes: https://lore.kernel.org/r/3c4a446a-b167-11b8-f36f-d3c1b49b42e9@oracle.com > Reported-by: Jonathan Woithe <jwoithe@just42.net> > Closes: https://lore.kernel.org/r/ZXpaNCLiDM+Kv38H@marvin.atrad.com.au > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: <stable@vger.kernel.org> > Cc: Igor Mammedov <imammedo@redhat.com> It's up to you whether to apply the revert - hopefully a fix can be developed soon. The revert itself looks like it's done correctly so from that POV: Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/pci/hotplug/acpiphp_glue.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index 601129772b2d..5b1f271c6034 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -512,15 +512,12 @@ 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 (pci_is_root_bus(bus)) > - __pci_bus_size_bridges(dev->subordinate, &add_list); > + __pci_bus_size_bridges(dev->subordinate, > + &add_list); > } > } > } > - if (pci_is_root_bus(bus)) > - __pci_bus_assign_resources(bus, &add_list, NULL); > - else > - pci_assign_unassigned_bridge_resources(bus->self); > + __pci_bus_assign_resources(bus, &add_list, NULL); > } > > acpiphp_sanitize_bus(bus); > -- > 2.34.1
On Thu, 14 Dec 2023 10:51:02 -0600 Bjorn Helgaas <helgaas@kernel.org> wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > This reverts commit 40613da52b13fb21c5566f10b287e0ca8c12c4e9 and the > subsequent fix to it: > > cc22522fd55e ("PCI: acpiphp: Use pci_assign_unassigned_bridge_resources() only for non-root bus") > > 40613da52b13 fixed a problem where hot-adding a device with large BARs > failed if the bridge windows programmed by firmware were not large enough. > > cc22522fd55e ("PCI: acpiphp: Use pci_assign_unassigned_bridge_resources() > only for non-root bus") fixed a problem with 40613da52b13: an ACPI hot-add > of a device on a PCI root bus (common in the virt world) or firmware > sending ACPI Bus Check to non-existent Root Ports (e.g., on Dell Inspiron > 7352/0W6WV0) caused a NULL pointer dereference and suspend/resume hangs. > > Unfortunately the combination of 40613da52b13 and cc22522fd55e caused other > problems: > > - Fiona reported that hot-add of SCSI disks in QEMU virtual machine fails > sometimes. > > - Dongli reported a similar problem with hot-add of SCSI disks. > > - Jonathan reported a console freeze during boot on bare metal due to an > error in radeon GPU initialization. > > Revert both patches to avoid adding these problems. This means we will > again see the problems with hot-adding devices with large BARs and the NULL > pointer dereferences and suspend/resume issues that 40613da52b13 and > cc22522fd55e were intended to fix. > > Fixes: 40613da52b13 ("PCI: acpiphp: Reassign resources on bridge if necessary") > Fixes: cc22522fd55e ("PCI: acpiphp: Use pci_assign_unassigned_bridge_resources() only for non-root bus") > Reported-by: Fiona Ebner <f.ebner@proxmox.com> > Closes: https://lore.kernel.org/r/9eb669c0-d8f2-431d-a700-6da13053ae54@proxmox.com > Reported-by: Dongli Zhang <dongli.zhang@oracle.com> > Closes: https://lore.kernel.org/r/3c4a446a-b167-11b8-f36f-d3c1b49b42e9@oracle.com > Reported-by: Jonathan Woithe <jwoithe@just42.net> > Closes: https://lore.kernel.org/r/ZXpaNCLiDM+Kv38H@marvin.atrad.com.au > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: <stable@vger.kernel.org> > Cc: Igor Mammedov <imammedo@redhat.com> Acked-by: Igor Mammedov <imammedo@redhat.com> > --- > drivers/pci/hotplug/acpiphp_glue.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index 601129772b2d..5b1f271c6034 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -512,15 +512,12 @@ 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 (pci_is_root_bus(bus)) > - __pci_bus_size_bridges(dev->subordinate, &add_list); > + __pci_bus_size_bridges(dev->subordinate, > + &add_list); > } > } > } > - if (pci_is_root_bus(bus)) > - __pci_bus_assign_resources(bus, &add_list, NULL); > - else > - pci_assign_unassigned_bridge_resources(bus->self); > + __pci_bus_assign_resources(bus, &add_list, NULL); > } > > acpiphp_sanitize_bus(bus);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 601129772b2d..5b1f271c6034 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -512,15 +512,12 @@ 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 (pci_is_root_bus(bus)) - __pci_bus_size_bridges(dev->subordinate, &add_list); + __pci_bus_size_bridges(dev->subordinate, + &add_list); } } } - if (pci_is_root_bus(bus)) - __pci_bus_assign_resources(bus, &add_list, NULL); - else - pci_assign_unassigned_bridge_resources(bus->self); + __pci_bus_assign_resources(bus, &add_list, NULL); } acpiphp_sanitize_bus(bus);