On Fri, 15 Dec 2023 20:47:31 +0100
"Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx> wrote:
> On Friday, December 15, 2023 5:15:39 PM CET Jonathan Cameron wrote:
> > On Fri, 15 Dec 2023 15:31:55 +0000
> > "Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> wrote:
> >
> > > On Thu, Dec 14, 2023 at 07:37:10PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Dec 14, 2023 at 7:16 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, Dec 14, 2023 at 7:10 PM Russell King (Oracle)
> > > > > <linux@xxxxxxxxxxxxxxx> wrote:
> > > > > > I guess we need something like:
> > > > > >
> > > > > > if (device->status.present)
> > > > > > return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
> > > > > > device->status.enabled;
> > > > > > else
> > > > > > return device->status.functional;
> > > > > >
> > > > > > so we only check device->status.enabled for processor-type devices?
> > > > >
> > > > > Yes, something like this.
> > > >
> > > > However, that is not sufficient, because there are
> > > > ACPI_BUS_TYPE_DEVICE devices representing processors.
> > > >
> > > > I'm not sure about a clean way to do it ATM.
> > >
> > > Ok, how about:
> > >
> > > static bool acpi_dev_is_processor(const struct acpi_device *device)
> > > {
> > > struct acpi_hardware_id *hwid;
> > >
> > > if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
> > > return true;
> > >
> > > if (device->device_type != ACPI_BUS_TYPE_DEVICE)
> > > return false;
> > >
> > > list_for_each_entry(hwid, &device->pnp.ids, list)
> > > if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
> > > !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
> > > return true;
> > >
> > > return false;
> > > }
> > >
> > > and then:
> > >
> > > if (device->status.present)
> > > return !acpi_dev_is_processor(device) || device->status.enabled;
> > > else
> > > return device->status.functional;
> > >
> > > ?
> > >
> > Changing it to CPU only for now makes sense to me and I think this code snippet should do the
> > job. Nice and simple.
>
> Well, except that it does checks that are done elsewhere slightly
> differently, which from the maintenance POV is not nice.
>
> Maybe something like the appended patch (untested).
Hi Rafael,
As far as I can see that's functionally equivalent, so looks good to me.
I'm not set up to test this today though, so will defer to Russell on whether
there is anything missing
Thanks for putting this together.
Jonathan
>
> ---
> drivers/acpi/acpi_processor.c | 11 +++++++++++
> drivers/acpi/internal.h | 3 +++
> drivers/acpi/scan.c | 24 +++++++++++++++++++++++-
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/acpi/acpi_processor.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_processor.c
> +++ linux-pm/drivers/acpi/acpi_processor.c
> @@ -644,6 +644,17 @@ static struct acpi_scan_handler processo
> },
> };
>
> +bool acpi_device_is_processor(const struct acpi_device *adev)
> +{
> + if (adev->device_type == ACPI_BUS_TYPE_PROCESSOR)
> + return true;
> +
> + if (adev->device_type != ACPI_BUS_TYPE_DEVICE)
> + return false;
> +
> + return acpi_scan_check_handler(adev, &processor_handler);
> +}
> +
> static int acpi_processor_container_attach(struct acpi_device *dev,
> const struct acpi_device_id *id)
> {
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -62,6 +62,8 @@ void acpi_sysfs_add_hotplug_profile(stru
> int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
> const char *hotplug_profile_name);
> void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
> +bool acpi_scan_check_handler(const struct acpi_device *adev,
> + struct acpi_scan_handler *handler);
>
> #ifdef CONFIG_DEBUG_FS
> extern struct dentry *acpi_debugfs_dir;
> @@ -133,6 +135,7 @@ int acpi_bus_register_early_device(int t
> const struct acpi_device *acpi_companion_match(const struct device *dev);
> int __acpi_device_uevent_modalias(const struct acpi_device *adev,
> struct kobj_uevent_env *env);
> +bool acpi_device_is_processor(const struct acpi_device *adev);
>
> /* --------------------------------------------------------------------------
> Power Resource
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1938,6 +1938,19 @@ static bool acpi_scan_handler_matching(s
> return false;
> }
>
> +bool acpi_scan_check_handler(const struct acpi_device *adev,
> + struct acpi_scan_handler *handler)
> +{
> + struct acpi_hardware_id *hwid;
> +
> + list_for_each_entry(hwid, &adev->pnp.ids, list) {
> + if (acpi_scan_handler_matching(handler, hwid->id, NULL))
> + return true;
> + }
> +
> + return false;
> +}
> +
> static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr,
> const struct acpi_device_id **matchid)
> {
> @@ -2410,7 +2423,16 @@ bool acpi_dev_ready_for_enumeration(cons
> if (device->flags.honor_deps && device->dep_unmet)
> return false;
>
> - return acpi_device_is_present(device);
> + if (device->status.functional)
> + return true;
> +
> + if (!device->status.present)
> + return false;
> +
> + if (device->status.enabled)
> + return true; /* Fast path. */
> +
> + return !acpi_device_is_processor(device);
> }
> EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>
>
>
>
|