[PATCH v2 0/5] ACPI: scan: Check enabled _STA bit on Bus/Device Checks

[PATCH v2 0/5] ACPI: scan: Check enabled _STA bit on Bus/Device Checks

[Rafael J. Wysocki]

Hi,

This is an update of the patch series at

https://lore.kernel.org/linux-acpi/4562925.LvFx2qVVIh@kreacher/

which mostly only rearranges the code and makes one functional
change in acpi_scan_devie_check().

The original series description is mostly still applicable:

"This series adds _STA enabled bit checking for processors (in all cases)
 and for all devices in the Bus Check and Device Check notification handling
 paths.  It does so without any side-effects on non-processor devices, unlike

 https://lore.kernel.org/linux-acpi/E1rVDmP-0027YJ-EW@rmk-PC.armlinux.org.uk/

 The first patch fixes an issue with failing Device Check notifications
 without a valid reason.

 The next two patches together make Bus Check and Device Check notification
 handling take the enabled bit into account.

 The last patch modifies the processor scan handler to check the enabled bit
 it its add callback.

 I would appreciate testing this on a system where the enabled bit really
 matters."

Thanks!

[PATCH v2 1/5] ACPI: scan: Fix device check notification handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is generally invalid to fail a Device Check notification if the scan
handler has not been attached to the given device after a bus rescan,
because there may be valid reasons for the scan handler to refuse
attaching to the device (for example, the device is not ready).

For this reason, modify acpi_scan_device_check() to return 0 in that
case without printing a warning.

While at it, reduce the log level of the "already enumerated" message
in the same function, because it is only interesting when debugging
notification handling

Fixes: 443fc8202272 ("ACPI / hotplug: Rework generic code to handle suprise removals")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Add R-by from Jonathan.

---
 drivers/acpi/scan.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -314,18 +314,14 @@ static int acpi_scan_device_check(struct
 		 * again).
 		 */
 		if (adev->handler) {
-			dev_warn(&adev->dev, "Already enumerated\n");
-			return -EALREADY;
+			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;
 		}
-		if (!adev->handler) {
-			dev_warn(&adev->dev, "Enumeration failure\n");
-			error = -ENODEV;
-		}
 	} else {
 		error = acpi_scan_device_not_enumerated(adev);
 	}

[PATCH v2 2/5] ACPI: scan: Relocate acpi_bus_trim_one()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Relocate acpi_bus_trim_one() (without modifications) so as to avoid the
need to add a forward declaration of it in a subsequent patch.

No functional changes.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Add R-by from Jonathan.

---
 drivers/acpi/scan.c |   52 ++++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -244,6 +244,32 @@ static int acpi_scan_try_to_offline(stru
 	return 0;
 }
 
+static int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
+{
+	struct acpi_scan_handler *handler = adev->handler;
+
+	acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
+
+	adev->flags.match_driver = false;
+	if (handler) {
+		if (handler->detach)
+			handler->detach(adev);
+
+		adev->handler = NULL;
+	} else {
+		device_release_driver(&adev->dev);
+	}
+	/*
+	 * Most likely, the device is going away, so put it into D3cold before
+	 * that.
+	 */
+	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
+	adev->flags.initialized = false;
+	acpi_device_clear_enumerated(adev);
+
+	return 0;
+}
+
 static int acpi_scan_hot_remove(struct acpi_device *device)
 {
 	acpi_handle handle = device->handle;
@@ -2547,32 +2573,6 @@ int acpi_bus_scan(acpi_handle handle)
 }
 EXPORT_SYMBOL(acpi_bus_scan);
 
