Re: [PATCH v2 4/6] ACPI: scan: Add ACPI device enumerated marker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message establishes a strict structural invariant: 'The rule regarding flags.visited and the "enumerated" field is that the latter cannot be set until the former has been set and they need to be cleared in reverse order.' However, the implementation violates this rule in multiple paths. For early and fixed devices, `acpi_bus_register_early_device()` and `acpi_bus_add_fixed_device_object()` call `acpi_default_enumeration()`, which sets `enumerated = true`, but their `flags.visited` is never set to 1. Additionally, for generic devices, `acpi_scan_attach_handler()` calls `acpi_generic_device_attach()`, which triggers `acpi_default_enumeration()`, setting `enumerated = true` before `acpi_bus_attach()` sets `device->flags.visited = 1`.
--

commit 193e84bf6b51af7db08c641525537f0a2b63b3c2
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI: scan: Add ACPI device enumerated marker

Currently, the "visited" flag of ACPI device objects has two roles.
One of them is to indicate that the given ACPI device object has been
enumerated, and the other is to indicate that acpi_bus_attach() has
processed the device. This commit adds a new "enumerated" bool field to
struct acpi_device to separate these purposes.

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 7208cc5158660..2e853add5e605 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
[ ... ]
> @@ -2338,46 +2340,42 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
>  	return ret;
>  }
>  
> -static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
> +static int acpi_bus_attach(struct acpi_device *device, void *not_used)
>  {
[ ... ]
>  	acpi_bus_init_power(device);
>  
> -	if (device->flags.visited)
> -		goto ok;
> -
>  	ret = acpi_scan_attach_handler(device);
>  	if (ret < 0)
>  		return 0;
>  
> +	device->flags.visited = 1;

[Severity: Medium]
The commit message establishes a strict structural invariant: 'The rule
regarding flags.visited and the "enumerated" field is that the latter cannot
be set until the former has been set and they need to be cleared in reverse
order.'

Does the implementation violate this rule in multiple paths?

For generic devices, acpi_scan_attach_handler() (called just above) calls
acpi_generic_device_attach(), which triggers acpi_default_enumeration().
This sets enumerated = true before acpi_bus_attach() reaches this line to
set device->flags.visited = 1.

Additionally, for early and fixed devices, acpi_bus_register_early_device()
and acpi_bus_add_fixed_device_object() call acpi_default_enumeration(),
which sets enumerated = true, but their flags.visited is never set to 1.

> +
>  	if (device->flags.enumeration_by_parent ||
>  	    (!ret && (device->pnp.type.platform_id || device->pnp.type.backlight)))
>  		acpi_default_enumeration(device);
>  	else
>  		acpi_device_set_enumerated(device);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=4