diff mbox series

[v2,4/5] ACPI: scan: Rework Device Check and Bus Check notification handling

Message ID 1958983.PYKUYFuaPT@kreacher
State New
Headers show
Series ACPI: scan: Check enabled _STA bit on Bus/Device Checks | expand

Commit Message

Rafael J. Wysocki Feb. 26, 2024, 4:45 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The underlying problem is the handling of the enabled bit in device
status (bit 1 of _STA return value) which is required by the ACPI
specification to be observed in addition to the present bit (bit 0
of _STA return value) [1], but Linux does not observe it.

Since Linux has not looked at that bit for a long time, it is generally
risky to start obseving it in all device enumeration cases, especially
at the system initialization time, but it can be observed when the
kernel receives a Bus Check or Device Check notification indicating a
change in device configuration.  In those cases, seeing the enabled bit
clear may be regarded as an indication that the device at hand should
not be used any more.

For this reason, rework the handling of Device Check and Bus Check
notifications in the ACPI core device enumeration code in the
following way:

 1. Rename acpi_bus_trim_one() to acpi_scan_check_and_detach() and make
    it check device status if its second argument is not NULL, in which
    case it will detach scan handlers or ACPI drivers from devices whose
    _STA returns the enabled bit clear.

 2. Make acpi_scan_device_check() and acpi_scan_bus_check() invoke
    acpi_scan_check_and_detach() with a non-NULL second argument
    unconditionally, so scan handlers and ACPI drivers are detached
    from the target device and its ancestors if their _STA returns the
    enabled bit clear.

Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Split patch [3/4] from v1 and put changes related to the "enabled" _STA bit
     into this patch.
   * Leave the acpi_device_is_present() check in acpi_scan_device_check(), so as to
     continue walking the bus below the device when it is not enabled (for
     backwards compatibility).

---
 drivers/acpi/scan.c |   84 +++++++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

Comments

Jonathan Cameron Feb. 27, 2024, 9:39 a.m. UTC | #1 
On Mon, 26 Feb 2024 17:45:11 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The underlying problem is the handling of the enabled bit in device
> status (bit 1 of _STA return value) which is required by the ACPI
> specification to be observed in addition to the present bit (bit 0
> of _STA return value) [1], but Linux does not observe it.
> 
> Since Linux has not looked at that bit for a long time, it is generally
> risky to start obseving it in all device enumeration cases, especially
> at the system initialization time, but it can be observed when the
> kernel receives a Bus Check or Device Check notification indicating a
> change in device configuration.  In those cases, seeing the enabled bit
> clear may be regarded as an indication that the device at hand should
> not be used any more.
> 
> For this reason, rework the handling of Device Check and Bus Check
> notifications in the ACPI core device enumeration code in the
> following way:
> 
>  1. Rename acpi_bus_trim_one() to acpi_scan_check_and_detach() and make
>     it check device status if its second argument is not NULL, in which
>     case it will detach scan handlers or ACPI drivers from devices whose
>     _STA returns the enabled bit clear.

New name is much better - thanks!

> 
>  2. Make acpi_scan_device_check() and acpi_scan_bus_check() invoke
>     acpi_scan_check_and_detach() with a non-NULL second argument
>     unconditionally, so scan handlers and ACPI drivers are detached
>     from the target device and its ancestors if their _STA returns the
>     enabled bit clear.
> 
> Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Looks good.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
diff mbox series

Patch

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -244,11 +244,27 @@  static int acpi_scan_try_to_offline(stru
 	return 0;
 }
 
-static int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
+static int acpi_scan_check_and_detach(struct acpi_device *adev, void *check)
 {
 	struct acpi_scan_handler *handler = adev->handler;
 
-	acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
+	acpi_dev_for_each_child_reverse(adev, acpi_scan_check_and_detach, check);
+
+	if (check) {
+		acpi_bus_get_status(adev);
+		/*
+		 * Skip devices that are still there and take the enabled
+		 * flag into account.
+		 */
+		if (acpi_device_is_enabled(adev))
+			return 0;
+
+		/* Skip device that have not been enumerated. */
+		if (!acpi_device_enumerated(adev)) {
+			dev_dbg(&adev->dev, "Still not enumerated\n");
+			return 0;
+		}
+	}
 
 	adev->flags.match_driver = false;
 	if (handler) {
@@ -270,6 +286,11 @@  static int acpi_bus_trim_one(struct acpi
 	return 0;
 }
 
+static void acpi_scan_check_subtree(struct acpi_device *adev)
+{
+	acpi_scan_check_and_detach(adev, (void *)true);
+}
+
 static int acpi_scan_hot_remove(struct acpi_device *device)
 {
 	acpi_handle handle = device->handle;
@@ -315,42 +336,30 @@  static int acpi_scan_hot_remove(struct a
 	return 0;
 }
 
-static int acpi_scan_device_not_enumerated(struct acpi_device *adev)
-{
-	if (!acpi_device_enumerated(adev)) {
-		dev_warn(&adev->dev, "Still not enumerated\n");
-		return -EALREADY;
-	}
-	acpi_bus_trim(adev);
-	return 0;
-}
-
 static int acpi_scan_device_check(struct acpi_device *adev)
 {
 	int error;
 
-	acpi_bus_get_status(adev);
-	if (acpi_device_is_present(adev)) {
-		/*
-		 * This function is only called for device objects for which
-		 * matching scan handlers exist.  The only situation in which
-		 * the scan handler is not attached to this device object yet
-		 * is when the device has just appeared (either it wasn't
-		 * present at all before or it was removed and then added
-		 * again).
-		 */
-		if (adev->handler) {
-			dev_dbg(&adev->dev, "Already enumerated\n");
-			return 0;
-		}
-		error = acpi_bus_scan(adev->handle);
-		if (error) {
-			dev_warn(&adev->dev, "Namespace scan failure\n");
-			return error;
-		}
-	} else {
-		error = acpi_scan_device_not_enumerated(adev);
+	acpi_scan_check_subtree(adev);
+
+	if (!acpi_device_is_present(adev))
+		return 0;
+
+	/*
+	 * This function is only called for device objects for which matching
+	 * scan handlers exist.  The only situation in which the scan handler
+	 * is not attached to this device object yet is when the device has
+	 * just appeared (either it wasn't present at all before or it was
+	 * removed and then added again).
+	 */
+	if (adev->handler) {
+		dev_dbg(&adev->dev, "Already enumerated\n");
+		return 0;
 	}
+	error = acpi_bus_scan(adev->handle);
+	if (error)
+		dev_warn(&adev->dev, "Namespace scan failure\n");
+
 	return error;
 }
 
@@ -359,11 +368,8 @@  static int acpi_scan_bus_check(struct ac
 	struct acpi_scan_handler *handler = adev->handler;
 	int error;
 
-	acpi_bus_get_status(adev);
-	if (!acpi_device_is_present(adev)) {
-		acpi_scan_device_not_enumerated(adev);
-		return 0;
-	}
+	acpi_scan_check_subtree(adev);
+
 	if (handler && handler->hotplug.scan_dependent)
 		return handler->hotplug.scan_dependent(adev);
 
@@ -2586,7 +2592,7 @@  EXPORT_SYMBOL(acpi_bus_scan);
  */
 void acpi_bus_trim(struct acpi_device *adev)
 {
-	acpi_bus_trim_one(adev, NULL);
+	acpi_scan_check_and_detach(adev, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);