ACPI: TAD: Use auto-cleanup macros for runtime PM

PATCH Superseded Rafael J. Wysocki 2 participants 4 messages Oct 18, 2025 – Oct 20, 2025 ACPI

lore

Share & tools

lore

Rafael J. Wysocki

author maintainer

2025-10-18 12:25 UTC #0 lore raw

v2 cover ACPI: TAD: Use auto-cleanup macros for runtime PM

Hi All,

This supersedes

https://lore.kernel.org/linux-pm/[email protected]/

the first patch of which proved to be controversial (quite as expected), so it has been dropped and the patch that was the second one previously is [1/2] now, and it has not been changed.

Patch [2/2] (previously [3/3]) has been updated to use the full ACQUIRE()/ ACQUIRE_ERR() syntax.

Thanks!

Rafael J. Wysocki

author maintainer

2025-10-18 12:25 UTC #1 lore raw

PATCH v2 1/2 ACPI: TAD: Rearrange runtime PM operations in acpi_tad_remove()

From: Rafael J. Wysocki <[email protected]>

It is not necessary to resume the device upfront in acpi_tad_remove() because both acpi_tad_disable_timer() and acpi_tad_clear_status() attempt to resume it, but it is better to prevent it from suspending between these calls by incrementing its runtime PM usage counter.

Accordingly, replace the pm_runtime_get_sync() call in acpi_tad_remove() with a pm_runtime_get_noresume() one and put the latter right before the first invocation of acpi_tad_disable_timer().

In addition, use pm_runtime_put_noidle() to drop the device's runtime PM usage counter after using pm_runtime_get_noresume() to bump it up to follow a common pattern and use pm_runtime_suspend() for suspending the device afterward.

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpi_tad.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/acpi/acpi_tad.c +4 -3

