[PATCH 0/2] ACPI: scan: Introduce a replacement for acpi_bus_get_device()

[PATCH 0/2] ACPI: scan: Introduce a replacement for acpi_bus_get_device()

[Rafael J. Wysocki]

Hi All,

Because acpi_bus_get_device() turned out to be problematic in the past, it has
been changed to the point that its calling convention doesn't make much sense
any more (ie. the pointer passed to it as the second argument is cleared on
errors and it can only return one error value if that pointer is nonzero, so
there is some duplication of information in there) and it has to make redundant
checks.

Moreover, its name suggests some kind of reference counting which really isn't
the case.

Thus patch [1/2] introduces a replacement for it, called acpi_fetch_acpi_dev(),
and makes the code in scan.c use it instead of acpi_bus_get_device() internally.

Patch [2/2] updates all of the callers of acpi_bus_get_device() within the ACPI
subsystem to use the replacement (which involves fixing a couple of bugs related
to that).

Thanks!

[PATCH 1/2] ACPI: scan: Introduce acpi_fetch_acpi_dev()

[Rafael J. Wysocki]

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

Introduce acpi_fetch_acpi_dev() as a more reasonable replacement for
acpi_bus_get_device() and modify the code in scan.c to use it instead
of the latter.

No expected functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c     |   34 ++++++++++++++++++++++++----------
 include/acpi/acpi_bus.h |    1 +
 2 files changed, 25 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -135,12 +135,12 @@ bool acpi_scan_is_offline(struct acpi_de
 static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
 				    void **ret_p)
 {
-	struct acpi_device *device = NULL;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_device_physical_node *pn;
 	bool second_pass = (bool)data;
 	acpi_status status = AE_OK;
 
-	if (acpi_bus_get_device(handle, &device))
+	if (!device)
 		return AE_OK;
 
 	if (device->handler && !device->handler->hotplug.enabled) {
@@ -180,10 +180,10 @@ static acpi_status acpi_bus_offline(acpi
 static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data,
 				   void **ret_p)
 {
-	struct acpi_device *device = NULL;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_device_physical_node *pn;
 
-	if (acpi_bus_get_device(handle, &device))
+	if (!device)
 		return AE_OK;
 
 	mutex_lock(&device->physical_node_lock);
@@ -599,6 +599,19 @@ int acpi_bus_get_device(acpi_handle hand
 }
 EXPORT_SYMBOL(acpi_bus_get_device);
 
+/**
+ * acpi_fetch_acpi_dev - Retrieve ACPI device object.
+ * @handle: ACPI handle associated with the requested ACPI device object.
+ *
+ * Return a pointer to the ACPI device object associated with @handle, if
+ * present, or NULL otherwise.
+ */
+struct acpi_device *acpi_fetch_acpi_dev(acpi_handle handle)
+{
+	return handle_to_device(handle, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_fetch_acpi_dev);
+
 static void get_acpi_device(void *dev)
 {
 	acpi_dev_get(dev);
@@ -799,7 +812,7 @@ static const char * const acpi_ignore_de
 
 static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
 {
-	struct acpi_device *device = NULL;
+	struct acpi_device *device;
 	acpi_status status;
 
 	/*
@@ -814,7 +827,9 @@ static struct acpi_device *acpi_bus_get_
 		status = acpi_get_parent(handle, &handle);
 		if (ACPI_FAILURE(status))
 			return status == AE_NULL_ENTRY ? NULL : acpi_root;
-	} while (acpi_bus_get_device(handle, &device));
+
+		device = acpi_fetch_acpi_dev(handle);
+	} while (!device);
 	return device;
 }
 
@@ -2003,11 +2018,10 @@ static bool acpi_bus_scan_second_pass;
 static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
 				      struct acpi_device **adev_p)
 {
-	struct acpi_device *device = NULL;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	acpi_object_type acpi_type;
 	int type;
 
-	acpi_bus_get_device(handle, &device);
 	if (device)
 		goto out;
 
@@ -2548,8 +2562,8 @@ int __init acpi_scan_init(void)
 	if (result)
 		goto out;
 
-	result = acpi_bus_get_device(ACPI_ROOT_OBJECT, &acpi_root);
-	if (result)
+	acpi_root = acpi_fetch_acpi_dev(ACPI_ROOT_OBJECT);
+	if (!acpi_root)
 		goto out;
 
 	/* Fixed feature devices do not exist on HW-reduced platform */
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -505,6 +505,7 @@ extern int unregister_acpi_notifier(stru
  */
 
 int acpi_bus_get_device(acpi_handle handle, struct acpi_device **device);
+struct acpi_device *acpi_fetch_acpi_dev(acpi_handle handle);
 acpi_status acpi_bus_get_status_handle(acpi_handle handle,
 				       unsigned long long *sta);
 int acpi_bus_get_status(struct acpi_device *device);

[PATCH 2/2] ACPI: Use acpi_fetch_acpi_dev() instead of acpi_bus_get_device()

[Rafael J. Wysocki]

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

Modify the ACPI code to use acpi_fetch_acpi_dev() instead of
acpi_bus_get_device() where applicable.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_video.c       |    5 ++---
 drivers/acpi/device_pm.c        |   31 +++++++++++++------------------
 drivers/acpi/dock.c             |    3 +--
 drivers/acpi/pci_link.c         |   12 ++++--------
 drivers/acpi/pci_root.c         |   10 ++++------
 drivers/acpi/power.c            |    7 +++----
 drivers/acpi/processor_driver.c |   10 +++++++---
 drivers/acpi/processor_idle.c   |    2 +-
 drivers/acpi/property.c         |   11 +++++------
 drivers/acpi/resource.c         |    4 ++--
 drivers/acpi/thermal.c          |    9 ++++-----
 drivers/acpi/video_detect.c     |    6 ++----
 drivers/acpi/x86/s2idle.c       |    4 ++--
 13 files changed, 50 insertions(+), 64 deletions(-)

Index: linux-pm/drivers/acpi/acpi_video.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_video.c
+++ linux-pm/drivers/acpi/acpi_video.c
@@ -1733,13 +1733,12 @@ acpi_video_bus_match(acpi_handle handle,
 {
 	struct acpi_device *device = context;
 	struct acpi_device *sibling;
-	int result;
 
 	if (handle == device->handle)
 		return AE_CTRL_TERMINATE;
 
-	result = acpi_bus_get_device(handle, &sibling);
-	if (result)
+	sibling = acpi_fetch_acpi_dev(handle);
+	if (!sibling)
 		return AE_OK;
 
 	if (!strcmp(acpi_device_name(sibling), ACPI_VIDEO_BUS_NAME))
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -285,14 +285,12 @@ EXPORT_SYMBOL(acpi_device_set_power);
 
 int acpi_bus_set_power(acpi_handle handle, int state)
 {
-	struct acpi_device *device;
-	int result;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 
-	result = acpi_bus_get_device(handle, &device);
-	if (result)
-		return result;
+	if (device)
+		return acpi_device_set_power(device, state);
 
-	return acpi_device_set_power(device, state);
+	return -ENODEV;
 }
 EXPORT_SYMBOL(acpi_bus_set_power);
 
@@ -410,21 +408,20 @@ EXPORT_SYMBOL_GPL(acpi_device_update_pow
 
 int acpi_bus_update_power(acpi_handle handle, int *state_p)
 {
-	struct acpi_device *device;
-	int result;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 
-	result = acpi_bus_get_device(handle, &device);
-	return result ? result : acpi_device_update_power(device, state_p);
+	if (device)
+		return acpi_device_update_power(device, state_p);
+
+	return -ENODEV;
 }
 EXPORT_SYMBOL_GPL(acpi_bus_update_power);
 
 bool acpi_bus_power_manageable(acpi_handle handle)
 {
-	struct acpi_device *device;
-	int result;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 
-	result = acpi_bus_get_device(handle, &device);
-	return result ? false : device->flags.power_manageable;
+	return device && device->flags.power_manageable;
 }
 EXPORT_SYMBOL(acpi_bus_power_manageable);
 
@@ -543,11 +540,9 @@ acpi_status acpi_remove_pm_notifier(stru
 
 bool acpi_bus_can_wakeup(acpi_handle handle)
 {
-	struct acpi_device *device;
-	int result;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 
-	result = acpi_bus_get_device(handle, &device);
-	return result ? false : device->wakeup.flags.valid;
+	return device && device->wakeup.flags.valid;
 }
 EXPORT_SYMBOL(acpi_bus_can_wakeup);
 
Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -489,9 +489,8 @@ static ssize_t docked_show(struct device
 			   struct device_attribute *attr, char *buf)
 {
 	struct dock_station *dock_station = dev->platform_data;
-	struct acpi_device *adev = NULL;
+	struct acpi_device *adev = acpi_fetch_acpi_dev(dock_station->handle);
 
-	acpi_bus_get_device(dock_station->handle, &adev);
 	return sysfs_emit(buf, "%u\n", acpi_device_enumerated(adev));
 }
 static DEVICE_ATTR_RO(docked);
Index: linux-pm/drivers/acpi/pci_link.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_link.c
+++ linux-pm/drivers/acpi/pci_link.c
@@ -606,12 +606,10 @@ static int acpi_pci_link_allocate(struct
 int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
 			       int *polarity, char **name)
 {
-	int result;
-	struct acpi_device *device;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_pci_link *link;
 
-	result = acpi_bus_get_device(handle, &device);
-	if (result) {
+	if (!device) {
 		acpi_handle_err(handle, "Invalid link device\n");
 		return -1;
 	}
@@ -658,12 +656,10 @@ int acpi_pci_link_allocate_irq(acpi_hand
  */
 int acpi_pci_link_free_irq(acpi_handle handle)
 {
-	struct acpi_device *device;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_pci_link *link;
-	acpi_status result;
 
-	result = acpi_bus_get_device(handle, &device);
-	if (result) {
+	if (!device) {
 		acpi_handle_err(handle, "Invalid link device\n");
 		return -1;
 	}
Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_root.c
+++ linux-pm/drivers/acpi/pci_root.c
@@ -67,11 +67,10 @@ static struct acpi_scan_handler pci_root
  */
 int acpi_is_root_bridge(acpi_handle handle)
 {
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	int ret;
-	struct acpi_device *device;
 
-	ret = acpi_bus_get_device(handle, &device);
-	if (ret)
+	if (!device)
 		return 0;
 
 	ret = acpi_match_device_ids(device, root_device_ids);
@@ -215,11 +214,10 @@ static acpi_status acpi_pci_query_osc(st
 
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
 {
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_pci_root *root;
-	struct acpi_device *device;
 
-	if (acpi_bus_get_device(handle, &device) ||
-	    acpi_match_device_ids(device, root_device_ids))
+	if (!device || acpi_match_device_ids(device, root_device_ids))
 		return NULL;
 
 	root = acpi_driver_data(device);
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -81,9 +81,9 @@ struct acpi_power_resource *to_power_res
 
 static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
 {
-	struct acpi_device *device;
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 
-	if (acpi_bus_get_device(handle, &device))
+	if (!device)
 		return NULL;
 
 	return to_power_resource(device);
@@ -928,15 +928,14 @@ static void acpi_power_add_resource_to_l
 
 struct acpi_device *acpi_add_power_resource(acpi_handle handle)
 {
+	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	struct acpi_power_resource *resource;
-	struct acpi_device *device = NULL;
 	union acpi_object acpi_object;
 	struct acpi_buffer buffer = { sizeof(acpi_object), &acpi_object };
 	acpi_status status;
 	u8 state_dummy;
 	int result;
 
-	acpi_bus_get_device(handle, &device);
 	if (device)
 		return device;
 
Index: linux-pm/drivers/acpi/processor_driver.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_driver.c
+++ linux-pm/drivers/acpi/processor_driver.c
@@ -98,8 +98,13 @@ static int acpi_soft_cpu_online(unsigned
 	struct acpi_processor *pr = per_cpu(processors, cpu);
 	struct acpi_device *device;
 
-	if (!pr || acpi_bus_get_device(pr->handle, &device))
+	if (!pr)
 		return 0;
+
+	device = acpi_fetch_acpi_dev(pr->handle);
+	if (!device)
+		return 0;
+
 	/*
 	 * CPU got physically hotplugged and onlined for the first time:
 	 * Initialize missing things.
@@ -125,9 +130,8 @@ static int acpi_soft_cpu_online(unsigned
 static int acpi_soft_cpu_dead(unsigned int cpu)
 {
 	struct acpi_processor *pr = per_cpu(processors, cpu);
-	struct acpi_device *device;
 
-	if (!pr || acpi_bus_get_device(pr->handle, &device))
+	if (!pr || !acpi_fetch_acpi_dev(pr->handle))
 		return 0;
 
 	acpi_processor_reevaluate_tstate(pr, true);
Index: linux-pm/drivers/acpi/processor_idle.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_idle.c
+++ linux-pm/drivers/acpi/processor_idle.c
@@ -1099,7 +1099,7 @@ static int acpi_processor_get_lpi_info(s
 
 	status = acpi_get_parent(handle, &pr_ahandle);
 	while (ACPI_SUCCESS(status)) {
-		acpi_bus_get_device(pr_ahandle, &d);
+		d = acpi_fetch_acpi_dev(pr_ahandle);
 		handle = pr_ahandle;
 
 		if (strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -687,9 +687,9 @@ int __acpi_node_get_property_reference(c
 		if (index)
 			return -EINVAL;
 
-		ret = acpi_bus_get_device(obj->reference.handle, &device);
-		if (ret)
-			return ret == -ENODEV ? -EINVAL : ret;
+		device = acpi_fetch_acpi_dev(obj->reference.handle);
+		if (!device)
+			return -EINVAL;
 
 		args->fwnode = acpi_fwnode_handle(device);
 		args->nargs = 0;
@@ -719,9 +719,8 @@ int __acpi_node_get_property_reference(c
 		if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
 			struct fwnode_handle *ref_fwnode;
 
-			ret = acpi_bus_get_device(element->reference.handle,
-						  &device);
-			if (ret)
+			device = acpi_fetch_acpi_dev(element->reference.handle);
+			if (!device)
 				return -EINVAL;
 
 			nargs = 0;
Index: linux-pm/drivers/acpi/resource.c
===================================================================
--- linux-pm.orig/drivers/acpi/resource.c
+++ linux-pm/drivers/acpi/resource.c
@@ -791,9 +791,9 @@ static acpi_status acpi_res_consumer_cb(
 {
 	struct resource *res = context;
 	struct acpi_device **consumer = (struct acpi_device **) ret;
-	struct acpi_device *adev;
+	struct acpi_device *adev = acpi_fetch_acpi_dev(handle);
 
-	if (acpi_bus_get_device(handle, &adev))
+	if (!adev)
 		return AE_OK;
 
 	if (acpi_dev_consumes_res(adev, res)) {
Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -697,7 +697,6 @@ static int acpi_thermal_cooling_device_c
 	struct acpi_device *device = cdev->devdata;
 	struct acpi_thermal *tz = thermal->devdata;
 	struct acpi_device *dev;
-	acpi_status status;
 	acpi_handle handle;
 	int i;
 	int j;
@@ -715,8 +714,8 @@ static int acpi_thermal_cooling_device_c
 		for (i = 0; i < tz->trips.passive.devices.count;
 		    i++) {
 			handle = tz->trips.passive.devices.handles[i];
-			status = acpi_bus_get_device(handle, &dev);
-			if (ACPI_FAILURE(status) || dev != device)
+			dev = acpi_fetch_acpi_dev(handle);
+			if (dev != device)
 				continue;
 			if (bind)
 				result =
@@ -741,8 +740,8 @@ static int acpi_thermal_cooling_device_c
 		    j < tz->trips.active[i].devices.count;
 		    j++) {
 			handle = tz->trips.active[i].devices.handles[j];
-			status = acpi_bus_get_device(handle, &dev);
-			if (ACPI_FAILURE(status) || dev != device)
+			dev = acpi_fetch_acpi_dev(handle);
+			if (dev != device)
 				continue;
 			if (bind)
 				result = thermal_zone_bind_cooling_device
Index: linux-pm/drivers/acpi/video_detect.c
===================================================================
--- linux-pm.orig/drivers/acpi/video_detect.c
+++ linux-pm/drivers/acpi/video_detect.c
@@ -59,18 +59,16 @@ static void acpi_video_parse_cmdline(voi
 static acpi_status
 find_video(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
+	struct acpi_device *acpi_dev = acpi_fetch_acpi_dev(handle);
 	long *cap = context;
 	struct pci_dev *dev;
-	struct acpi_device *acpi_dev;
 
 	static const struct acpi_device_id video_ids[] = {
 		{ACPI_VIDEO_HID, 0},
 		{"", 0},
 	};
-	if (acpi_bus_get_device(handle, &acpi_dev))
-		return AE_OK;
 
-	if (!acpi_match_device_ids(acpi_dev, video_ids)) {
+	if (acpi_dev && !acpi_match_device_ids(acpi_dev, video_ids)) {
 		dev = acpi_get_pci_dev(handle);
 		if (!dev)
 			return AE_OK;
Index: linux-pm/drivers/acpi/x86/s2idle.c
===================================================================
--- linux-pm.orig/drivers/acpi/x86/s2idle.c
+++ linux-pm/drivers/acpi/x86/s2idle.c
@@ -293,9 +293,9 @@ static void lpi_check_constraints(void)
 
 	for (i = 0; i < lpi_constraints_table_size; ++i) {
 		acpi_handle handle = lpi_constraints_table[i].handle;
-		struct acpi_device *adev;
+		struct acpi_device *adev = acpi_fetch_acpi_dev(handle);
 
-		if (!handle || acpi_bus_get_device(handle, &adev))
+		if (!adev)
 			continue;
 
 		acpi_handle_debug(handle,

Re: [PATCH 0/2] ACPI: scan: Introduce a replacement for acpi_bus_get_device()

[Hans de Goede]

Hi,

On 12/3/21 17:34, Rafael J. Wysocki wrote:
> Hi All,
> 
> Because acpi_bus_get_device() turned out to be problematic in the past, it has
> been changed to the point that its calling convention doesn't make much sense
> any more (ie. the pointer passed to it as the second argument is cleared on
> errors and it can only return one error value if that pointer is nonzero, so
> there is some duplication of information in there) and it has to make redundant
> checks.
> 
> Moreover, its name suggests some kind of reference counting which really isn't
> the case.
> 
> Thus patch [1/2] introduces a replacement for it, called acpi_fetch_acpi_dev(),
> and makes the code in scan.c use it instead of acpi_bus_get_device() internally.
> 
> Patch [2/2] updates all of the callers of acpi_bus_get_device() within the ACPI
> subsystem to use the replacement (which involves fixing a couple of bugs related
> to that).

Thanks, the series looks good to me:

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

for the series.

Regards,

Hans

Re: [PATCH 0/2] ACPI: scan: Introduce a replacement for acpi_bus_get_device()

[Mika Westerberg]

On Fri, Dec 03, 2021 at 05:34:58PM +0100, Rafael J. Wysocki wrote:
> Hi All,
> 
> Because acpi_bus_get_device() turned out to be problematic in the past, it has
> been changed to the point that its calling convention doesn't make much sense
> any more (ie. the pointer passed to it as the second argument is cleared on
> errors and it can only return one error value if that pointer is nonzero, so
> there is some duplication of information in there) and it has to make redundant
> checks.
> 
> Moreover, its name suggests some kind of reference counting which really isn't
> the case.
> 
> Thus patch [1/2] introduces a replacement for it, called acpi_fetch_acpi_dev(),
> and makes the code in scan.c use it instead of acpi_bus_get_device() internally.
> 
> Patch [2/2] updates all of the callers of acpi_bus_get_device() within the ACPI
> subsystem to use the replacement (which involves fixing a couple of bugs related
> to that).

Makes perfect sense!

For the series,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>