-static int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
-{
-	struct acpi_scan_handler *handler = adev->handler;
-
-	acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
-
-	adev->flags.match_driver = false;
-	if (handler) {
-		if (handler->detach)
-			handler->detach(adev);
-
-		adev->handler = NULL;
-	} else {
-		device_release_driver(&adev->dev);
-	}
-	/*
-	 * Most likely, the device is going away, so put it into D3cold before
-	 * that.
-	 */
-	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
-	adev->flags.initialized = false;
-	acpi_device_clear_enumerated(adev);
-
-	return 0;
-}
-
 /**
  * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
  * @adev: Root of the ACPI namespace scope to walk.

[PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the device enabled bit

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify acpi_processor_add() return an error if _STA returns the enabled
bit clear for the given processor device, so as to avoid using processors
that don't decode their resources, as per the ACPI specification. [1]

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:
   * Move acpi_device_is_enabled() to this patch.
   * Change patch ordering.
   * Do not check the "functional" _STA bit in acpi_device_is_present().

---
 drivers/acpi/acpi_processor.c |    3 +++
 drivers/acpi/internal.h       |    1 +
 drivers/acpi/scan.c           |    5 +++++
 3 files changed, 9 insertions(+)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -121,6 +121,7 @@ int acpi_device_setup_files(struct acpi_
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
+bool acpi_device_is_enabled(const struct acpi_device *adev);
 bool acpi_device_is_present(const struct acpi_device *adev);
 bool acpi_device_is_battery(struct acpi_device *adev);
 bool acpi_device_is_first_physical_node(struct acpi_device *adev,
Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_processor.c
+++ linux-pm/drivers/acpi/acpi_processor.c
@@ -381,6 +381,9 @@ static int acpi_processor_add(struct acp
 	struct device *dev;
 	int result = 0;
 
+	if (!acpi_device_is_enabled(device))
+		return -ENODEV;
+
 	pr = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
 	if (!pr)
 		return -ENOMEM;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1945,6 +1945,11 @@ bool acpi_device_is_present(const struct
 	return adev->status.present || adev->status.functional;
 }
 
+bool acpi_device_is_enabled(const struct acpi_device *adev)
+{
+	return adev->status.present && adev->status.enabled;
+}
+
 static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
 				       const char *idstr,
 				       const struct acpi_device_id **matchid)

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

[Rafael J. Wysocki]

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(-)

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);
 

[PATCH v2 5/5] ACPI: scan: Consolidate Device Check and Bus Check notification handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is no particular reason why device object subtree rescans in
acpi_scan_device_check() and acpi_scan_device_check() should be carried
out differently, so move the rescan code into a new function called
acpi_scan_rescan_bus() and make both the functions above invoke it.

While at it, in the Device Check case, start the device object subtree
rescan mentioned above from the target device's parent, as per the
specification. [1]

Link: https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#device-object-notification-values # [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * New patch (split off patch [3/4] from v1).

---
 drivers/acpi/scan.c |   44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -336,9 +336,25 @@ static int acpi_scan_hot_remove(struct a
 	return 0;
 }
 
+static int acpi_scan_rescan_bus(struct acpi_device *adev)
+{
+	struct acpi_scan_handler *handler = adev->handler;
+	int ret;
+
+	if (handler && handler->hotplug.scan_dependent)
+		ret = handler->hotplug.scan_dependent(adev);
+	else
+		ret = acpi_bus_scan(adev->handle);
+
+	if (ret)
+		dev_info(&adev->dev, "Namespace scan failure\n");
+
+	return ret;
+}
+
 static int acpi_scan_device_check(struct acpi_device *adev)
 {
-	int error;
+	struct acpi_device *parent;
 
 	acpi_scan_check_subtree(adev);
 
@@ -356,36 +372,26 @@ static int acpi_scan_device_check(struct
 		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;
+	parent = acpi_dev_parent(adev);
+	if (!parent)
+		parent = adev;
+
+	return acpi_scan_rescan_bus(parent);
 }
 
-static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
+static int acpi_scan_bus_check(struct acpi_device *adev)
 {
-	struct acpi_scan_handler *handler = adev->handler;
-	int error;
-
 	acpi_scan_check_subtree(adev);
 
-	if (handler && handler->hotplug.scan_dependent)
-		return handler->hotplug.scan_dependent(adev);
-
-	error = acpi_bus_scan(adev->handle);
-	if (error) {
-		dev_warn(&adev->dev, "Namespace scan failure\n");
-		return error;
-	}
-	return acpi_dev_for_each_child(adev, acpi_scan_bus_check, NULL);
+	return acpi_scan_rescan_bus(adev);
 }
 
 static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
 {
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
-		return acpi_scan_bus_check(adev, NULL);
+		return acpi_scan_bus_check(adev);
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		return acpi_scan_device_check(adev);
 	case ACPI_NOTIFY_EJECT_REQUEST:

Re: [PATCH v2 2/5] ACPI: scan: Relocate acpi_bus_trim_one()

[Rafael J. Wysocki]

On Mon, Feb 26, 2024 at 5:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Relocate acpi_bus_trim_one() (without modifications) so as to avoid the
> need to add a forward declaration of it in a subsequent patch.
>
> No functional changes.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> v1 -> v2: Add R-by from Jonathan.

I actually forgot to add the R-bys, but let's assume they are here (no
need to look at the first two patches again).

> ---
>  drivers/acpi/scan.c |   52 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -244,6 +244,32 @@ static int acpi_scan_try_to_offline(stru
>         return 0;
>  }
>
> +static int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
> +{
> +       struct acpi_scan_handler *handler = adev->handler;
> +
> +       acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
> +
> +       adev->flags.match_driver = false;
> +       if (handler) {
> +               if (handler->detach)
> +                       handler->detach(adev);
> +
> +               adev->handler = NULL;
> +       } else {
> +               device_release_driver(&adev->dev);
> +       }
> +       /*
> +        * Most likely, the device is going away, so put it into D3cold before
> +        * that.
> +        */
> +       acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
> +       adev->flags.initialized = false;
> +       acpi_device_clear_enumerated(adev);
> +
> +       return 0;
> +}
> +
>  static int acpi_scan_hot_remove(struct acpi_device *device)
>  {
>         acpi_handle handle = device->handle;
> @@ -2547,32 +2573,6 @@ int acpi_bus_scan(acpi_handle handle)
>  }
>  EXPORT_SYMBOL(acpi_bus_scan);
>
> -static int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
> -{
> -       struct acpi_scan_handler *handler = adev->handler;
> -
> -       acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
> -
> -       adev->flags.match_driver = false;
> -       if (handler) {
> -               if (handler->detach)
> -                       handler->detach(adev);
> -
> -               adev->handler = NULL;
> -       } else {
> -               device_release_driver(&adev->dev);
> -       }
> -       /*
> -        * Most likely, the device is going away, so put it into D3cold before
> -        * that.
> -        */
> -       acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
> -       adev->flags.initialized = false;
> -       acpi_device_clear_enumerated(adev);
> -
> -       return 0;
> -}
> -
>  /**
>   * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
>   * @adev: Root of the ACPI namespace scope to walk.
>
>
>

Re: [PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the device enabled bit

[Jonathan Cameron]

On Mon, 26 Feb 2024 17:40:52 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify acpi_processor_add() return an error if _STA returns the enabled
> bit clear for the given processor device, so as to avoid using processors
> that don't decode their resources, as per the ACPI specification. [1]
> 
> 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>

Sorry for lack of reply on discussion. Your follow up mails never reached my
inbox for some reason so I just caught up on lore. I'll keep an eye on
the archives to make sure I don't miss further discussion.

Agreed that functional isn't relevant here so this patch is correct.
Also agree that it would be nice to clarify the spec as you mentioned
to say that bit 1 is reserved if bit 0 of _STA result is clear.
Depending on interpretation it's either a clarification or a relaxation
of current statements, so should be uncontroversial (famous last words ;)

+CC kangkang so this is on his radar as an ACPI cleanup suggestion.
For his reference, discussion is here:
https://lore.kernel.org/linux-acpi/CAJZ5v0jjD=KN0pOuWZZ8DT5yHdu03KgOSHYe3wB7h2vafNa44w@mail.gmail.com/

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> 
> v1 -> v2:
>    * Move acpi_device_is_enabled() to this patch.
>    * Change patch ordering.
>    * Do not check the "functional" _STA bit in acpi_device_is_present().
> 
> ---
>  drivers/acpi/acpi_processor.c |    3 +++
>  drivers/acpi/internal.h       |    1 +
>  drivers/acpi/scan.c           |    5 +++++
>  3 files changed, 9 insertions(+)
> 
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -121,6 +121,7 @@ int acpi_device_setup_files(struct acpi_
>  void acpi_device_remove_files(struct acpi_device *dev);
>  void acpi_device_add_finalize(struct acpi_device *device);
>  void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> +bool acpi_device_is_enabled(const struct acpi_device *adev);
>  bool acpi_device_is_present(const struct acpi_device *adev);
>  bool acpi_device_is_battery(struct acpi_device *adev);
>  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
> Index: linux-pm/drivers/acpi/acpi_processor.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_processor.c
> +++ linux-pm/drivers/acpi/acpi_processor.c
> @@ -381,6 +381,9 @@ static int acpi_processor_add(struct acp
>  	struct device *dev;
>  	int result = 0;
>  
> +	if (!acpi_device_is_enabled(device))
> +		return -ENODEV;
> +
>  	pr = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
>  	if (!pr)
>  		return -ENOMEM;
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1945,6 +1945,11 @@ bool acpi_device_is_present(const struct
>  	return adev->status.present || adev->status.functional;
>  }
>  
> +bool acpi_device_is_enabled(const struct acpi_device *adev)
> +{
> +	return adev->status.present && adev->status.enabled;
> +}
> +
>  static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
>  				       const char *idstr,
>  				       const struct acpi_device_id **matchid)
> 
> 
> 

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

[Jonathan Cameron]

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>

Re: [PATCH v2 5/5] ACPI: scan: Consolidate Device Check and Bus Check notification handling

[Jonathan Cameron]

On Mon, 26 Feb 2024 17:46:41 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There is no particular reason why device object subtree rescans in
> acpi_scan_device_check() and acpi_scan_device_check() should be carried
> out differently, so move the rescan code into a new function called
> acpi_scan_rescan_bus() and make both the functions above invoke it.
> 
> While at it, in the Device Check case, start the device object subtree
> rescan mentioned above from the target device's parent, as per the
> specification. [1]
> 
> Link: https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#device-object-notification-values # [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Jonathan

Re: [PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the device enabled bit

[Rafael J. Wysocki]

On Tue, Feb 27, 2024 at 10:28 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 26 Feb 2024 17:40:52 +0100
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Modify acpi_processor_add() return an error if _STA returns the enabled
> > bit clear for the given processor device, so as to avoid using processors
> > that don't decode their resources, as per the ACPI specification. [1]
> >
> > 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>
>
> Sorry for lack of reply on discussion.

No worries.

> Your follow up mails never reached my inbox for some reason

/me blames spam filters somewhere.

> so I just caught up on lore. I'll keep an eye on
> the archives to make sure I don't miss further discussion.

Thanks!

> Agreed that functional isn't relevant here so this patch is correct.
> Also agree that it would be nice to clarify the spec as you mentioned
> to say that bit 1 is reserved if bit 0 of _STA result is clear.
> Depending on interpretation it's either a clarification or a relaxation
> of current statements, so should be uncontroversial (famous last words ;)

Right.

> +CC kangkang so this is on his radar as an ACPI cleanup suggestion.
> For his reference, discussion is here:
> https://lore.kernel.org/linux-acpi/CAJZ5v0jjD=KN0pOuWZZ8DT5yHdu03KgOSHYe3wB7h2vafNa44w@mail.gmail.com/
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks for all of the reviews!