[PATCH v1 0/5] ACPI: scan: acpi_bus_check_add() simplifications

[PATCH v1 0/5] ACPI: scan: acpi_bus_check_add() simplifications

[Rafael J. Wysocki]

Hi,

This series simplifies acpi_bus_check_add() and related code.

It mostly is not expected to alter functionality, except for patch [4/5] that
unifies the handling of device and processor objects.

Please refer to the patch changelogs for details.

Thanks!

[PATCH v1 1/5] ACPI: scan: Fold acpi_bus_type_and_status() into its caller

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is only one caller of acpi_bus_type_and_status() which is
acpi_bus_check_add(), so fold the former into the latter and use
the observation that the initial status of the device is
ACPI_STA_DEFAULT in all cases except for ACPI_BUS_TYPE_PROCESSOR
to simplify the code.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   80 ++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 48 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1763,50 +1763,6 @@ static bool acpi_device_should_be_hidden
 	return true;
 }
 
-static int acpi_bus_type_and_status(acpi_handle handle, int *type,
-				    unsigned long long *sta)
-{
-	acpi_status status;
-	acpi_object_type acpi_type;
-
-	status = acpi_get_type(handle, &acpi_type);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	switch (acpi_type) {
-	case ACPI_TYPE_ANY:		/* for ACPI_ROOT_OBJECT */
-	case ACPI_TYPE_DEVICE:
-		if (acpi_device_should_be_hidden(handle))
-			return -ENODEV;
-
-		*type = ACPI_BUS_TYPE_DEVICE;
-		/*
-		 * acpi_add_single_object updates this once we've an acpi_device
-		 * so that acpi_bus_get_status' quirk handling can be used.
-		 */
-		*sta = ACPI_STA_DEFAULT;
-		break;
-	case ACPI_TYPE_PROCESSOR:
-		*type = ACPI_BUS_TYPE_PROCESSOR;
-		status = acpi_bus_get_status_handle(handle, sta);
-		if (ACPI_FAILURE(status))
-			return -ENODEV;
-		break;
-	case ACPI_TYPE_THERMAL:
-		*type = ACPI_BUS_TYPE_THERMAL;
-		*sta = ACPI_STA_DEFAULT;
-		break;
-	case ACPI_TYPE_POWER:
-		*type = ACPI_BUS_TYPE_POWER;
-		*sta = ACPI_STA_DEFAULT;
-		break;
-	default:
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
 bool acpi_device_is_present(const struct acpi_device *adev)
 {
 	return adev->status.present || adev->status.functional;
@@ -1953,18 +1909,46 @@ static acpi_status acpi_bus_check_add(ac
 				      struct acpi_device **adev_p)
 {
 	struct acpi_device *device = NULL;
-	unsigned long long sta;
+	unsigned long long sta = ACPI_STA_DEFAULT;
+	acpi_object_type acpi_type;
 	int type;
-	int result;
 
 	acpi_bus_get_device(handle, &device);
 	if (device)
 		goto out;
 
-	result = acpi_bus_type_and_status(handle, &type, &sta);
-	if (result)
+	if (ACPI_FAILURE(acpi_get_type(handle, &acpi_type)))
 		return AE_OK;
 
+	switch (acpi_type) {
+	case ACPI_TYPE_DEVICE:
+		if (acpi_device_should_be_hidden(handle))
+			return AE_OK;
+
+		fallthrough;
+	case ACPI_TYPE_ANY:	/* for ACPI_ROOT_OBJECT */
+		type = ACPI_BUS_TYPE_DEVICE;
+		break;
+
+	case ACPI_TYPE_PROCESSOR:
+		if (ACPI_FAILURE(acpi_bus_get_status_handle(handle, &sta)))
+			return AE_OK;
+
+		type = ACPI_BUS_TYPE_PROCESSOR;
+		break;
+
+	case ACPI_TYPE_THERMAL:
+		type = ACPI_BUS_TYPE_THERMAL;
+		break;
+
+	case ACPI_TYPE_POWER:
+		type = ACPI_BUS_TYPE_POWER;
+		break;
+
+	default:
+		return AE_OK;
+	}
+
 	if (type == ACPI_BUS_TYPE_POWER) {
 		acpi_add_power_resource(handle);
 		return AE_OK;

[PATCH v1 2/5] ACPI: scan: Rearrange checks in acpi_bus_check_add()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rearrange the checks in acpi_bus_check_add() to avoid checking
the "type" twice and take "check_dep" into account only for
ACPI_TYPE_DEVICE objects.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1831,7 +1831,7 @@ static void acpi_scan_init_hotplug(struc
 	}
 }
 
-static u32 acpi_scan_check_dep(acpi_handle handle)
+static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
 {
 	struct acpi_handle_list dep_devices;
 	acpi_status status;
@@ -1844,7 +1844,8 @@ static u32 acpi_scan_check_dep(acpi_hand
 	 * 2. ACPI nodes describing USB ports.
 	 * Still, checking for _HID catches more then just these cases ...
 	 */
-	if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, "_HID"))
+	if (!check_dep || !acpi_has_method(handle, "_DEP") ||
+	    !acpi_has_method(handle, "_HID"))
 		return 0;
 
 	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
@@ -1925,6 +1926,12 @@ static acpi_status acpi_bus_check_add(ac
 		if (acpi_device_should_be_hidden(handle))
 			return AE_OK;
 
+		/* Bail out if there are dependencies. */
+		if (acpi_scan_check_dep(handle, check_dep) > 0) {
+			acpi_bus_scan_second_pass = true;
+			return AE_CTRL_DEPTH;
+		}
+
 		fallthrough;
 	case ACPI_TYPE_ANY:	/* for ACPI_ROOT_OBJECT */
 		type = ACPI_BUS_TYPE_DEVICE;
@@ -1942,27 +1949,12 @@ static acpi_status acpi_bus_check_add(ac
 		break;
 
 	case ACPI_TYPE_POWER:
-		type = ACPI_BUS_TYPE_POWER;
-		break;
-
-	default:
-		return AE_OK;
-	}
-
-	if (type == ACPI_BUS_TYPE_POWER) {
 		acpi_add_power_resource(handle);
+		fallthrough;
+	default:
 		return AE_OK;
 	}
 
-	if (type == ACPI_BUS_TYPE_DEVICE && check_dep) {
-		u32 count = acpi_scan_check_dep(handle);
-		/* Bail out if the number of recorded dependencies is not 0. */
-		if (count > 0) {
-			acpi_bus_scan_second_pass = true;
-			return AE_CTRL_DEPTH;
-		}
-	}
-
 	acpi_add_single_object(&device, handle, type, sta);
 	if (!device)
 		return AE_CTRL_DEPTH;

[PATCH v1 3/5] ACPI: scan: Drop sta argument from acpi_add_single_object()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move the initial status check for ACPI_BUS_TYPE_PROCESSOR objects
into acpi_add_single_object() so it is not necessary to pass the
"sta" argument to it, get rid of that argument from there and update
the callers of that function accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1681,15 +1681,18 @@ void acpi_device_add_finalize(struct acp
 }
 
 static int acpi_add_single_object(struct acpi_device **child,
-				  acpi_handle handle, int type,
-				  unsigned long long sta)
+				  acpi_handle handle, int type)
 {
 	struct acpi_device_info *info = NULL;
+	unsigned long long sta = ACPI_STA_DEFAULT;
 	struct acpi_device *device;
 	int result;
 
-	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
+	if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
 		acpi_get_object_info(handle, &info);
+	else if (type == ACPI_BUS_TYPE_PROCESSOR &&
+		 ACPI_FAILURE(acpi_bus_get_status_handle(handle, &sta)))
+		return -ENODEV;
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
 	if (!device) {
@@ -1910,7 +1913,6 @@ static acpi_status acpi_bus_check_add(ac
 				      struct acpi_device **adev_p)
 {
 	struct acpi_device *device = NULL;
-	unsigned long long sta = ACPI_STA_DEFAULT;
 	acpi_object_type acpi_type;
 	int type;
 
@@ -1938,9 +1940,6 @@ static acpi_status acpi_bus_check_add(ac
 		break;
 
 	case ACPI_TYPE_PROCESSOR:
-		if (ACPI_FAILURE(acpi_bus_get_status_handle(handle, &sta)))
-			return AE_OK;
-
 		type = ACPI_BUS_TYPE_PROCESSOR;
 		break;
 
@@ -1955,7 +1954,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);
 	if (!device)
 		return AE_CTRL_DEPTH;
 
@@ -2229,8 +2228,7 @@ int acpi_bus_register_early_device(int t
 	struct acpi_device *device = NULL;
 	int result;
 
-	result = acpi_add_single_object(&device, NULL,
-					type, ACPI_STA_DEFAULT);
+	result = acpi_add_single_object(&device, NULL, type);
 	if (result)
 		return result;
 
@@ -2250,8 +2248,7 @@ static int acpi_bus_scan_fixed(void)
 		struct acpi_device *device = NULL;
 
 		result = acpi_add_single_object(&device, NULL,
-						ACPI_BUS_TYPE_POWER_BUTTON,
-						ACPI_STA_DEFAULT);
+						ACPI_BUS_TYPE_POWER_BUTTON);
 		if (result)
 			return result;
 
@@ -2267,8 +2264,7 @@ static int acpi_bus_scan_fixed(void)
 		struct acpi_device *device = NULL;
 
 		result = acpi_add_single_object(&device, NULL,
-						ACPI_BUS_TYPE_SLEEP_BUTTON,
-						ACPI_STA_DEFAULT);
+						ACPI_BUS_TYPE_SLEEP_BUTTON);
 		if (result)
 			return result;
 

[PATCH v1 4/5] ACPI: scan: Drop sta argument from acpi_init_device_object()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use the observation that the initial status check for
ACPI_BUS_TYPE_PROCESSOR objects can be carried out in the same way
as for ACPI_BUS_TYPE_DEVICE objects and it is not necessary to fail
acpi_add_single_object() if acpi_bus_get_status_handle() returns an
error for a processor (its status can be set to 0 instead) to
simplify acpi_add_single_object().

Accordingly, drop the "sta" argument from acpi_init_device_object()
as it can always set the initial status to ACPI_STA_DEFAULT and let
its caller correct that later on.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/internal.h |    3 +--
 drivers/acpi/power.c    |    3 +--
 drivers/acpi/scan.c     |   28 ++++++++++++++--------------
 3 files changed, 16 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1649,15 +1649,14 @@ static bool acpi_device_enumeration_by_p
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-			     int type, unsigned long long sta,
-			     struct acpi_device_info *info)
+			     int type, struct acpi_device_info *info)
 {
 	INIT_LIST_HEAD(&device->pnp.ids);
 	device->device_type = type;
 	device->handle = handle;
 	device->parent = acpi_bus_get_parent(handle);
 	fwnode_init(&device->fwnode, &acpi_device_fwnode_ops);
-	acpi_set_device_status(device, sta);
+	acpi_set_device_status(device, ACPI_STA_DEFAULT);
 	acpi_device_get_busid(device);
 	acpi_set_pnp_ids(handle, &device->pnp, type, info);
 	acpi_init_properties(device);
@@ -1680,19 +1679,21 @@ void acpi_device_add_finalize(struct acp
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }
 
+static void acpi_scan_init_status(struct acpi_device *adev)
+{
+	if (acpi_bus_get_status(adev))
+		acpi_set_device_status(adev, 0);
+}
+
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type)
 {
 	struct acpi_device_info *info = NULL;
-	unsigned long long sta = ACPI_STA_DEFAULT;
 	struct acpi_device *device;
 	int result;
 
 	if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
 		acpi_get_object_info(handle, &info);
-	else if (type == ACPI_BUS_TYPE_PROCESSOR &&
-		 ACPI_FAILURE(acpi_bus_get_status_handle(handle, &sta)))
-		return -ENODEV;
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
 	if (!device) {
@@ -1700,16 +1701,15 @@ static int acpi_add_single_object(struct
 		return -ENOMEM;
 	}
 
-	acpi_init_device_object(device, handle, type, sta, info);
+	acpi_init_device_object(device, handle, type, info);
 	kfree(info);
 	/*
-	 * For ACPI_BUS_TYPE_DEVICE getting the status is delayed till here so
-	 * that we can call acpi_bus_get_status() and use its quirk handling.
-	 * Note this must be done before the get power-/wakeup_dev-flags calls.
+	 * Getting the status is delayed till here so that we can call
+	 * acpi_bus_get_status() and use its quirk handling.  Note that
+	 * this must be done before the get power-/wakeup_dev-flags calls.
 	 */
-	if (type == ACPI_BUS_TYPE_DEVICE)
-		if (acpi_bus_get_status(device) < 0)
-			acpi_set_device_status(device, 0);
+	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR)
+		acpi_scan_init_status(device);
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -109,8 +109,7 @@ struct acpi_device_bus_id {
 int acpi_device_add(struct acpi_device *device,
 		    void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-			     int type, unsigned long long sta,
-			     struct acpi_device_info *info);
+			     int type, struct acpi_device_info *info);
 int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -925,8 +925,7 @@ int acpi_add_power_resource(acpi_handle
 		return -ENOMEM;
 
 	device = &resource->device;
-	acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER,
-				ACPI_STA_DEFAULT, NULL);
+	acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER, NULL);
 	mutex_init(&resource->resource_lock);
 	INIT_LIST_HEAD(&resource->list_node);
 	INIT_LIST_HEAD(&resource->dependents);

[PATCH v1 5/5] ACPI: scan: Call acpi_get_object_info() from acpi_set_pnp_ids()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that it is not necessary to call acpi_get_object_info() from
acpi_add_single_object() in order to pass the pointer returned by it
to acpi_init_device_object() and from there to acpi_set_pnp_ids().

It is more straightforward to call acpi_get_object_info() from
acpi_set_pnp_ids() and avoid unnecessary pointer passing, so change
the code accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/internal.h |    2 +-
 drivers/acpi/power.c    |    2 +-
 drivers/acpi/scan.c     |   21 +++++++++------------
 3 files changed, 11 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1307,8 +1307,9 @@ static bool acpi_object_is_system_bus(ac
 }
 
 static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
-				int device_type, struct acpi_device_info *info)
+			     int device_type)
 {
+	struct acpi_device_info *info = NULL;
 	struct acpi_pnp_device_id_list *cid_list;
 	int i;
 
@@ -1319,6 +1320,7 @@ static void acpi_set_pnp_ids(acpi_handle
 			break;
 		}
 
+		acpi_get_object_info(handle, &info);
 		if (!info) {
 			pr_err(PREFIX "%s: Error reading device info\n",
 					__func__);
@@ -1344,6 +1346,8 @@ static void acpi_set_pnp_ids(acpi_handle
 		if (info->valid & ACPI_VALID_CLS)
 			acpi_add_id(pnp, info->class_code.string);
 
+		kfree(info);
+
 		/*
 		 * Some devices don't reliably have _HIDs & _CIDs, so add
 		 * synthetic HIDs to make sure drivers can find them.
@@ -1649,7 +1653,7 @@ static bool acpi_device_enumeration_by_p
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-			     int type, struct acpi_device_info *info)
+			     int type)
 {
 	INIT_LIST_HEAD(&device->pnp.ids);
 	device->device_type = type;
@@ -1658,7 +1662,7 @@ void acpi_init_device_object(struct acpi
 	fwnode_init(&device->fwnode, &acpi_device_fwnode_ops);
 	acpi_set_device_status(device, ACPI_STA_DEFAULT);
 	acpi_device_get_busid(device);
-	acpi_set_pnp_ids(handle, &device->pnp, type, info);
+	acpi_set_pnp_ids(handle, &device->pnp, type);
 	acpi_init_properties(device);
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
@@ -1688,21 +1692,14 @@ static void acpi_scan_init_status(struct
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type)
 {
-	struct acpi_device_info *info = NULL;
 	struct acpi_device *device;
 	int result;
 
-	if (type == ACPI_BUS_TYPE_DEVICE && handle != ACPI_ROOT_OBJECT)
-		acpi_get_object_info(handle, &info);
-
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
-	if (!device) {
-		kfree(info);
+	if (!device)
 		return -ENOMEM;
-	}
 
-	acpi_init_device_object(device, handle, type, info);
-	kfree(info);
+	acpi_init_device_object(device, handle, type);
 	/*
 	 * Getting the status is delayed till here so that we can call
 	 * acpi_bus_get_status() and use its quirk handling.  Note that
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -109,7 +109,7 @@ struct acpi_device_bus_id {
 int acpi_device_add(struct acpi_device *device,
 		    void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-			     int type, struct acpi_device_info *info);
+			     int type);
 int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -925,7 +925,7 @@ int acpi_add_power_resource(acpi_handle
 		return -ENOMEM;
 
 	device = &resource->device;
-	acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER, NULL);
+	acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER);
 	mutex_init(&resource->resource_lock);
 	INIT_LIST_HEAD(&resource->list_node);
 	INIT_LIST_HEAD(&resource->dependents);

Re: [PATCH v1 0/5] ACPI: scan: acpi_bus_check_add() simplifications

[Hans de Goede]

Hi,

On 4/7/21 4:27 PM, Rafael J. Wysocki wrote:
> Hi,
> 
> This series simplifies acpi_bus_check_add() and related code.
> 
> It mostly is not expected to alter functionality, except for patch [4/5] that
> unifies the handling of device and processor objects.
> 
> Please refer to the patch changelogs for details.

Thanks, the entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans