Hi,
On 12/5/20 4:44 PM, Rafael J. Wysocki wrote:
> On Thursday, December 3, 2020 3:27:27 PM CET Rafael J. Wysocki wrote:
>> On Thu, Dec 3, 2020 at 10:53 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> [cut]
>
>>> That indeed is not necessary if you take the entire set and always enable the
>>> new behavior instead of using the module option. I guess we could go that route
>>> for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
>>> testing.
>
> I'd prefer to do the whole thing in 5.11-rc and possibly revert something if it
> causes problems to occur.
Ok, that is you call to make, so that is fine with me.
>>> Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
>>> thing and the module option and simply always uses the new behavior?
>>
>> No, something else. Stay tuned.
>
> The patch below illustrates what I'd like to do. It at least doesn't kill my
> test-bed system, but also it doesn't cause the enumeration ordering to change
> on that system.
>
> It really is three patches in one and (again) I borrowed some code from your
> patches in the $subject series.
Borrowing some of my code is fine, no worries about that. I'm happy as some
fix for this gets upstream in some form :)
> [It is on top of the "ACPI: scan: Add PNP0D80
> to the _DEP exceptions list" patch I've just posted.]
>
>
> Please let me know what you think.
I think this should work fine. I've 2 small remarks inline below, but nothing
big / important.
My list of kernel things to look into is longer then my available time
(something which I assume you are familiar with), so for now I've only taken
a good look at your proposed solution and not actually tested it.
Let me know if you want me to give this a spin on the device with the _HID
issue as is, or if you have something closer to a final version ready
which you want me to test.
> ---
> drivers/acpi/scan.c | 141 ++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 120 insertions(+), 21 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1646,17 +1646,77 @@ void acpi_device_add_finalize(struct acp
> kobject_uevent(&device->dev.kobj, KOBJ_ADD);
> }
>
> -static int acpi_add_single_object(struct acpi_device **child,
> - acpi_handle handle, int type,
> - unsigned long long sta)
> +/*
> + * List of IDs for which we defer enumeration them to the second pass, because
> + * some of their methods used during addition depend on OpRegions registered by
> + * the drivers for other ACPI devices.
> + */
> +static const char * const acpi_defer_enumeration_ids[] = {
> + "BCM2E5B", /* Acer SW3-016 bluetooth HID vs GPIO OpRegs */
> + NULL
> +};
Note that since you defer if there are unmet _DEP-s, this won't be necessary:
This list was purely there as a safer way to select devices which to defer,
the kernel cmdline option in my patch-set would switch between either using
this list, or checking _DEP. Since we are going to fully go for using _DEP
now, this can be dropped.
> +
> +static bool acpi_should_defer_enumeration(acpi_handle handle,
> + struct acpi_device_info *info)
> +{
> + struct acpi_handle_list dep_devices;
> + acpi_status status;
> + int i;
> +
> + if (acpi_info_matches_ids(info, acpi_defer_enumeration_ids))
> + return true;
> +
> + /*
> + * We check for _HID here to avoid deferring the enumeration of:
> + * 1. PCI devices
> + * 2. ACPI nodes describing USB ports
> + * However, checking for _HID catches more then just these cases ...
> + */
> + if (!(info->valid & ACPI_VALID_HID))
> + return false;
Interesting approach (using _HID), I went with _ADR since _ADR indicates
(more or less) that the ACPI fwnode is a companion node for a device
which will be enumerated through other means (such as PCI devices), rather
then one where the ACPI code will instantiate a platform_device, or
i2c_client (etc) for it.
Maybe if we want to play it safe check both, and if there either is no
HID, or there is an ADR do not defer ? Note just thinking out loud here,
I'm fine with either approach.
> +
> + status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
> + if (ACPI_FAILURE(status))
> + return false;
> +
> + for (i = 0; i < dep_devices.count; i++) {
> + struct acpi_device_info *dep_info;
> + bool ignore;
> +
> + status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
> + if (ACPI_FAILURE(status))
> + continue;
> +
> + ignore = acpi_info_matches_ids(dep_info, acpi_ignore_dep_ids);
> + kfree(dep_info);
> +
> + if (ignore)
> + continue;
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int __acpi_add_single_object(struct acpi_device **child,
> + acpi_handle handle, int type,
> + unsigned long long sta, bool check_dep)
> {
> int result;
> struct acpi_device *device;
> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> struct acpi_device_info *info = NULL;
>
> - if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
> + if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE) {
> acpi_get_object_info(handle, &info);
> + if (check_dep && info &&
> + acpi_should_defer_enumeration(handle, info)) {
> + kfree(info);
> + acpi_handle_info(handle, "Missing dependencies\n");
> + return -EAGAIN;
> + }
> + }
>
> device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> if (!device) {
> @@ -1696,6 +1756,13 @@ static int acpi_add_single_object(struct
> return 0;
> }
>
> +static int acpi_add_single_object(struct acpi_device **child,
> + acpi_handle handle, int type,
> + unsigned long long sta)
> +{
> + return __acpi_add_single_object(child, handle, type, sta, false);
> +}
> +
> static acpi_status acpi_get_resource_memory(struct acpi_resource *ares,
> void *context)
> {
> @@ -1892,8 +1959,8 @@ static void acpi_device_dep_initialize(s
> }
> }
>
> -static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
> - void *not_used, void **return_value)
> +static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
> + struct acpi_device **adev_p)
> {
> struct acpi_device *device = NULL;
> int type;
> @@ -1913,7 +1980,7 @@ static acpi_status acpi_bus_check_add(ac
> return AE_OK;
> }
>
> - acpi_add_single_object(&device, handle, type, sta);
> + __acpi_add_single_object(&device, handle, type, sta, check_dep);
> if (!device)
> return AE_CTRL_DEPTH;
>
> @@ -1921,12 +1988,24 @@ static acpi_status acpi_bus_check_add(ac
> acpi_device_dep_initialize(device);
>
> out:
> - if (!*return_value)
> - *return_value = device;
> + if (!*adev_p)
> + *adev_p = device;
>
> return AE_OK;
> }
>
> +static acpi_status acpi_bus_check_add_1(acpi_handle handle, u32 lvl_not_used,
> + void *not_used, void **ret_p)
> +{
> + return acpi_bus_check_add(handle, true, (struct acpi_device **)ret_p);
> +}
> +
> +static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
> + void *not_used, void **ret_p)
> +{
> + return acpi_bus_check_add(handle, false, (struct acpi_device **)ret_p);
> +}
> +
> static void acpi_default_enumeration(struct acpi_device *device)
> {
> /*
> @@ -1994,12 +2073,16 @@ static int acpi_scan_attach_handler(stru
> return ret;
> }
>
> -static void acpi_bus_attach(struct acpi_device *device)
> +static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
> {
> struct acpi_device *child;
> + bool skip = !first_pass && device->flags.visited;
> acpi_handle ejd;
> int ret;
>
> + if (skip)
> + goto ok;
> +
> if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
> register_dock_dependent_device(device, ejd);
>
> @@ -2046,9 +2129,9 @@ static void acpi_bus_attach(struct acpi_
>
> ok:
> list_for_each_entry(child, &device->children, node)
> - acpi_bus_attach(child);
> + acpi_bus_attach(child, first_pass);
>
> - if (device->handler && device->handler->hotplug.notify_online)
> + if (!skip && device->handler && device->handler->hotplug.notify_online)
> device->handler->hotplug.notify_online(device);
> }
>
> @@ -2066,7 +2149,8 @@ void acpi_walk_dep_device_list(acpi_hand
>
> adev->dep_unmet--;
> if (!adev->dep_unmet)
> - acpi_bus_attach(adev);
> + acpi_bus_attach(adev, true);
> +
> list_del(&dep->node);
> kfree(dep);
> }
> @@ -2091,17 +2175,32 @@ EXPORT_SYMBOL_GPL(acpi_walk_dep_device_l
> */
> int acpi_bus_scan(acpi_handle handle)
> {
> - void *device = NULL;
> + struct acpi_device *device = NULL;
> +
> + /* Pass 1: Avoid enumerating devices with missing dependencies. */
>
> - if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
> + if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
> acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> - acpi_bus_check_add, NULL, NULL, &device);
> + acpi_bus_check_add_1, NULL, NULL,
> + (void **)&device);
>
> - if (device) {
> - acpi_bus_attach(device);
> - return 0;
> - }
> - return -ENODEV;
> + if (!device)
> + return -ENODEV;
> +
> + acpi_bus_attach(device, true);
> +
> + /* Pass 2: Enumerate all of the remaining devices. */
> +
> + device = NULL;
> +
> + if (ACPI_SUCCESS(acpi_bus_check_add(handle, false, &device)))
> + acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> + acpi_bus_check_add_2, NULL, NULL,
> + (void **)&device);
> +
> + acpi_bus_attach(device, false);
> +
> + return 0;
> }
> EXPORT_SYMBOL(acpi_bus_scan);
Regards,
Hans
