Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede <hdegoede@redhat.com>]

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@redhat.com> 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