* [PATCH v1 0/4] ACPI: scan: Check enabled _STA bit on Bus/Device Checks @ 2024-02-21 19:59 Rafael J. Wysocki 2024-02-21 20:01 ` [PATCH v1 1/4] ACPI: scan: Fix device check notification handling Rafael J. Wysocki ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-21 19:59 UTC (permalink / raw) To: Linux ACPI, Jonathan Cameron Cc: LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) Hi, 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! ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 1/4] ACPI: scan: Fix device check notification handling 2024-02-21 19:59 [PATCH v1 0/4] ACPI: scan: Check enabled _STA bit on Bus/Device Checks Rafael J. Wysocki @ 2024-02-21 20:01 ` Rafael J. Wysocki 2024-02-22 18:50 ` Jonathan Cameron 2024-02-21 20:01 ` [PATCH v1 2/4] ACPI: scan: Relocate acpi_bus_trim_one() Rafael J. Wysocki ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-21 20:01 UTC (permalink / raw) To: Linux ACPI, Jonathan Cameron Cc: LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) 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> --- 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); } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/4] ACPI: scan: Fix device check notification handling 2024-02-21 20:01 ` [PATCH v1 1/4] ACPI: scan: Fix device check notification handling Rafael J. Wysocki @ 2024-02-22 18:50 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2024-02-22 18:50 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux ACPI, LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) On Wed, 21 Feb 2024 21:01:02 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > 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> Seems reasonable to me. Not sure it fixes any bugs anyone has seen in the wild though. I'd not give it a fixes tag without such a known case, but your subsystem so fair enough! Thanks for resolving how to handle the processor case. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > 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); > } > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 2/4] ACPI: scan: Relocate acpi_bus_trim_one() 2024-02-21 19:59 [PATCH v1 0/4] ACPI: scan: Check enabled _STA bit on Bus/Device Checks Rafael J. Wysocki 2024-02-21 20:01 ` [PATCH v1 1/4] ACPI: scan: Fix device check notification handling Rafael J. Wysocki @ 2024-02-21 20:01 ` Rafael J. Wysocki 2024-02-22 17:09 ` Jonathan Cameron 2024-02-21 20:02 ` [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling Rafael J. Wysocki 2024-02-21 20:03 ` [PATCH v1 4/4] ACPI: scan: Make acpi_processor_add() check the device enabled bit Rafael J. Wysocki 3 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-21 20:01 UTC (permalink / raw) To: Linux ACPI, Jonathan Cameron Cc: LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) 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> --- 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/4] ACPI: scan: Relocate acpi_bus_trim_one() 2024-02-21 20:01 ` [PATCH v1 2/4] ACPI: scan: Relocate acpi_bus_trim_one() Rafael J. Wysocki @ 2024-02-22 17:09 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2024-02-22 17:09 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux ACPI, LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) On Wed, 21 Feb 2024 21:01:48 +0100 "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> FWIW you indeed moved the code :) Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > 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. > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling 2024-02-21 19:59 [PATCH v1 0/4] ACPI: scan: Check enabled _STA bit on Bus/Device Checks Rafael J. Wysocki 2024-02-21 20:01 ` [PATCH v1 1/4] ACPI: scan: Fix device check notification handling Rafael J. Wysocki 2024-02-21 20:01 ` [PATCH v1 2/4] ACPI: scan: Relocate acpi_bus_trim_one() Rafael J. Wysocki @ 2024-02-21 20:02 ` Rafael J. Wysocki 2024-02-22 18:28 ` Jonathan Cameron 2024-02-21 20:03 ` [PATCH v1 4/4] ACPI: scan: Make acpi_processor_add() check the device enabled bit Rafael J. Wysocki 3 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-21 20:02 UTC (permalink / raw) To: Linux ACPI, Jonathan Cameron Cc: LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) 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. Make acpi_bus_trim_one() check device status if its second argument is not NULL, in which case it will only 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_bus_trim_one() 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. 3. Make acpi_scan_device_check() skip the bus rescan if _STA for the target device return the enabled bit clear, which is allowed by the ACPI specification in the Device Check case. [2] In addition to the above: 4. Make sure that the bus rescan that needs to be triggered in the case when a new device has appeared is carried out in the same way in both the Device Check and Bus Check cases. 5. In the Device Check case, start the bus rescan mentioned above from the target device's parent, as per the specification. [2] Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1] Link: https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#device-object-notification-values # [2] Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/internal.h | 1 drivers/acpi/scan.c | 109 +++++++++++++++++++++++++++--------------------- 2 files changed, 64 insertions(+), 46 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_bus_trim_one(struct acpi_device *adev, void *check_status) { 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_bus_trim_one, check_status); + + if (check_status) { + 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) { @@ -315,71 +331,67 @@ static int acpi_scan_hot_remove(struct a return 0; } -static int acpi_scan_device_not_enumerated(struct acpi_device *adev) +static void acpi_scan_check_gone(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; + acpi_bus_trim_one(adev, (void *)true); } -static int acpi_scan_device_check(struct acpi_device *adev) +static int acpi_scan_rescan_bus(struct acpi_device *adev) { + struct acpi_scan_handler *handler = adev->handler; 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; - } + if (handler && handler->hotplug.scan_dependent) + error = handler->hotplug.scan_dependent(adev); + else 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); - } + + if (error) + dev_info(&adev->dev, "Namespace scan failure\n"); + return error; } -static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used) +static int acpi_scan_device_check(struct acpi_device *adev) { - struct acpi_scan_handler *handler = adev->handler; - int error; + struct acpi_device *parent; - acpi_bus_get_status(adev); - if (!acpi_device_is_present(adev)) { - acpi_scan_device_not_enumerated(adev); + acpi_scan_check_gone(adev); + + if (!acpi_device_is_enabled(adev)) return 0; - } - 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; + /* + * 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 or enabled at all before or + * it was removed and then added again). + */ + if (adev->handler) { + dev_dbg(&adev->dev, "Already enumerated\n"); + return 0; } - return acpi_dev_for_each_child(adev, acpi_scan_bus_check, NULL); + + 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) +{ + acpi_scan_check_gone(adev); + + 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: @@ -1945,6 +1957,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 acpi_device_is_present(adev) && adev->status.enabled; +} + static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler, const char *idstr, const struct acpi_device_id **matchid) 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, ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling 2024-02-21 20:02 ` [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling Rafael J. Wysocki @ 2024-02-22 18:28 ` Jonathan Cameron 2024-02-26 15:37 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2024-02-22 18:28 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux ACPI, LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) On Wed, 21 Feb 2024 21:02:33 +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. Hi Rafael, I rebased the vCPU HP series Russell was working to go on top of this and give me a basis to check the flows through your new conditions. It may have it's own issues, but at least it makes use of some of these bits and related checks. For now the only key thing is make sure we don't check enabled() in the hot remove path (until after _EJ0). That happens correctly because acpi_device_trim() is called and that doesn't have check_status set. The naming however doesn't make it obvious that path elides the check, so I wonder if that call in acpi_scan_hotremove() should be replaced with acpi_bus_trim_one(, NULL); > > 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. Make acpi_bus_trim_one() check device status if its second argument > is not NULL, in which case it will only 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_bus_trim_one() 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. > > 3. Make acpi_scan_device_check() skip the bus rescan if _STA for the > target device return the enabled bit clear, which is allowed by the > ACPI specification in the Device Check case. [2] > > In addition to the above: > > 4. Make sure that the bus rescan that needs to be triggered in the case > when a new device has appeared is carried out in the same way in > both the Device Check and Bus Check cases. > > 5. In the Device Check case, start the bus rescan mentioned above from > the target device's parent, as per the specification. [2] This feels like an 'and' kind of a patch. Can we split it up so refactors are done first and the _STA check stuff in a follow up patch? End result is good, just could possibly be easier to review in two hops. > > Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1] > Link: https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#device-object-notification-values # [2] > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Diff had a field day here and generated an somewhat unreadable patch. A few other comments inline, but superficial stuff. The handling looks correct to me. > --- > drivers/acpi/internal.h | 1 > drivers/acpi/scan.c | 109 +++++++++++++++++++++++++++--------------------- > 2 files changed, 64 insertions(+), 46 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_bus_trim_one(struct acpi_device *adev, void *check_status) Bool as pointer is a bit hard to read particularly as you use (void *)true for true, but not (void *)false for false. However it's not too bad. My current version of the vCPU patches needs to pass more data in here anyway (as there a few things we need to not do on eject, that don't correspond to !check_status) so I have this as a struct anyway after a rebase. > { > 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_bus_trim_one, check_status); > + > + if (check_status) { > + 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) { > @@ -315,71 +331,67 @@ static int acpi_scan_hot_remove(struct a > return 0; > } > > -static int acpi_scan_device_not_enumerated(struct acpi_device *adev) > +static void acpi_scan_check_gone(struct acpi_device *adev) The name of this had me initially a little confused. Maybe acpi_bus_trim_if_gone() Or maybe just drop this wrapper entirely as it doesn't save much and naming it clearly is hard. > { > - if (!acpi_device_enumerated(adev)) { > - dev_warn(&adev->dev, "Still not enumerated\n"); > - return -EALREADY; > - } > - acpi_bus_trim(adev); > - return 0; > + acpi_bus_trim_one(adev, (void *)true); > } > 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: > @@ -1945,6 +1957,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 acpi_device_is_present(adev) && adev->status.enabled; This resolves as (present or functional) && enabled. By my reading you are not allowed functional && enabled, but not present. Line one of the description says. "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)." I don't much care about that though (I think we discussed this in Russel's earlier series) > +} > + > static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler, > const char *idstr, > const struct acpi_device_id **matchid) > 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, > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling 2024-02-22 18:28 ` Jonathan Cameron @ 2024-02-26 15:37 ` Rafael J. Wysocki 2024-02-26 15:54 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-26 15:37 UTC (permalink / raw) To: Jonathan Cameron Cc: Rafael J. Wysocki, Linux ACPI, LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) On Thu, Feb 22, 2024 at 7:28 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Wed, 21 Feb 2024 21:02:33 +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. > > Hi Rafael, > > I rebased the vCPU HP series Russell was working to go on top of this > and give me a basis to check the flows through your new conditions. > It may have it's own issues, but at least it makes use of some of these > bits and related checks. > > For now the only key thing is make sure we don't check enabled() > in the hot remove path (until after _EJ0). That happens correctly > because acpi_device_trim() is called and that doesn't have check_status > set. The naming however doesn't make it obvious that path elides the > check, so I wonder if that call in acpi_scan_hotremove() > should be replaced with acpi_bus_trim_one(, NULL); Well, that's how acpi_bus_trim() is supposed to work: Detach everything under the target device (and including that device itself) unconditionally. I would prefer to rename acpi_bus_trim_one() to something closer reflecting its purpose. > > > > 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. Make acpi_bus_trim_one() check device status if its second argument > > is not NULL, in which case it will only 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_bus_trim_one() 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. > > > > 3. Make acpi_scan_device_check() skip the bus rescan if _STA for the > > target device return the enabled bit clear, which is allowed by the > > ACPI specification in the Device Check case. [2] > > > > In addition to the above: > > > > 4. Make sure that the bus rescan that needs to be triggered in the case > > when a new device has appeared is carried out in the same way in > > both the Device Check and Bus Check cases. > > > > 5. In the Device Check case, start the bus rescan mentioned above from > > the target device's parent, as per the specification. [2] > > This feels like an 'and' kind of a patch. Can we split it up so refactors > are done first and the _STA check stuff in a follow up patch? Sure, that will produce more readable patches. > End result is good, just could possibly be easier to review in two hops. Sure. > > > > Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1] > > Link: https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#device-object-notification-values # [2] > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Diff had a field day here and generated an somewhat unreadable patch. Well, agreed. > A few other comments inline, but superficial stuff. The handling looks > correct to me. > > > --- > > drivers/acpi/internal.h | 1 > > drivers/acpi/scan.c | 109 +++++++++++++++++++++++++++--------------------- > > 2 files changed, 64 insertions(+), 46 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_bus_trim_one(struct acpi_device *adev, void *check_status) > > Bool as pointer is a bit hard to read particularly as you use (void *)true > for true, but not (void *)false for false. > > However it's not too bad. My current version of the vCPU patches needs > to pass more data in here anyway (as there a few things we need to > not do on eject, that don't correspond to !check_status) > so I have this as a struct anyway after a rebase. The reason for using void * here is that this function is called recursively via acpi_dev_for_each_child_reverse() which requires a pointer as the second arg. I guess I could define a wrapper around it for this, but that would be more code without any functional difference. > > { > > 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_bus_trim_one, check_status); > > + > > + if (check_status) { > > + 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) { > > @@ -315,71 +331,67 @@ static int acpi_scan_hot_remove(struct a > > return 0; > > } > > > > -static int acpi_scan_device_not_enumerated(struct acpi_device *adev) > > +static void acpi_scan_check_gone(struct acpi_device *adev) > > The name of this had me initially a little confused. Maybe > acpi_bus_trim_if_gone() > > Or maybe just drop this wrapper entirely as it doesn't save much > and naming it clearly is hard. I'll try to make it somewhat better. > > > { > > - if (!acpi_device_enumerated(adev)) { > > - dev_warn(&adev->dev, "Still not enumerated\n"); > > - return -EALREADY; > > - } > > - acpi_bus_trim(adev); > > - return 0; > > + acpi_bus_trim_one(adev, (void *)true); > > } > > > > 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: > > @@ -1945,6 +1957,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 acpi_device_is_present(adev) && adev->status.enabled; > > This resolves as (present or functional) && enabled. > > By my reading you are not allowed functional && enabled, but not present. > Line one of the description says. > > "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)." > > I don't much care about that though (I think we discussed this > in Russel's earlier series) Functional and enabled, but not present would go against the spec. I guess the kernel could protect itself against this, but then whatever it chooses to do has not been defined anyway. The spec doesn't actually say what the OSPM is supposed to do when it sees that combination of bits. I'm inclined to clarify it to say "if bit [0] is cleared, bit [1] has no defined meaning and it should be ignored by the OSPM". To be consistent with this interpretation, acpi_device_is_enabled() should return "(present and enabled) or functional". I'll change it along these lines. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling 2024-02-26 15:37 ` Rafael J. Wysocki @ 2024-02-26 15:54 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-26 15:54 UTC (permalink / raw) To: Jonathan Cameron Cc: Rafael J. Wysocki, Linux ACPI, LKML, Mika Westerberg, Russell King (Oracle) On Mon, Feb 26, 2024 at 4:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Feb 22, 2024 at 7:28 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Wed, 21 Feb 2024 21:02:33 +0100 > > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: [...] > > > +bool acpi_device_is_enabled(const struct acpi_device *adev) > > > +{ > > > + return acpi_device_is_present(adev) && adev->status.enabled; > > > > This resolves as (present or functional) && enabled. > > > > By my reading you are not allowed functional && enabled, but not present. > > Line one of the description says. > > > > "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)." > > > > I don't much care about that though (I think we discussed this > > in Russel's earlier series) > > Functional and enabled, but not present would go against the spec. I > guess the kernel could protect itself against this, but then whatever > it chooses to do has not been defined anyway. > > The spec doesn't actually say what the OSPM is supposed to do when it > sees that combination of bits. I'm inclined to clarify it to say "if > bit [0] is cleared, bit [1] has no defined meaning and it should be > ignored by the OSPM". To be consistent with this interpretation, > acpi_device_is_enabled() should return "(present and enabled) or > functional". > > I'll change it along these lines. Actually, I don't think that the "functional" bit has any bearing on this. It only means that the OSPM should continue the enumeration below the device regardless of the present bit value. In the acpi_processor_add() case it is clearly irrelevant. acpi_scan_bus_check() needs to walk the entire subtree below the target device anyway. As for acpi_scan_device_check(), IMO it's better to make it walk the subtree below the device if it is present, but not enabled, for backwards compatibility. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 4/4] ACPI: scan: Make acpi_processor_add() check the device enabled bit 2024-02-21 19:59 [PATCH v1 0/4] ACPI: scan: Check enabled _STA bit on Bus/Device Checks Rafael J. Wysocki ` (2 preceding siblings ...) 2024-02-21 20:02 ` [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling Rafael J. Wysocki @ 2024-02-21 20:03 ` Rafael J. Wysocki 2024-02-22 18:47 ` Jonathan Cameron 3 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2024-02-21 20:03 UTC (permalink / raw) To: Linux ACPI, Jonathan Cameron Cc: LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) 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> --- drivers/acpi/acpi_processor.c | 3 +++ 1 file changed, 3 insertions(+) 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 4/4] ACPI: scan: Make acpi_processor_add() check the device enabled bit 2024-02-21 20:03 ` [PATCH v1 4/4] ACPI: scan: Make acpi_processor_add() check the device enabled bit Rafael J. Wysocki @ 2024-02-22 18:47 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2024-02-22 18:47 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux ACPI, LKML, Mika Westerberg, Rafael J. Wysocki, Russell King (Oracle) On Wed, 21 Feb 2024 21:03:17 +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> This does the job for us so if you are happier with this approach that works for me. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/acpi/acpi_processor.c | 3 +++ > 1 file changed, 3 insertions(+) > > 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; > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-02-26 15:54 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-21 19:59 [PATCH v1 0/4] ACPI: scan: Check enabled _STA bit on Bus/Device Checks Rafael J. Wysocki 2024-02-21 20:01 ` [PATCH v1 1/4] ACPI: scan: Fix device check notification handling Rafael J. Wysocki 2024-02-22 18:50 ` Jonathan Cameron 2024-02-21 20:01 ` [PATCH v1 2/4] ACPI: scan: Relocate acpi_bus_trim_one() Rafael J. Wysocki 2024-02-22 17:09 ` Jonathan Cameron 2024-02-21 20:02 ` [PATCH v1 3/4] ACPI: scan: Rework Device Check and Bus Check notification handling Rafael J. Wysocki 2024-02-22 18:28 ` Jonathan Cameron 2024-02-26 15:37 ` Rafael J. Wysocki 2024-02-26 15:54 ` Rafael J. Wysocki 2024-02-21 20:03 ` [PATCH v1 4/4] ACPI: scan: Make acpi_processor_add() check the device enabled bit Rafael J. Wysocki 2024-02-22 18:47 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).