On Friday, September 27th, 2024 at 8:34 PM, Athul Krishna <athul.krishna.kr@xxxxxxxxxxxxxx> wrote: > On Friday, September 27th, 2024 at 1:23 AM, Rafael J. Wysocki rafael@xxxxxxxxxx wrote: > > > CC lists. > > > > On Wed, Sep 25, 2024 at 8:36 PM Accardi, Kristen C > > kristen.c.accardi@xxxxxxxxx wrote: > > > > > On Wed, 2024-09-25 at 17:52 +0000, Athul Krishna wrote: > > > > > > > Device: Asus Zephyrus G14 GA402RJ > > > > Kernel: 6.11 > > > > CPU: R7 67800H > > > > GPU: RX 6700S > > > > > > > > Hello Kristen, > > > > > > > > I'd like provide feedback on the acpiphp module. My laptop is all > > > > AMD(CPU+GPU). So the discrete gpu: \SB.PCI0.GPP0.SWUS.SWDS.VGA_, > > > > and the hotplug bridge: \SB.PCI0.GPP0. And there's two PCI-PCI > > > > bridges: SWUS and SWDS. > > > > > > > > Eject notification on the discrete GPU, will remove the GPU, and the > > > > two PCI bridges. > > > > > > > > The issue I've encountered is, Device check notification cannot reach > > > > GPU, as it's hotplug context is lost, with the current implementation > > > > of acpiphp(acpiphp_glue.c) module. > > > > > > > > I hope this helps. Also a small disclaimer: I'm a newbie to Linux > > > > kernel internals. So I might be limited in the help I can provide. > > > > I can provide dmesg logs or ACPI tables if required. > > > > > > > > Thanks & Regards, > > > > Athul > > > > > > Hi Athul, > > > I think that Rafael might be maintaining this driver these days. I've > > > copied him on this reply. > > > > I don't maintain it (Bjorn does that AFAICS), but I may be one of the > > developers who have touched it most recently. > > > > In any case, I don't think it is at fault here, but firmware > > expectations that go beyond provisions made by the ACPI specification: > > > > https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#device-object-notifications > > > > because it seems to really want to trigger a bus rescan starting from > > a device that has been discovered already that will lead to a > > discovery of a specific "new" device along with its parent. Sending a > > device check notification on a device that has not been discovered yet > > and whose parent has not been discovered yet either may reasonably be > > regarded as unsuitable for this purpose. > > > > That said, it appears to be possible to adjust the generic ACPI device > > notification handling code to take this firmware behavior into account > > kind of as expected. That is, if a device check notification is > > received on a device object without a hotplug context, look for its > > closest ancestor that has a hotplug context or otherwise can handle > > hotplug and trigger a bus check from it as though it got a bus check > > notification. > > > > Please check the attached (completely untested) patch. > > > Hello Rafael, > I've patched the kernel and tested it. In the dmesg log, you can see: > [ 77.467946] ACPI: \SB.PCI0.GPP0.SWUS: Bus check in hotplug_event() bridge: 0 > > I've modified the debug message to: > acpi_handle_debug(handle, "Bus check in %s() bridge: %d\n", func, bridge ? true : false); > > It seems acpi hotplug context exists for bridge \SB.PCI0.GPP0.SWUS. But acpiphp_bridge for the context > is NULL. I do see when free_bridge() is called, context->bridge = NULL. > > > Thanks, > Athul Hello Rafael, I've made some slight modifications to the patch you've provided. Basically, using acpi_device_enumerated() to check acpi_device, so that hotplug context of \_SB.PCI0.GPP0 will be used for Bus check notification. I've also modified enable_slot() to check whether the subordinate bus of GPP0 is empty, and call acpiphp_native_scan_bridge() on GPP0. But it seems there's some problem when binding driver(amdgpu) to the discrete GPU. I've attached the modified patch and dmesg logs. Thanks, Athul
From 3a92aabf3b058d7a63993f15308e05b2cd7978ff Mon Sep 17 00:00:00 2001
From: Athul Krishna K R <athul.krishna.kr@xxxxxxxxxxxxxx>
Date: Thu, 3 Oct 2024 19:17:41 +0000
Subject: [PATCH] ACPI hotplug extend for GA402RJ
---
drivers/acpi/scan.c | 79 +++++++++++++++++++-----------
drivers/pci/hotplug/acpiphp_glue.c | 32 ++++++++----
2 files changed, 73 insertions(+), 38 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7ecc401fb..34f032287 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -435,42 +435,66 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
return -EINVAL;
}
-void acpi_device_hotplug(struct acpi_device *adev, u32 src)
+static int acpi_device_hotplug_notify(struct acpi_device *adev, u32 type)
{
- u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
- int error = -ENODEV;
+ while (adev) {
+ /*
+ * The device object's ACPI handle cannot become invalid as long
+ * as acpi_scan_lock is being held, but it might have become
+ * invalid before that lock was acquired.
+ */
+ if (adev->handle == INVALID_ACPI_HANDLE)
+ return -ENODEV;
- lock_device_hotplug();
- mutex_lock(&acpi_scan_lock);
+ if (adev->flags.is_dock_station)
+ return dock_notify(adev, type);
- /*
- * The device object's ACPI handle cannot become invalid as long as we
- * are holding acpi_scan_lock, but it might have become invalid before
- * that lock was acquired.
- */
- if (adev->handle == INVALID_ACPI_HANDLE)
- goto err_out;
+ if (adev->flags.hotplug_notify)
+ return acpi_generic_hotplug_event(adev, type);
- if (adev->flags.is_dock_station) {
- error = dock_notify(adev, src);
- } else if (adev->flags.hotplug_notify) {
- error = acpi_generic_hotplug_event(adev, src);
- } else {
- acpi_hp_notify notify;
+ if (acpi_device_enumerated(adev)) {
+ acpi_lock_hp_context();
+
+ acpi_hp_notify notify = adev->hp->notify;
+
+ acpi_unlock_hp_context();
+
+ if (WARN_ON_ONCE(!notify))
+ return -EINVAL;
+
+ return notify(adev, type);
+ }
- acpi_lock_hp_context();
- notify = adev->hp ? adev->hp->notify : NULL;
- acpi_unlock_hp_context();
/*
* There may be additional notify handlers for device objects
- * without the .event() callback, so ignore them here.
+ * without the .event() callback, so ignore them here, but if
+ * the notification type is either ACPI_NOTIFY_DEVICE_CHECK or
+ * ACPI_NOTIFY_BUS_CHECK, propagate to the parent and above it.
*/
- if (notify)
- error = notify(adev, src);
- else
- goto out;
+ if (type == ACPI_NOTIFY_DEVICE_CHECK)
+ type = ACPI_NOTIFY_BUS_CHECK;
+ else if (type != ACPI_NOTIFY_BUS_CHECK)
+ return 1;
+
+ adev = acpi_dev_parent(adev);
}
- switch (error) {
+
+ return -ENODEV;
+}
+
+void acpi_device_hotplug(struct acpi_device *adev, u32 src)
+{
+ u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
+ int ret;
+
+ lock_device_hotplug();
+ mutex_lock(&acpi_scan_lock);
+
+ ret = acpi_device_hotplug_notify(adev, src);
+ if (ret > 0)
+ goto out;
+
+ switch (ret) {
case 0:
ost_code = ACPI_OST_SC_SUCCESS;
break;
@@ -485,7 +509,6 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
break;
}
- err_out:
acpi_evaluate_ost(adev->handle, src, ost_code, NULL);
out:
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 5b1f271c6..cdc7d5546 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -152,6 +152,7 @@ static void free_bridge(struct kref *kref)
struct acpiphp_bridge *bridge;
struct acpiphp_slot *slot, *next;
struct acpiphp_func *func, *tmp;
+ struct acpi_device *adev;
acpi_lock_hp_context();
@@ -167,10 +168,15 @@ static void free_bridge(struct kref *kref)
context = bridge->context;
/* Root bridges will not have hotplug context. */
if (context) {
+ adev = context->hp.self;
/* Release the reference taken by acpiphp_enumerate_slots(). */
put_bridge(context->func.parent);
context->bridge = NULL;
acpiphp_put_context(context);
+
+ acpi_scan_lock_acquire();
+ acpi_bus_trim(adev);
+ acpi_scan_lock_release();
}
put_device(&bridge->pci_bus->dev);
@@ -486,16 +492,20 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
struct acpiphp_func *func;
if (bridge && bus->self && hotplug_is_native(bus->self)) {
- /*
- * If native hotplug is used, it will take care of hotplug
- * slot management and resource allocation for hotplug
- * bridges. However, ACPI hotplug may still be used for
- * non-hotplug bridges to bring in additional devices such
- * as a Thunderbolt host controller.
- */
- for_each_pci_bridge(dev, bus) {
- if (PCI_SLOT(dev->devfn) == slot->device)
- acpiphp_native_scan_bridge(dev);
+ if (list_empty(&bus->devices)) {
+ acpiphp_native_scan_bridge(bus->self);
+ } else {
+ /*
+ * If native hotplug is used, it will take care of hotplug
+ * slot management and resource allocation for hotplug
+ * bridges. However, ACPI hotplug may still be used for
+ * non-hotplug bridges to bring in additional devices such
+ * as a Thunderbolt host controller.
+ */
+ for_each_pci_bridge(dev, bus) {
+ if (PCI_SLOT(dev->devfn) == slot->device)
+ acpiphp_native_scan_bridge(dev);
+ }
}
} else {
LIST_HEAD(add_list);
@@ -697,11 +707,13 @@ static void trim_stale_devices(struct pci_dev *dev)
static void acpiphp_check_bridge(struct acpiphp_bridge *bridge)
{
struct acpiphp_slot *slot;
+ struct acpi_device *adev = bridge->context->hp.self;
/* Bail out if the bridge is going away. */
if (bridge->is_going_away)
return;
+ acpi_bus_scan(adev->handle);
if (bridge->pci_dev)
pm_runtime_get_sync(&bridge->pci_dev->dev);
--
2.46.0
Attachment:
dmesg
Description: Binary data
