Re: acpiphp module and Asus GA402RJ

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux