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/[email protected]/
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!
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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.
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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);
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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);
}
On Mon, Feb 26, 2024 at 5:47 PM Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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.
>
>
>
On Mon, 26 Feb 2024 17:45:11 +0100
"Rafael J. Wysocki" <[email protected]> wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
Looks good.
Reviewed-by: Jonathan Cameron <[email protected]>