@@ -563,8 +563,6 @@ static void acpi_tad_remove(struct platf
 
 	device_init_wakeup(dev, false);
 
-	pm_runtime_get_sync(dev);
-
 	if (dd->capabilities & ACPI_TAD_RT)
 		sysfs_remove_group(&dev->kobj, &acpi_tad_time_attr_group);

@@ -573,6 +571,8 @@ static void acpi_tad_remove(struct platf
 
 	sysfs_remove_group(&dev->kobj, &acpi_tad_attr_group);
 
+	pm_runtime_get_noresume(dev);
+
 	acpi_tad_disable_timer(dev, ACPI_TAD_AC_TIMER);
 	acpi_tad_clear_status(dev, ACPI_TAD_AC_TIMER);
 	if (dd->capabilities & ACPI_TAD_DC_WAKE) {

@@ -580,7 +580,8 @@ static void acpi_tad_remove(struct platf
 		acpi_tad_clear_status(dev, ACPI_TAD_DC_TIMER);
 	}
 
-	pm_runtime_put_sync(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_suspend(dev);
 	pm_runtime_disable(dev);
 	acpi_remove_cmos_rtc_space_handler(handle);
 }

Rafael J. Wysocki

author maintainer

2025-10-18 12:24 UTC #2 lore raw

PATCH v2 2/2 ACPI: TAD: Improve runtime PM using guard macros

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Use guard pm_runtime_active_try to simplify runtime PM cleanup and implement runtime resume error handling in multiple places.

Also use guard pm_runtime_noresume to simplify acpi_tad_remove().

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpi_tad.c |   57 +++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

drivers/acpi/acpi_tad.c +25 ~~-32~~

@@ -90,12 +90,11 @@ static int acpi_tad_set_real_time(struct
 	args[0].buffer.pointer = (u8 *)rt;
 	args[0].buffer.length = sizeof(*rt);
 
-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_integer(handle, "_SRT", &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status) || retval)
 		return -EIO;

@@ -111,12 +110,11 @@ static int acpi_tad_get_real_time(struct
 	acpi_status status;
 	int ret = -EIO;
 
-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_object(handle, "_GRT", NULL, &output);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))
 		goto out_free;

@@ -266,12 +264,11 @@ static int acpi_tad_wake_set(struct devi
 	args[0].integer.value = timer_id;
 	args[1].integer.value = value;
 
-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_integer(handle, method, &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status) || retval)
 		return -EIO;

@@ -314,12 +311,11 @@ static ssize_t acpi_tad_wake_read(struct
 
 	args[0].integer.value = timer_id;
 
-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_integer(handle, method, &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))
 		return -EIO;

@@ -370,12 +366,11 @@ static int acpi_tad_clear_status(struct
 
 	args[0].integer.value = timer_id;
 
-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_integer(handle, "_CWS", &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status) || retval)
 		return -EIO;

@@ -411,12 +406,11 @@ static ssize_t acpi_tad_status_read(stru
 
 	args[0].integer.value = timer_id;
 
-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_integer(handle, "_GWS", &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))
 		return -EIO;

@@ -571,16 +565,15 @@ static void acpi_tad_remove(struct platf
 
 	sysfs_remove_group(&dev->kobj, &acpi_tad_attr_group);
 
-	pm_runtime_get_noresume(dev);
-
-	acpi_tad_disable_timer(dev, ACPI_TAD_AC_TIMER);
-	acpi_tad_clear_status(dev, ACPI_TAD_AC_TIMER);
-	if (dd->capabilities & ACPI_TAD_DC_WAKE) {
-		acpi_tad_disable_timer(dev, ACPI_TAD_DC_TIMER);
-		acpi_tad_clear_status(dev, ACPI_TAD_DC_TIMER);
+	scoped_guard(pm_runtime_noresume, dev) {
+		acpi_tad_disable_timer(dev, ACPI_TAD_AC_TIMER);
+		acpi_tad_clear_status(dev, ACPI_TAD_AC_TIMER);
+		if (dd->capabilities & ACPI_TAD_DC_WAKE) {
+			acpi_tad_disable_timer(dev, ACPI_TAD_DC_TIMER);
+			acpi_tad_clear_status(dev, ACPI_TAD_DC_TIMER);
+		}
 	}
 
-	pm_runtime_put_noidle(dev);
 	pm_runtime_suspend(dev);
 	pm_runtime_disable(dev);
 	acpi_remove_cmos_rtc_space_handler(handle);

Jonathan Cameron

2025-10-20 15:11 UTC #3 lore raw

Re: [PATCH v2 2/2] ACPI: TAD: Improve runtime PM using guard macros

Quoted 19 lines · On Sat, 18 Oct 2025 14:24:42 +0200 "Rafael J. Wysocki" <[email protected]>…

From: Rafael J. Wysocki <[email protected]>
Use guard pm_runtime_active_try to simplify runtime PM cleanup and implement runtime resume error handling in multiple places.
Also use guard pm_runtime_noresume to simplify acpi_tad_remove().
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/acpi_tad.c |   57 +++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)
drivers/acpi/acpi_tad.c +0 -0
@@ -111,12 +110,11 @@ static int acpi_tad_get_real_time(struct
 	acpi_status status;
 	int ret = -EIO;
 

drivers/acpi/acpi_tad.c static int acpi_tad_get_real_time(struct · line 118

Quoted 10 lines

drivers/acpi/acpi_tad.c +3 -4

-	pm_runtime_get_sync(dev);
+	ACQUIRE(pm_runtime_active_try, pm)(dev);
+	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+		return -ENXIO;
 
 	status = acpi_evaluate_object(handle, "_GRT", NULL, &output);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))

Whilst it isn't actually a bug, this does run up against the guidance in cleanup.h to avoid mixing gotos and cleanup.h usage in a single function. That's partly a simplification to avoid having to explain the issues with jumping past inline declarations.

If you want to follow that guidance, either you'd need to add a helper along the lines of:

DEFINE_FREE(acpi_buffer, void *m ACPI_FREE(_T));
void *acpi_eval_grt(acpi_handle handle, struct acpi_buffer *output)

{
	acpi_status status = acpi_evaluate_object(handle, "_GRT", NULL, &output);

	if (ACPI_FAILURE(status)) {
		ACPI_FREE(output.pointer);
		return ERR_PTR(-EIO); //whatever error makse sense.
	}

	return output.pointer;
}

void *out_obj __free(acpi_buffer) = acpi_eval_grt(handle, &output);

Then can return directly at all error paths. Not the nicest bit of code though.

Or, factor out everthing after that allocation down
to the label as helper function and have direct returns in that.

or leave it all as is and hope Linus doesn't get grumpy about mix and match (which lead to that guidance being so general in the first place!)

The rest look good to me.

Jonathan

Quoted 2 lines

drivers/acpi/acpi_tad.c +0 -0
 		goto out_free;
 