2023-12-14 16:52:35

by Bjorn Helgaas

[permalink] [raw]
Subject: [PATCH] Revert "PCI: acpiphp: Reassign resources on bridge if necessary"

From: Bjorn Helgaas <[email protected]>

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 <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]
Reported-by: Dongli Zhang <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]
Reported-by: Jonathan Woithe <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: <[email protected]>
Cc: Igor Mammedov <[email protected]>
---
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


2023-12-14 16:54:20

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH] Revert "PCI: acpiphp: Reassign resources on bridge if necessary"

On Thu, Dec 14, 2023 at 10:51:02AM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> 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 <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> Reported-by: Dongli Zhang <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> Reported-by: Jonathan Woithe <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Cc: <[email protected]>
> Cc: Igor Mammedov <[email protected]>


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 <[email protected]>



> ---
> 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

2023-12-15 09:13:47

by Igor Mammedov

[permalink] [raw]
Subject: Re: [PATCH] Revert "PCI: acpiphp: Reassign resources on bridge if necessary"

On Thu, 14 Dec 2023 10:51:02 -0600
Bjorn Helgaas <[email protected]> wrote:

> From: Bjorn Helgaas <[email protected]>
>
> 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 <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> Reported-by: Dongli Zhang <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> Reported-by: Jonathan Woithe <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Cc: <[email protected]>
> Cc: Igor Mammedov <[email protected]>

Acked-by: Igor Mammedov <[email protected]>

> ---
> 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);