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.
---
drivers/acpi/scan.c | 79 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 52 insertions(+), 27 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -435,42 +435,68 @@ static int acpi_generic_hotplug_event(st
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.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 (adev->flags.hotplug_notify)
+ return acpi_generic_hotplug_event(adev, type);
acpi_lock_hp_context();
- notify = adev->hp ? adev->hp->notify : NULL;
+
+ if (adev->hp) {
+ acpi_hp_notify notify = adev->hp->notify;
+
+ acpi_unlock_hp_context();
+
+ if (WARN_ON_ONCE(!notify))
+ return -EINVAL;
+
+ return notify(adev, type);
+ }
+
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 +511,6 @@ void acpi_device_hotplug(struct acpi_dev
break;
}
- err_out:
acpi_evaluate_ost(adev->handle, src, ost_code, NULL);
out:
