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