mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex)
@ 2026-08-06  6:19 Dmitry Torokhov
  2026-08-06  6:19 ` [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR() Dmitry Torokhov
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2026-08-06  6:19 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark
  Cc: Henrique de Moraes Holschuh, Hans de Goede, Ilpo Järvinen,
	Nitin Joshi, platform-driver-x86, ibm-acpi-devel, linux-kernel

Convert straightforward mutex_lock() and mutex_unlock() usages for
hotkey_mutex, tpacpi_inputdev_send_mutex, kbdlight_mutex, lcdshadow_dev
lock, and dytc_mutex to guard(mutex) and scoped_guard(mutex) helpers
from linux/cleanup.h.

This improves code readability and ensures that mutexes are
automatically released when exiting their respective scopes.

Assisted-by: Antigravity:gemini-3.6-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/platform/x86/lenovo/thinkpad_acpi.c | 139 ++++++++------------
 1 file changed, 57 insertions(+), 82 deletions(-)

diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index f8e116e8a65d..beb85ea1103b 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -2164,7 +2164,7 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
 		return 0;
 	}
 
-	mutex_lock(&hotkey_mutex);
+	guard(mutex)(&hotkey_mutex);
 
 	HOTKEY_CONFIG_CRITICAL_START
 	hotkey_driver_mask = mask;
@@ -2177,8 +2177,6 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
 							~hotkey_source_mask);
 	hotkey_poll_setup(true);
 
-	mutex_unlock(&hotkey_mutex);
-
 	return rc;
 }
 
@@ -2202,15 +2200,12 @@ static void tpacpi_input_send_tabletsw(void)
 {
 	int state;
 
-	if (tp_features.hotkey_tablet &&
-	    !hotkey_get_tablet_mode(&state)) {
-		mutex_lock(&tpacpi_inputdev_send_mutex);
+	if (tp_features.hotkey_tablet && !hotkey_get_tablet_mode(&state)) {
+		guard(mutex)(&tpacpi_inputdev_send_mutex);
 
 		input_report_switch(tpacpi_inputdev,
 				    SW_TABLET_MODE, !!state);
 		input_sync(tpacpi_inputdev);
-
-		mutex_unlock(&tpacpi_inputdev_send_mutex);
 	}
 }
 
@@ -2235,7 +2230,6 @@ static int get_camera_shutter(void)
 
 static bool tpacpi_input_send_key(const u32 hkey, bool *send_acpi_ev)
 {
-	bool known_ev;
 	u32 scancode;
 
 	if (tpacpi_driver_event(hkey))
@@ -2278,11 +2272,8 @@ static bool tpacpi_input_send_key(const u32 hkey, bool *send_acpi_ev)
 		scancode = hkey;
 	}
 
-	mutex_lock(&tpacpi_inputdev_send_mutex);
-	known_ev = sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
-	mutex_unlock(&tpacpi_inputdev_send_mutex);
-
-	return known_ev;
+	guard(mutex)(&tpacpi_inputdev_send_mutex);
+	return sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
 }
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
@@ -2572,9 +2563,8 @@ static void hotkey_poll_setup(const bool may_warn)
 
 static void hotkey_poll_setup_safe(const bool may_warn)
 {
-	mutex_lock(&hotkey_mutex);
+	guard(mutex)(&hotkey_mutex);
 	hotkey_poll_setup(may_warn);
-	mutex_unlock(&hotkey_mutex);
 }
 
 static void hotkey_poll_set_freq(unsigned int freq)
@@ -3077,13 +3067,11 @@ static void tpacpi_send_radiosw_update(void)
 
 	/* Issue rfkill input event for WLSW switch */
 	if (!(wlsw < 0)) {
-		mutex_lock(&tpacpi_inputdev_send_mutex);
+		guard(mutex)(&tpacpi_inputdev_send_mutex);
 
 		input_report_switch(tpacpi_inputdev,
 				    SW_RFKILL_ALL, (wlsw > 0));
 		input_sync(tpacpi_inputdev);
-
-		mutex_unlock(&tpacpi_inputdev_send_mutex);
 	}
 
 	/*
@@ -3095,7 +3083,7 @@ static void tpacpi_send_radiosw_update(void)
 
 static void hotkey_exit(void)
 {
-	mutex_lock(&hotkey_mutex);
+	guard(mutex)(&hotkey_mutex);
 	hotkey_poll_stop_sync();
 	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
 		   "restoring original HKEY status and mask\n");
@@ -3105,8 +3093,6 @@ static void hotkey_exit(void)
 	      hotkey_mask_set(hotkey_orig_mask)) |
 	     hotkey_status_set(false)) != 0)
 		pr_err("failed to restore hot key mask to BIOS defaults\n");
-
-	mutex_unlock(&hotkey_mutex);
 }
 
 /*
@@ -3423,11 +3409,11 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	if (tp_features.hotkey_mask) {
 		/* hotkey_source_mask *must* be zero for
 		 * the first hotkey_mask_get to return hotkey_orig_mask */
-		mutex_lock(&hotkey_mutex);
-		res = hotkey_mask_get();
-		mutex_unlock(&hotkey_mutex);
-		if (res)
-			return res;
+		scoped_guard(mutex, &hotkey_mutex) {
+			res = hotkey_mask_get();
+			if (res)
+				return res;
+		}
 
 		hotkey_orig_mask = hotkey_acpi_mask;
 	} else {
@@ -3526,11 +3512,11 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		hotkey_exit();
 		return res;
 	}
-	mutex_lock(&hotkey_mutex);
-	res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
-			       | hotkey_driver_mask)
-			      & ~hotkey_source_mask);
-	mutex_unlock(&hotkey_mutex);
+	scoped_guard(mutex, &hotkey_mutex) {
+		res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
+				       | hotkey_driver_mask)
+				      & ~hotkey_source_mask);
+	}
 	if (res < 0 && res != -ENXIO) {
 		hotkey_exit();
 		return res;
@@ -3977,11 +3963,11 @@ static void hotkey_resume(void)
 {
 	tpacpi_disable_brightness_delay();
 
-	mutex_lock(&hotkey_mutex);
-	if (hotkey_status_set(true) < 0 ||
-	    hotkey_mask_set(hotkey_acpi_mask) < 0)
-		pr_err("error while attempting to reset the event firmware interface\n");
-	mutex_unlock(&hotkey_mutex);
+	scoped_guard(mutex, &hotkey_mutex) {
+		if (hotkey_status_set(true) < 0 ||
+		    hotkey_mask_set(hotkey_acpi_mask) < 0)
+			pr_err("error while attempting to reset the event firmware interface\n");
+	}
 
 	tpacpi_send_radiosw_update();
 	tpacpi_input_send_tabletsw();
@@ -5034,21 +5020,16 @@ static DEFINE_MUTEX(kbdlight_mutex);
 
 static int kbdlight_set_level(int level)
 {
-	int ret = 0;
-
 	if (!hkey_handle)
 		return -ENXIO;
 
-	mutex_lock(&kbdlight_mutex);
+	guard(mutex)(&kbdlight_mutex);
 
 	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
-		ret = -EIO;
-	else
-		kbdlight_brightness = level;
-
-	mutex_unlock(&kbdlight_mutex);
+		return -EIO;
 
-	return ret;
+	kbdlight_brightness = level;
+	return 0;
 }
 
 static int kbdlight_get_level(void)
@@ -10103,9 +10084,8 @@ static void lcdshadow_resume(void)
 	if (!lcdshadow_dev)
 		return;
 
-	mutex_lock(&lcdshadow_dev->lock);
+	guard(mutex)(&lcdshadow_dev->lock);
 	lcdshadow_set_sw_state(lcdshadow_dev, lcdshadow_dev->sw_state);
-	mutex_unlock(&lcdshadow_dev->lock);
 }
 
 static int lcdshadow_read(struct seq_file *m)
@@ -10137,9 +10117,8 @@ static int lcdshadow_write(char *buf)
 	if (state >= 2 || state < 0)
 		return -EINVAL;
 
-	mutex_lock(&lcdshadow_dev->lock);
-	res = lcdshadow_set_sw_state(lcdshadow_dev, state);
-	mutex_unlock(&lcdshadow_dev->lock);
+	scoped_guard(mutex, &lcdshadow_dev->lock)
+		res = lcdshadow_set_sw_state(lcdshadow_dev, state);
 
 	drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
 
@@ -10603,26 +10582,26 @@ static const struct platform_profile_ops dytc_profile_ops = {
 static void dytc_profile_refresh(void)
 {
 	enum platform_profile_option profile;
-	int output = 0, err = 0;
+	int output = 0, err;
 	int perfmode, funcmode = 0;
 
-	mutex_lock(&dytc_mutex);
-	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
-		if (dytc_mmc_get_available)
-			err = dytc_command(DYTC_CMD_MMC_GET, &output);
-		else
-			err = dytc_cql_command(DYTC_CMD_GET, &output);
-		funcmode = DYTC_FUNCTION_MMC;
-	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
-		err = dytc_command(DYTC_CMD_GET, &output);
-		/* Check if we are PSC mode, or have AMT enabled */
-		funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
-	} else { /* Unknown profile mode */
-		err = -ENODEV;
+	scoped_guard(mutex, &dytc_mutex) {
+		if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
+			if (dytc_mmc_get_available)
+				err = dytc_command(DYTC_CMD_MMC_GET, &output);
+			else
+				err = dytc_cql_command(DYTC_CMD_GET, &output);
+			funcmode = DYTC_FUNCTION_MMC;
+		} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
+			err = dytc_command(DYTC_CMD_GET, &output);
+			/* Check if we are PSC mode, or have AMT enabled */
+			funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
+		} else { /* Unknown profile mode */
+			err = -ENODEV;
+		}
+		if (err)
+			return;
 	}
-	mutex_unlock(&dytc_mutex);
-	if (err)
-		return;
 
 	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
 	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
@@ -11425,7 +11404,7 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 		if (tp_features.kbdlight) {
 			enum led_brightness brightness;
 
-			mutex_lock(&kbdlight_mutex);
+			guard(mutex)(&kbdlight_mutex);
 
 			/*
 			 * Check the brightness actually changed, setting the brightness
@@ -11437,8 +11416,6 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 				led_classdev_notify_brightness_hw_changed(
 					&tpacpi_led_kbdlight.led_classdev, brightness);
 			}
-
-			mutex_unlock(&kbdlight_mutex);
 		}
 		/* Key events are suppressed by default hotkey_user_mask */
 		return false;
@@ -11460,11 +11437,11 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 			enum drm_privacy_screen_status old_hw_state;
 			bool changed;
 
-			mutex_lock(&lcdshadow_dev->lock);
-			old_hw_state = lcdshadow_dev->hw_state;
-			lcdshadow_get_hw_state(lcdshadow_dev);
-			changed = lcdshadow_dev->hw_state != old_hw_state;
-			mutex_unlock(&lcdshadow_dev->lock);
+			scoped_guard(mutex, &lcdshadow_dev->lock) {
+				old_hw_state = lcdshadow_dev->hw_state;
+				lcdshadow_get_hw_state(lcdshadow_dev);
+				changed = lcdshadow_dev->hw_state != old_hw_state;
+			}
 
 			if (changed)
 				drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
@@ -11485,12 +11462,10 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 			pr_err("Error retrieving camera shutter state after shutter event\n");
 			return true;
 		}
-		mutex_lock(&tpacpi_inputdev_send_mutex);
-
-		input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, camera_shutter_state);
-		input_sync(tpacpi_inputdev);
-
-		mutex_unlock(&tpacpi_inputdev_send_mutex);
+		scoped_guard(mutex, &tpacpi_inputdev_send_mutex) {
+			input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, camera_shutter_state);
+			input_sync(tpacpi_inputdev);
+		}
 		return true;
 	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
 		/* Toggle kernel-level doubletap event filtering */
-- 
2.55.0.679.g6767b8d81c-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR()
  2026-08-06  6:19 [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Dmitry Torokhov
@ 2026-08-06  6:19 ` Dmitry Torokhov
  2026-08-12 18:53   ` Mark Pearson
  2026-09-06 13:10   ` Ali Ahmet Memiş
  2026-08-06  6:19 ` [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup Dmitry Torokhov
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2026-08-06  6:19 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark
  Cc: Henrique de Moraes Holschuh, Hans de Goede, Ilpo Järvinen,
	Nitin Joshi, platform-driver-x86, ibm-acpi-devel, linux-kernel

Convert conditional mutex_lock_killable() and mutex_lock_interruptible()
calls to ACQUIRE() and ACQUIRE_ERR() from linux/cleanup.h.

This eliminates explicit mutex_unlock() calls on return paths and
simplifies error handling across hotkey, brightness, volume, fan, and
dytc functions.

Assisted-by: Antigravity:gemini-3.6-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/platform/x86/lenovo/thinkpad_acpi.c | 153 ++++++++++----------
 1 file changed, 76 insertions(+), 77 deletions(-)

diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index beb85ea1103b..0d0d6fe7eecd 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -2671,8 +2671,10 @@ static ssize_t hotkey_mask_store(struct device *dev,
 	if (parse_strtoul(buf, 0xffffffffUL, &t))
 		return -EINVAL;
 
-	if (mutex_lock_killable(&hotkey_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
+	res = ACQUIRE_ERR(mutex_kill, &guard);
+	if (res)
+		return res;
 
 	res = hotkey_user_mask_set(t);
 
@@ -2680,8 +2682,6 @@ static ssize_t hotkey_mask_store(struct device *dev,
 	hotkey_poll_setup(true);
 #endif
 
-	mutex_unlock(&hotkey_mutex);
-
 	tpacpi_disclose_usertask("hotkey_mask", "set to 0x%08lx\n", t);
 
 	return (res) ? res : count;
@@ -2767,8 +2767,10 @@ static ssize_t hotkey_source_mask_store(struct device *dev,
 		((t & ~TPACPI_HKEY_NVRAM_KNOWN_MASK) != 0))
 		return -EINVAL;
 
-	if (mutex_lock_killable(&hotkey_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	HOTKEY_CONFIG_CRITICAL_START
 	hotkey_source_mask = t;
@@ -2782,8 +2784,6 @@ static ssize_t hotkey_source_mask_store(struct device *dev,
 	r_ev = hotkey_driver_mask & ~(hotkey_acpi_mask & hotkey_all_mask)
 		& ~hotkey_source_mask & TPACPI_HKEY_NVRAM_KNOWN_MASK;
 
-	mutex_unlock(&hotkey_mutex);
-
 	if (rc < 0)
 		pr_err("hotkey_source_mask: failed to update the firmware event mask!\n");
 
@@ -2811,18 +2811,19 @@ static ssize_t hotkey_poll_freq_store(struct device *dev,
 			    const char *buf, size_t count)
 {
 	unsigned long t;
+	int err;
 
 	if (parse_strtoul(buf, 25, &t))
 		return -EINVAL;
 
-	if (mutex_lock_killable(&hotkey_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
+	err = ACQUIRE_ERR(mutex_kill, &guard);
+	if (err)
+		return err;
 
 	hotkey_poll_set_freq(t);
 	hotkey_poll_setup(true);
 
-	mutex_unlock(&hotkey_mutex);
-
 	tpacpi_disclose_usertask("hotkey_poll_freq", "set to %lu\n", t);
 
 	return count;
@@ -3995,12 +3996,13 @@ static int hotkey_read(struct seq_file *m)
 		return 0;
 	}
 
-	if (mutex_lock_killable(&hotkey_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
+	res = ACQUIRE_ERR(mutex_kill, &guard);
+	if (res)
+		return res;
 	res = hotkey_status_get(&status);
 	if (!res)
 		res = hotkey_mask_get();
-	mutex_unlock(&hotkey_mutex);
 	if (res)
 		return res;
 
@@ -4033,8 +4035,10 @@ static int hotkey_write(char *buf)
 	if (!tp_features.hotkey)
 		return -ENODEV;
 
-	if (mutex_lock_killable(&hotkey_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
+	res = ACQUIRE_ERR(mutex_kill, &guard);
+	if (res)
+		return res;
 
 	mask = hotkey_user_mask;
 
@@ -4053,8 +4057,7 @@ static int hotkey_write(char *buf)
 		} else if (sscanf(cmd, "%x", &mask) == 1) {
 			/* mask set */
 		} else {
-			res = -EINVAL;
-			goto errexit;
+			return -EINVAL;
 		}
 	}
 
@@ -4064,8 +4067,6 @@ static int hotkey_write(char *buf)
 		res = hotkey_user_mask_set(mask);
 	}
 
-errexit:
-	mutex_unlock(&hotkey_mutex);
 	return res;
 }
 
@@ -6460,11 +6461,12 @@ static void tpacpi_brightness_checkpoint_nvram(void)
 	vdbg_printk(TPACPI_DBG_BRGHT,
 		"trying to checkpoint backlight level to NVRAM...\n");
 
-	if (mutex_lock_killable(&brightness_mutex) < 0)
+	ACQUIRE(mutex_kill, guard)(&brightness_mutex);
+	if (ACQUIRE_ERR(mutex_kill, &guard))
 		return;
 
 	if (unlikely(!acpi_ec_read(TP_EC_BACKLIGHT, &lec)))
-		goto unlock;
+		return;
 	lec &= TP_EC_BACKLIGHT_LVLMSK;
 	b_nvram = nvram_read_byte(TP_NVRAM_ADDR_BRIGHTNESS);
 
@@ -6482,9 +6484,6 @@ static void tpacpi_brightness_checkpoint_nvram(void)
 		vdbg_printk(TPACPI_DBG_BRGHT,
 			   "NVRAM backlight level already is %u (0x%02x)\n",
 			   (unsigned int) lec, (unsigned int) b_nvram);
-
-unlock:
-	mutex_unlock(&brightness_mutex);
 }
 
 
@@ -6562,8 +6561,9 @@ static int brightness_set(unsigned int value)
 	vdbg_printk(TPACPI_DBG_BRGHT,
 			"set backlight level to %d\n", value);
 
-	res = mutex_lock_killable(&brightness_mutex);
-	if (res < 0)
+	ACQUIRE(mutex_kill, guard)(&brightness_mutex);
+	res = ACQUIRE_ERR(mutex_kill, &guard);
+	if (res)
 		return res;
 
 	switch (brightness_mode) {
@@ -6578,7 +6578,6 @@ static int brightness_set(unsigned int value)
 		res = -ENXIO;
 	}
 
-	mutex_unlock(&brightness_mutex);
 	return res;
 }
 
@@ -6601,16 +6600,14 @@ static int brightness_get(struct backlight_device *bd)
 {
 	int status, res;
 
-	res = mutex_lock_killable(&brightness_mutex);
-	if (res < 0)
-		return 0;
+	ACQUIRE(mutex_kill, guard)(&brightness_mutex);
+	res = ACQUIRE_ERR(mutex_kill, &guard);
+	if (res)
+		return res;
 
 	res = tpacpi_brightness_get_raw(&status);
-
-	mutex_unlock(&brightness_mutex);
-
 	if (res < 0)
-		return 0;
+		return res;
 
 	return status & TP_EC_BACKLIGHT_LVLMSK;
 }
@@ -7073,11 +7070,12 @@ static void tpacpi_volume_checkpoint_nvram(void)
 	else
 		ec_mask = TP_EC_AUDIO_MUTESW_MSK | TP_EC_AUDIO_LVL_MSK;
 
-	if (mutex_lock_killable(&volume_mutex) < 0)
+	ACQUIRE(mutex_kill, guard)(&volume_mutex);
+	if (ACQUIRE_ERR(mutex_kill, &guard))
 		return;
 
 	if (unlikely(!acpi_ec_read(TP_EC_AUDIO, &lec)))
-		goto unlock;
+		return;
 	lec &= ec_mask;
 	b_nvram = nvram_read_byte(TP_NVRAM_ADDR_MIXER);
 
@@ -7094,9 +7092,6 @@ static void tpacpi_volume_checkpoint_nvram(void)
 			   "NVRAM mixer status already is 0x%02x (0x%02x)\n",
 			   (unsigned int) lec, (unsigned int) b_nvram);
 	}
-
-unlock:
-	mutex_unlock(&volume_mutex);
 }
 
 static int volume_get_status_ec(u8 *status)
@@ -7145,12 +7140,14 @@ static int __volume_set_mute_ec(const bool mute)
 	int rc;
 	u8 s, n;
 
-	if (mutex_lock_killable(&volume_mutex) < 0)
-		return -EINTR;
+	ACQUIRE(mutex_kill, guard)(&volume_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	rc = volume_get_status_ec(&s);
 	if (rc)
-		goto unlock;
+		return rc;
 
 	n = (mute) ? s | TP_EC_AUDIO_MUTESW_MSK :
 		     s & ~TP_EC_AUDIO_MUTESW_MSK;
@@ -7161,8 +7158,6 @@ static int __volume_set_mute_ec(const bool mute)
 			rc = 1;
 	}
 
-unlock:
-	mutex_unlock(&volume_mutex);
 	return rc;
 }
 
@@ -7193,12 +7188,14 @@ static int __volume_set_volume_ec(const u8 vol)
 	if (vol > TP_EC_VOLUME_MAX)
 		return -EINVAL;
 
-	if (mutex_lock_killable(&volume_mutex) < 0)
-		return -EINTR;
+	ACQUIRE(mutex_kill, guard)(&volume_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	rc = volume_get_status_ec(&s);
 	if (rc)
-		goto unlock;
+		return rc;
 
 	n = (s & ~TP_EC_AUDIO_LVL_MSK) | vol;
 
@@ -7208,8 +7205,6 @@ static int __volume_set_volume_ec(const u8 vol)
 			rc = 1;
 	}
 
-unlock:
-	mutex_unlock(&volume_mutex);
 	return rc;
 }
 
@@ -8113,13 +8108,14 @@ static int fan_get_status_safe(u8 *status)
 	int rc;
 	u8 s;
 
-	if (mutex_lock_killable(&fan_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&fan_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 	rc = fan_get_status(&s);
 	/* NS EC doesn't have register with level settings */
 	if (!rc && !fan_with_ns_addr)
 		fan_update_desired_level(s);
-	mutex_unlock(&fan_mutex);
 
 	if (rc)
 		return rc;
@@ -8312,8 +8308,10 @@ static int fan_set_level_safe(int level)
 	if (!fan_control_allowed)
 		return -EPERM;
 
-	if (mutex_lock_killable(&fan_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&fan_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	if (level == TPACPI_FAN_LAST_LEVEL)
 		level = fan_control_desired_level;
@@ -8322,7 +8320,6 @@ static int fan_set_level_safe(int level)
 	if (!rc)
 		fan_update_desired_level(level);
 
-	mutex_unlock(&fan_mutex);
 	return rc;
 }
 
@@ -8334,8 +8331,10 @@ static int fan_set_enable(void)
 	if (!fan_control_allowed)
 		return -EPERM;
 
-	if (mutex_lock_killable(&fan_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&fan_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	switch (fan_control_access_mode) {
 	case TPACPI_FAN_WR_ACPI_FANS:
@@ -8391,8 +8390,6 @@ static int fan_set_enable(void)
 		rc = -ENXIO;
 	}
 
-	mutex_unlock(&fan_mutex);
-
 	if (!rc)
 		vdbg_printk(TPACPI_DBG_FAN,
 			"fan control: set fan control register to 0x%02x\n",
@@ -8407,8 +8404,10 @@ static int fan_set_disable(void)
 	if (!fan_control_allowed)
 		return -EPERM;
 
-	if (mutex_lock_killable(&fan_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&fan_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	rc = 0;
 	switch (fan_control_access_mode) {
@@ -8453,7 +8452,6 @@ static int fan_set_disable(void)
 		vdbg_printk(TPACPI_DBG_FAN,
 			"fan control: set fan control register to 0\n");
 
-	mutex_unlock(&fan_mutex);
 	return rc;
 }
 
@@ -8464,8 +8462,10 @@ static int fan_set_speed(int speed)
 	if (!fan_control_allowed)
 		return -EPERM;
 
-	if (mutex_lock_killable(&fan_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&fan_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	rc = 0;
 	switch (fan_control_access_mode) {
@@ -8499,7 +8499,6 @@ static int fan_set_speed(int speed)
 		rc = -ENXIO;
 	}
 
-	mutex_unlock(&fan_mutex);
 	return rc;
 }
 
@@ -8659,8 +8658,10 @@ static ssize_t fan_pwm1_store(struct device *dev,
 	/* scale down from 0-255 to 0-7 */
 	newlevel = (s >> 5) & 0x07;
 
-	if (mutex_lock_killable(&fan_mutex))
-		return -ERESTARTSYS;
+	ACQUIRE(mutex_kill, guard)(&fan_mutex);
+	rc = ACQUIRE_ERR(mutex_kill, &guard);
+	if (rc)
+		return rc;
 
 	rc = fan_get_status(&status);
 	if (!rc && (status &
@@ -8674,7 +8675,6 @@ static ssize_t fan_pwm1_store(struct device *dev,
 		}
 	}
 
-	mutex_unlock(&fan_mutex);
 	return (rc) ? rc : count;
 }
 
@@ -10522,13 +10522,14 @@ static int dytc_profile_set(struct device *dev,
 	int output;
 	int err;
 
-	err = mutex_lock_interruptible(&dytc_mutex);
+	ACQUIRE(mutex_intr, guard)(&dytc_mutex);
+	err = ACQUIRE_ERR(mutex_intr, &guard);
 	if (err)
 		return err;
 
 	err = convert_profile_to_dytc(profile, &perfmode);
 	if (err)
-		goto unlock;
+		return err;
 
 	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
 		if (profile == PLATFORM_PROFILE_BALANCED) {
@@ -10540,18 +10541,18 @@ static int dytc_profile_set(struct device *dev,
 			 */
 			err = dytc_cql_command(DYTC_CMD_RESET, &output);
 			if (err)
-				goto unlock;
+				return err;
 		} else {
 			/* Determine if we are in CQL mode. This alters the commands we do */
 			err = dytc_cql_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1),
 						&output);
 			if (err)
-				goto unlock;
+				return err;
 		}
 	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
 		err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_PSC, perfmode, 1), &output);
 		if (err)
-			goto unlock;
+			return err;
 
 		/* system supports AMT, activate it when on balanced */
 		if (dytc_capabilities & BIT(DYTC_FC_AMT))
@@ -10559,8 +10560,6 @@ static int dytc_profile_set(struct device *dev,
 	}
 	/* Success - update current profile */
 	dytc_current_profile = profile;
-unlock:
-	mutex_unlock(&dytc_mutex);
 	return err;
 }
 
-- 
2.55.0.679.g6767b8d81c-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup
  2026-08-06  6:19 [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Dmitry Torokhov
  2026-08-06  6:19 ` [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR() Dmitry Torokhov
@ 2026-08-06  6:19 ` Dmitry Torokhov
  2026-08-12 19:02   ` Mark Pearson
  2026-09-06 13:10   ` Ali Ahmet Memiş
  2026-08-12 18:29 ` [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Mark Pearson
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2026-08-06  6:19 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark
  Cc: Henrique de Moraes Holschuh, Hans de Goede, Ilpo Järvinen,
	Nitin Joshi, platform-driver-x86, ibm-acpi-devel, linux-kernel

Use __free(kfree) for local pointer allocations in dispatch_proc_write(),
tpacpi_brightness_get_ecnvram(), and auxmac_init().

This ensures automatic memory cleanup when exiting function scope and
removes explicit kfree() calls on exit paths.

Assisted-by: Antigravity:gemini-3.6-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/platform/x86/lenovo/thinkpad_acpi.c | 40 +++++++--------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index 0d0d6fe7eecd..200e20f90a4b 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -885,7 +885,6 @@ static ssize_t dispatch_proc_write(struct file *file,
 			size_t count, loff_t *pos)
 {
 	struct ibm_struct *ibm = pde_data(file_inode(file));
-	char *kernbuf;
 	int ret;
 
 	if (!ibm || !ibm->write)
@@ -893,16 +892,15 @@ static ssize_t dispatch_proc_write(struct file *file,
 	if (count > PAGE_SIZE - 1)
 		return -EINVAL;
 
-	kernbuf = memdup_user_nul(userbuf, count);
+	char *kernbuf __free(kfree) = memdup_user_nul(userbuf, count);
 	if (IS_ERR(kernbuf))
 		return PTR_ERR(kernbuf);
-	ret = ibm->write(kernbuf);
-	if (ret == 0)
-		ret = count;
 
-	kfree(kernbuf);
+	ret = ibm->write(kernbuf);
+	if (ret)
+		return ret;
 
-	return ret;
+	return count;
 }
 
 static const struct proc_ops dispatch_proc_ops = {
@@ -6628,26 +6626,21 @@ static const struct backlight_ops ibm_backlight_data = {
 static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
 	acpi_status status;
-	int rc;
 
 	status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer);
 	if (ACPI_FAILURE(status))
 		return 0;
 
-	obj = buffer.pointer;
+	union acpi_object *obj __free(kfree) = buffer.pointer;
 	if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
 		acpi_handle_info(adev->handle,
 				 "Unknown _BCL data, please report this to %s\n",
 				 TPACPI_MAIL);
-		rc = 0;
-	} else {
-		rc = obj->package.count;
+		return 0;
 	}
-	kfree(obj);
 
-	return rc;
+	return obj->package.count;
 }
 
 /*
@@ -10989,24 +10982,23 @@ static int auxmac_init(struct ibm_init_struct *iibm)
 {
 	acpi_status status;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
 
-	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
+	strscpy(auxmac, "unavailable", sizeof(auxmac));
 
+	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	obj = buffer.pointer;
-
+	union acpi_object *obj __free(kfree) = buffer.pointer;
 	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
 		pr_info("Invalid buffer for MAC address pass-through.\n");
-		goto auxmacinvalid;
+		return 0;
 	}
 
 	if (obj->string.pointer[AUXMAC_BEGIN_MARKER] != '#' ||
 	    obj->string.pointer[AUXMAC_END_MARKER] != '#') {
 		pr_info("Invalid header for MAC address pass-through.\n");
-		goto auxmacinvalid;
+		return 0;
 	}
 
 	if (strncmp(obj->string.pointer + AUXMAC_START, "XXXXXXXXXXXX", AUXMAC_LEN) != 0)
@@ -11014,13 +11006,7 @@ static int auxmac_init(struct ibm_init_struct *iibm)
 	else
 		strscpy(auxmac, "disabled", sizeof(auxmac));
 
-free:
-	kfree(obj);
 	return 0;
-
-auxmacinvalid:
-	strscpy(auxmac, "unavailable", sizeof(auxmac));
-	goto free;
 }
 
 static struct ibm_struct auxmac_data = {
-- 
2.55.0.679.g6767b8d81c-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex)
  2026-08-06  6:19 [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Dmitry Torokhov
  2026-08-06  6:19 ` [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR() Dmitry Torokhov
  2026-08-06  6:19 ` [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup Dmitry Torokhov
@ 2026-08-12 18:29 ` Mark Pearson
  2026-08-12 19:55   ` [ibm-acpi-devel] " Mark Pearson
  2026-09-06 13:10 ` Ali Ahmet Memiş
  2026-09-15 11:33 ` Ilpo Järvinen
  4 siblings, 1 reply; 11+ messages in thread
From: Mark Pearson @ 2026-08-12 18:29 UTC (permalink / raw)
  To: Dmitry Torokhov, Derek J . Clark
  Cc: Henrique de Moraes Holschuh, Hans de Goede, Ilpo Järvinen,
	Nitin Joshi, platform-driver-x86, ibm-acpi-devel, linux-kernel

Thanks Dmitry,

On Thu, Aug 6, 2026, at 2:19 AM, Dmitry Torokhov wrote:
> Convert straightforward mutex_lock() and mutex_unlock() usages for
> hotkey_mutex, tpacpi_inputdev_send_mutex, kbdlight_mutex, lcdshadow_dev
> lock, and dytc_mutex to guard(mutex) and scoped_guard(mutex) helpers
> from linux/cleanup.h.
>
> This improves code readability and ensures that mutexes are
> automatically released when exiting their respective scopes.
>
> Assisted-by: Antigravity:gemini-3.6-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 139 ++++++++------------
>  1 file changed, 57 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c 
> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index f8e116e8a65d..beb85ea1103b 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -2164,7 +2164,7 @@ static int tpacpi_hotkey_driver_mask_set(const 
> u32 mask)
>  		return 0;
>  	}
> 
> -	mutex_lock(&hotkey_mutex);
> +	guard(mutex)(&hotkey_mutex);
> 
>  	HOTKEY_CONFIG_CRITICAL_START
>  	hotkey_driver_mask = mask;
> @@ -2177,8 +2177,6 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
>  							~hotkey_source_mask);
>  	hotkey_poll_setup(true);
> 
> -	mutex_unlock(&hotkey_mutex);
> -
>  	return rc;
>  }
> 
> @@ -2202,15 +2200,12 @@ static void tpacpi_input_send_tabletsw(void)
>  {
>  	int state;
> 
> -	if (tp_features.hotkey_tablet &&
> -	    !hotkey_get_tablet_mode(&state)) {
> -		mutex_lock(&tpacpi_inputdev_send_mutex);
> +	if (tp_features.hotkey_tablet && !hotkey_get_tablet_mode(&state)) {
> +		guard(mutex)(&tpacpi_inputdev_send_mutex);
> 
>  		input_report_switch(tpacpi_inputdev,
>  				    SW_TABLET_MODE, !!state);
>  		input_sync(tpacpi_inputdev);
> -
> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>  	}
>  }
> 
> @@ -2235,7 +2230,6 @@ static int get_camera_shutter(void)
> 
>  static bool tpacpi_input_send_key(const u32 hkey, bool *send_acpi_ev)
>  {
> -	bool known_ev;
>  	u32 scancode;
> 
>  	if (tpacpi_driver_event(hkey))
> @@ -2278,11 +2272,8 @@ static bool tpacpi_input_send_key(const u32 
> hkey, bool *send_acpi_ev)
>  		scancode = hkey;
>  	}
> 
> -	mutex_lock(&tpacpi_inputdev_send_mutex);
> -	known_ev = sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
> -	mutex_unlock(&tpacpi_inputdev_send_mutex);
> -
> -	return known_ev;
> +	guard(mutex)(&tpacpi_inputdev_send_mutex);
> +	return sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
>  }
> 
>  #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> @@ -2572,9 +2563,8 @@ static void hotkey_poll_setup(const bool may_warn)
> 
>  static void hotkey_poll_setup_safe(const bool may_warn)
>  {
> -	mutex_lock(&hotkey_mutex);
> +	guard(mutex)(&hotkey_mutex);
>  	hotkey_poll_setup(may_warn);
> -	mutex_unlock(&hotkey_mutex);
>  }
> 
>  static void hotkey_poll_set_freq(unsigned int freq)
> @@ -3077,13 +3067,11 @@ static void tpacpi_send_radiosw_update(void)
> 
>  	/* Issue rfkill input event for WLSW switch */
>  	if (!(wlsw < 0)) {
> -		mutex_lock(&tpacpi_inputdev_send_mutex);
> +		guard(mutex)(&tpacpi_inputdev_send_mutex);
> 
>  		input_report_switch(tpacpi_inputdev,
>  				    SW_RFKILL_ALL, (wlsw > 0));
>  		input_sync(tpacpi_inputdev);
> -
> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>  	}
> 
>  	/*
> @@ -3095,7 +3083,7 @@ static void tpacpi_send_radiosw_update(void)
> 
>  static void hotkey_exit(void)
>  {
> -	mutex_lock(&hotkey_mutex);
> +	guard(mutex)(&hotkey_mutex);
>  	hotkey_poll_stop_sync();
>  	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
>  		   "restoring original HKEY status and mask\n");
> @@ -3105,8 +3093,6 @@ static void hotkey_exit(void)
>  	      hotkey_mask_set(hotkey_orig_mask)) |
>  	     hotkey_status_set(false)) != 0)
>  		pr_err("failed to restore hot key mask to BIOS defaults\n");
> -
> -	mutex_unlock(&hotkey_mutex);
>  }
> 
>  /*
> @@ -3423,11 +3409,11 @@ static int __init hotkey_init(struct 
> ibm_init_struct *iibm)
>  	if (tp_features.hotkey_mask) {
>  		/* hotkey_source_mask *must* be zero for
>  		 * the first hotkey_mask_get to return hotkey_orig_mask */
> -		mutex_lock(&hotkey_mutex);
> -		res = hotkey_mask_get();
> -		mutex_unlock(&hotkey_mutex);
> -		if (res)
> -			return res;
> +		scoped_guard(mutex, &hotkey_mutex) {
> +			res = hotkey_mask_get();
> +			if (res)
> +				return res;
> +		}
> 
>  		hotkey_orig_mask = hotkey_acpi_mask;
>  	} else {
> @@ -3526,11 +3512,11 @@ static int __init hotkey_init(struct 
> ibm_init_struct *iibm)
>  		hotkey_exit();
>  		return res;
>  	}
> -	mutex_lock(&hotkey_mutex);
> -	res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
> -			       | hotkey_driver_mask)
> -			      & ~hotkey_source_mask);
> -	mutex_unlock(&hotkey_mutex);
> +	scoped_guard(mutex, &hotkey_mutex) {
> +		res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
> +				       | hotkey_driver_mask)
> +				      & ~hotkey_source_mask);
> +	}
>  	if (res < 0 && res != -ENXIO) {
>  		hotkey_exit();
>  		return res;
> @@ -3977,11 +3963,11 @@ static void hotkey_resume(void)
>  {
>  	tpacpi_disable_brightness_delay();
> 
> -	mutex_lock(&hotkey_mutex);
> -	if (hotkey_status_set(true) < 0 ||
> -	    hotkey_mask_set(hotkey_acpi_mask) < 0)
> -		pr_err("error while attempting to reset the event firmware interface\n");
> -	mutex_unlock(&hotkey_mutex);
> +	scoped_guard(mutex, &hotkey_mutex) {
> +		if (hotkey_status_set(true) < 0 ||
> +		    hotkey_mask_set(hotkey_acpi_mask) < 0)
> +			pr_err("error while attempting to reset the event firmware interface\n");
> +	}
> 
>  	tpacpi_send_radiosw_update();
>  	tpacpi_input_send_tabletsw();
> @@ -5034,21 +5020,16 @@ static DEFINE_MUTEX(kbdlight_mutex);
> 
>  static int kbdlight_set_level(int level)
>  {
> -	int ret = 0;
> -
>  	if (!hkey_handle)
>  		return -ENXIO;
> 
> -	mutex_lock(&kbdlight_mutex);
> +	guard(mutex)(&kbdlight_mutex);
> 
>  	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
> -		ret = -EIO;
> -	else
> -		kbdlight_brightness = level;
> -
> -	mutex_unlock(&kbdlight_mutex);
> +		return -EIO;
> 
> -	return ret;
> +	kbdlight_brightness = level;
> +	return 0;
>  }
> 
>  static int kbdlight_get_level(void)
> @@ -10103,9 +10084,8 @@ static void lcdshadow_resume(void)
>  	if (!lcdshadow_dev)
>  		return;
> 
> -	mutex_lock(&lcdshadow_dev->lock);
> +	guard(mutex)(&lcdshadow_dev->lock);
>  	lcdshadow_set_sw_state(lcdshadow_dev, lcdshadow_dev->sw_state);
> -	mutex_unlock(&lcdshadow_dev->lock);
>  }
> 
>  static int lcdshadow_read(struct seq_file *m)
> @@ -10137,9 +10117,8 @@ static int lcdshadow_write(char *buf)
>  	if (state >= 2 || state < 0)
>  		return -EINVAL;
> 
> -	mutex_lock(&lcdshadow_dev->lock);
> -	res = lcdshadow_set_sw_state(lcdshadow_dev, state);
> -	mutex_unlock(&lcdshadow_dev->lock);
> +	scoped_guard(mutex, &lcdshadow_dev->lock)
> +		res = lcdshadow_set_sw_state(lcdshadow_dev, state);
> 
>  	drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
> 
> @@ -10603,26 +10582,26 @@ static const struct platform_profile_ops 
> dytc_profile_ops = {
>  static void dytc_profile_refresh(void)
>  {
>  	enum platform_profile_option profile;
> -	int output = 0, err = 0;
> +	int output = 0, err;
>  	int perfmode, funcmode = 0;
> 
> -	mutex_lock(&dytc_mutex);
> -	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
> -		if (dytc_mmc_get_available)
> -			err = dytc_command(DYTC_CMD_MMC_GET, &output);
> -		else
> -			err = dytc_cql_command(DYTC_CMD_GET, &output);
> -		funcmode = DYTC_FUNCTION_MMC;
> -	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
> -		err = dytc_command(DYTC_CMD_GET, &output);
> -		/* Check if we are PSC mode, or have AMT enabled */
> -		funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> -	} else { /* Unknown profile mode */
> -		err = -ENODEV;
> +	scoped_guard(mutex, &dytc_mutex) {
> +		if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
> +			if (dytc_mmc_get_available)
> +				err = dytc_command(DYTC_CMD_MMC_GET, &output);
> +			else
> +				err = dytc_cql_command(DYTC_CMD_GET, &output);
> +			funcmode = DYTC_FUNCTION_MMC;
> +		} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
> +			err = dytc_command(DYTC_CMD_GET, &output);
> +			/* Check if we are PSC mode, or have AMT enabled */
> +			funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> +		} else { /* Unknown profile mode */
> +			err = -ENODEV;
> +		}
> +		if (err)
> +			return;
>  	}
> -	mutex_unlock(&dytc_mutex);
> -	if (err)
> -		return;
> 
>  	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>  	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
> @@ -11425,7 +11404,7 @@ static bool tpacpi_driver_event(const unsigned 
> int hkey_event)
>  		if (tp_features.kbdlight) {
>  			enum led_brightness brightness;
> 
> -			mutex_lock(&kbdlight_mutex);
> +			guard(mutex)(&kbdlight_mutex);
> 
>  			/*
>  			 * Check the brightness actually changed, setting the brightness
> @@ -11437,8 +11416,6 @@ static bool tpacpi_driver_event(const unsigned 
> int hkey_event)
>  				led_classdev_notify_brightness_hw_changed(
>  					&tpacpi_led_kbdlight.led_classdev, brightness);
>  			}
> -
> -			mutex_unlock(&kbdlight_mutex);
>  		}
>  		/* Key events are suppressed by default hotkey_user_mask */
>  		return false;
> @@ -11460,11 +11437,11 @@ static bool tpacpi_driver_event(const 
> unsigned int hkey_event)
>  			enum drm_privacy_screen_status old_hw_state;
>  			bool changed;
> 
> -			mutex_lock(&lcdshadow_dev->lock);
> -			old_hw_state = lcdshadow_dev->hw_state;
> -			lcdshadow_get_hw_state(lcdshadow_dev);
> -			changed = lcdshadow_dev->hw_state != old_hw_state;
> -			mutex_unlock(&lcdshadow_dev->lock);
> +			scoped_guard(mutex, &lcdshadow_dev->lock) {
> +				old_hw_state = lcdshadow_dev->hw_state;
> +				lcdshadow_get_hw_state(lcdshadow_dev);
> +				changed = lcdshadow_dev->hw_state != old_hw_state;
> +			}
> 
>  			if (changed)
>  				drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
> @@ -11485,12 +11462,10 @@ static bool tpacpi_driver_event(const 
> unsigned int hkey_event)
>  			pr_err("Error retrieving camera shutter state after shutter 
> event\n");
>  			return true;
>  		}
> -		mutex_lock(&tpacpi_inputdev_send_mutex);
> -
> -		input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, 
> camera_shutter_state);
> -		input_sync(tpacpi_inputdev);
> -
> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
> +		scoped_guard(mutex, &tpacpi_inputdev_send_mutex) {
> +			input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, 
> camera_shutter_state);
> +			input_sync(tpacpi_inputdev);
> +		}
>  		return true;
>  	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
>  		/* Toggle kernel-level doubletap event filtering */
> -- 
> 2.55.0.679.g6767b8d81c-goog

Sorry, took me a while to get to this one.
Changes look good, nice cleanup.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR()
  2026-08-06  6:19 ` [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR() Dmitry Torokhov
@ 2026-08-12 18:53   ` Mark Pearson
  2026-09-06 13:10   ` Ali Ahmet Memiş
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Pearson @ 2026-08-12 18:53 UTC (permalink / raw)
  To: Dmitry Torokhov, Derek J . Clark
  Cc: Henrique de Moraes Holschuh, Hans de Goede, Ilpo Järvinen,
	Nitin Joshi, platform-driver-x86, ibm-acpi-devel, linux-kernel

Thanks Dmitry,

On Thu, Aug 6, 2026, at 2:19 AM, Dmitry Torokhov wrote:
> Convert conditional mutex_lock_killable() and mutex_lock_interruptible()
> calls to ACQUIRE() and ACQUIRE_ERR() from linux/cleanup.h.
>
> This eliminates explicit mutex_unlock() calls on return paths and
> simplifies error handling across hotkey, brightness, volume, fan, and
> dytc functions.
>
> Assisted-by: Antigravity:gemini-3.6-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 153 ++++++++++----------
>  1 file changed, 76 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c 
> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index beb85ea1103b..0d0d6fe7eecd 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -2671,8 +2671,10 @@ static ssize_t hotkey_mask_store(struct device 
> *dev,
>  	if (parse_strtoul(buf, 0xffffffffUL, &t))
>  		return -EINVAL;
> 
> -	if (mutex_lock_killable(&hotkey_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
> +	res = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (res)
> +		return res;
> 
>  	res = hotkey_user_mask_set(t);
> 
> @@ -2680,8 +2682,6 @@ static ssize_t hotkey_mask_store(struct device *dev,
>  	hotkey_poll_setup(true);
>  #endif
> 
> -	mutex_unlock(&hotkey_mutex);
> -
>  	tpacpi_disclose_usertask("hotkey_mask", "set to 0x%08lx\n", t);
> 
>  	return (res) ? res : count;
> @@ -2767,8 +2767,10 @@ static ssize_t hotkey_source_mask_store(struct 
> device *dev,
>  		((t & ~TPACPI_HKEY_NVRAM_KNOWN_MASK) != 0))
>  		return -EINVAL;
> 
> -	if (mutex_lock_killable(&hotkey_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	HOTKEY_CONFIG_CRITICAL_START
>  	hotkey_source_mask = t;
> @@ -2782,8 +2784,6 @@ static ssize_t hotkey_source_mask_store(struct 
> device *dev,
>  	r_ev = hotkey_driver_mask & ~(hotkey_acpi_mask & hotkey_all_mask)
>  		& ~hotkey_source_mask & TPACPI_HKEY_NVRAM_KNOWN_MASK;
> 
> -	mutex_unlock(&hotkey_mutex);
> -
>  	if (rc < 0)
>  		pr_err("hotkey_source_mask: failed to update the firmware event mask!\n");
> 
> @@ -2811,18 +2811,19 @@ static ssize_t hotkey_poll_freq_store(struct 
> device *dev,
>  			    const char *buf, size_t count)
>  {
>  	unsigned long t;
> +	int err;
> 
>  	if (parse_strtoul(buf, 25, &t))
>  		return -EINVAL;
> 
> -	if (mutex_lock_killable(&hotkey_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
> +	err = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (err)
> +		return err;
> 
>  	hotkey_poll_set_freq(t);
>  	hotkey_poll_setup(true);
> 
> -	mutex_unlock(&hotkey_mutex);
> -
>  	tpacpi_disclose_usertask("hotkey_poll_freq", "set to %lu\n", t);
> 
>  	return count;
> @@ -3995,12 +3996,13 @@ static int hotkey_read(struct seq_file *m)
>  		return 0;
>  	}
> 
> -	if (mutex_lock_killable(&hotkey_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
> +	res = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (res)
> +		return res;
>  	res = hotkey_status_get(&status);
>  	if (!res)
>  		res = hotkey_mask_get();
> -	mutex_unlock(&hotkey_mutex);
>  	if (res)
>  		return res;
> 
> @@ -4033,8 +4035,10 @@ static int hotkey_write(char *buf)
>  	if (!tp_features.hotkey)
>  		return -ENODEV;
> 
> -	if (mutex_lock_killable(&hotkey_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&hotkey_mutex);
> +	res = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (res)
> +		return res;
> 
>  	mask = hotkey_user_mask;
> 
> @@ -4053,8 +4057,7 @@ static int hotkey_write(char *buf)
>  		} else if (sscanf(cmd, "%x", &mask) == 1) {
>  			/* mask set */
>  		} else {
> -			res = -EINVAL;
> -			goto errexit;
> +			return -EINVAL;
>  		}
>  	}
> 
> @@ -4064,8 +4067,6 @@ static int hotkey_write(char *buf)
>  		res = hotkey_user_mask_set(mask);
>  	}
> 
> -errexit:
> -	mutex_unlock(&hotkey_mutex);
>  	return res;
>  }
> 
> @@ -6460,11 +6461,12 @@ static void tpacpi_brightness_checkpoint_nvram(void)
>  	vdbg_printk(TPACPI_DBG_BRGHT,
>  		"trying to checkpoint backlight level to NVRAM...\n");
> 
> -	if (mutex_lock_killable(&brightness_mutex) < 0)
> +	ACQUIRE(mutex_kill, guard)(&brightness_mutex);
> +	if (ACQUIRE_ERR(mutex_kill, &guard))
>  		return;
> 
>  	if (unlikely(!acpi_ec_read(TP_EC_BACKLIGHT, &lec)))
> -		goto unlock;
> +		return;
>  	lec &= TP_EC_BACKLIGHT_LVLMSK;
>  	b_nvram = nvram_read_byte(TP_NVRAM_ADDR_BRIGHTNESS);
> 
> @@ -6482,9 +6484,6 @@ static void tpacpi_brightness_checkpoint_nvram(void)
>  		vdbg_printk(TPACPI_DBG_BRGHT,
>  			   "NVRAM backlight level already is %u (0x%02x)\n",
>  			   (unsigned int) lec, (unsigned int) b_nvram);
> -
> -unlock:
> -	mutex_unlock(&brightness_mutex);
>  }
> 
> 
> @@ -6562,8 +6561,9 @@ static int brightness_set(unsigned int value)
>  	vdbg_printk(TPACPI_DBG_BRGHT,
>  			"set backlight level to %d\n", value);
> 
> -	res = mutex_lock_killable(&brightness_mutex);
> -	if (res < 0)
> +	ACQUIRE(mutex_kill, guard)(&brightness_mutex);
> +	res = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (res)
>  		return res;
> 
>  	switch (brightness_mode) {
> @@ -6578,7 +6578,6 @@ static int brightness_set(unsigned int value)
>  		res = -ENXIO;
>  	}
> 
> -	mutex_unlock(&brightness_mutex);
>  	return res;
>  }
> 
> @@ -6601,16 +6600,14 @@ static int brightness_get(struct backlight_device *bd)
>  {
>  	int status, res;
> 
> -	res = mutex_lock_killable(&brightness_mutex);
> -	if (res < 0)
> -		return 0;
> +	ACQUIRE(mutex_kill, guard)(&brightness_mutex);
> +	res = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (res)
> +		return res;
> 
>  	res = tpacpi_brightness_get_raw(&status);
> -
> -	mutex_unlock(&brightness_mutex);
> -
>  	if (res < 0)
> -		return 0;
> +		return res;
> 
>  	return status & TP_EC_BACKLIGHT_LVLMSK;
>  }
> @@ -7073,11 +7070,12 @@ static void tpacpi_volume_checkpoint_nvram(void)
>  	else
>  		ec_mask = TP_EC_AUDIO_MUTESW_MSK | TP_EC_AUDIO_LVL_MSK;
> 
> -	if (mutex_lock_killable(&volume_mutex) < 0)
> +	ACQUIRE(mutex_kill, guard)(&volume_mutex);
> +	if (ACQUIRE_ERR(mutex_kill, &guard))
>  		return;
> 
>  	if (unlikely(!acpi_ec_read(TP_EC_AUDIO, &lec)))
> -		goto unlock;
> +		return;
>  	lec &= ec_mask;
>  	b_nvram = nvram_read_byte(TP_NVRAM_ADDR_MIXER);
> 
> @@ -7094,9 +7092,6 @@ static void tpacpi_volume_checkpoint_nvram(void)
>  			   "NVRAM mixer status already is 0x%02x (0x%02x)\n",
>  			   (unsigned int) lec, (unsigned int) b_nvram);
>  	}
> -
> -unlock:
> -	mutex_unlock(&volume_mutex);
>  }
> 
>  static int volume_get_status_ec(u8 *status)
> @@ -7145,12 +7140,14 @@ static int __volume_set_mute_ec(const bool mute)
>  	int rc;
>  	u8 s, n;
> 
> -	if (mutex_lock_killable(&volume_mutex) < 0)
> -		return -EINTR;
> +	ACQUIRE(mutex_kill, guard)(&volume_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	rc = volume_get_status_ec(&s);
>  	if (rc)
> -		goto unlock;
> +		return rc;
> 
>  	n = (mute) ? s | TP_EC_AUDIO_MUTESW_MSK :
>  		     s & ~TP_EC_AUDIO_MUTESW_MSK;
> @@ -7161,8 +7158,6 @@ static int __volume_set_mute_ec(const bool mute)
>  			rc = 1;
>  	}
> 
> -unlock:
> -	mutex_unlock(&volume_mutex);
>  	return rc;
>  }
> 
> @@ -7193,12 +7188,14 @@ static int __volume_set_volume_ec(const u8 vol)
>  	if (vol > TP_EC_VOLUME_MAX)
>  		return -EINVAL;
> 
> -	if (mutex_lock_killable(&volume_mutex) < 0)
> -		return -EINTR;
> +	ACQUIRE(mutex_kill, guard)(&volume_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	rc = volume_get_status_ec(&s);
>  	if (rc)
> -		goto unlock;
> +		return rc;
> 
>  	n = (s & ~TP_EC_AUDIO_LVL_MSK) | vol;
> 
> @@ -7208,8 +7205,6 @@ static int __volume_set_volume_ec(const u8 vol)
>  			rc = 1;
>  	}
> 
> -unlock:
> -	mutex_unlock(&volume_mutex);
>  	return rc;
>  }
> 
> @@ -8113,13 +8108,14 @@ static int fan_get_status_safe(u8 *status)
>  	int rc;
>  	u8 s;
> 
> -	if (mutex_lock_killable(&fan_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&fan_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
>  	rc = fan_get_status(&s);
>  	/* NS EC doesn't have register with level settings */
>  	if (!rc && !fan_with_ns_addr)
>  		fan_update_desired_level(s);
> -	mutex_unlock(&fan_mutex);
> 
>  	if (rc)
>  		return rc;
> @@ -8312,8 +8308,10 @@ static int fan_set_level_safe(int level)
>  	if (!fan_control_allowed)
>  		return -EPERM;
> 
> -	if (mutex_lock_killable(&fan_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&fan_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	if (level == TPACPI_FAN_LAST_LEVEL)
>  		level = fan_control_desired_level;
> @@ -8322,7 +8320,6 @@ static int fan_set_level_safe(int level)
>  	if (!rc)
>  		fan_update_desired_level(level);
> 
> -	mutex_unlock(&fan_mutex);
>  	return rc;
>  }
> 
> @@ -8334,8 +8331,10 @@ static int fan_set_enable(void)
>  	if (!fan_control_allowed)
>  		return -EPERM;
> 
> -	if (mutex_lock_killable(&fan_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&fan_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	switch (fan_control_access_mode) {
>  	case TPACPI_FAN_WR_ACPI_FANS:
> @@ -8391,8 +8390,6 @@ static int fan_set_enable(void)
>  		rc = -ENXIO;
>  	}
> 
> -	mutex_unlock(&fan_mutex);
> -
>  	if (!rc)
>  		vdbg_printk(TPACPI_DBG_FAN,
>  			"fan control: set fan control register to 0x%02x\n",
> @@ -8407,8 +8404,10 @@ static int fan_set_disable(void)
>  	if (!fan_control_allowed)
>  		return -EPERM;
> 
> -	if (mutex_lock_killable(&fan_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&fan_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	rc = 0;
>  	switch (fan_control_access_mode) {
> @@ -8453,7 +8452,6 @@ static int fan_set_disable(void)
>  		vdbg_printk(TPACPI_DBG_FAN,
>  			"fan control: set fan control register to 0\n");
> 
> -	mutex_unlock(&fan_mutex);
>  	return rc;
>  }
> 
> @@ -8464,8 +8462,10 @@ static int fan_set_speed(int speed)
>  	if (!fan_control_allowed)
>  		return -EPERM;
> 
> -	if (mutex_lock_killable(&fan_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&fan_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	rc = 0;
>  	switch (fan_control_access_mode) {
> @@ -8499,7 +8499,6 @@ static int fan_set_speed(int speed)
>  		rc = -ENXIO;
>  	}
> 
> -	mutex_unlock(&fan_mutex);
>  	return rc;
>  }
> 
> @@ -8659,8 +8658,10 @@ static ssize_t fan_pwm1_store(struct device *dev,
>  	/* scale down from 0-255 to 0-7 */
>  	newlevel = (s >> 5) & 0x07;
> 
> -	if (mutex_lock_killable(&fan_mutex))
> -		return -ERESTARTSYS;
> +	ACQUIRE(mutex_kill, guard)(&fan_mutex);
> +	rc = ACQUIRE_ERR(mutex_kill, &guard);
> +	if (rc)
> +		return rc;
> 
>  	rc = fan_get_status(&status);
>  	if (!rc && (status &
> @@ -8674,7 +8675,6 @@ static ssize_t fan_pwm1_store(struct device *dev,
>  		}
>  	}
> 
> -	mutex_unlock(&fan_mutex);
>  	return (rc) ? rc : count;
>  }
> 
> @@ -10522,13 +10522,14 @@ static int dytc_profile_set(struct device *dev,
>  	int output;
>  	int err;
> 
> -	err = mutex_lock_interruptible(&dytc_mutex);
> +	ACQUIRE(mutex_intr, guard)(&dytc_mutex);
> +	err = ACQUIRE_ERR(mutex_intr, &guard);
>  	if (err)
>  		return err;
> 
>  	err = convert_profile_to_dytc(profile, &perfmode);
>  	if (err)
> -		goto unlock;
> +		return err;
> 
>  	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>  		if (profile == PLATFORM_PROFILE_BALANCED) {
> @@ -10540,18 +10541,18 @@ static int dytc_profile_set(struct device 
> *dev,
>  			 */
>  			err = dytc_cql_command(DYTC_CMD_RESET, &output);
>  			if (err)
> -				goto unlock;
> +				return err;
>  		} else {
>  			/* Determine if we are in CQL mode. This alters the commands we do 
> */
>  			err = dytc_cql_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, 
> perfmode, 1),
>  						&output);
>  			if (err)
> -				goto unlock;
> +				return err;
>  		}
>  	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
>  		err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_PSC, perfmode, 1), 
> &output);
>  		if (err)
> -			goto unlock;
> +			return err;
> 
>  		/* system supports AMT, activate it when on balanced */
>  		if (dytc_capabilities & BIT(DYTC_FC_AMT))
> @@ -10559,8 +10560,6 @@ static int dytc_profile_set(struct device *dev,
>  	}
>  	/* Success - update current profile */
>  	dytc_current_profile = profile;
> -unlock:
> -	mutex_unlock(&dytc_mutex);
>  	return err;
>  }
> 
> -- 
> 2.55.0.679.g6767b8d81c-goog

I've not come across ACQUIRE & ACQUIRE_KILL before - so this was all new to me.
I went and did some reading and all the above looks good to me, and looks like a valid clean-up.
But my reviewed-by tag does come with reduced value....

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Thanks for the learning experience. I will aim to go and try this out (and the other patches in the series) on some HW in the near future.

Mark

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup
  2026-08-06  6:19 ` [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup Dmitry Torokhov
@ 2026-08-12 19:02   ` Mark Pearson
  2026-09-06 13:10   ` Ali Ahmet Memiş
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Pearson @ 2026-08-12 19:02 UTC (permalink / raw)
  To: Dmitry Torokhov, Derek J . Clark
  Cc: Henrique de Moraes Holschuh, Hans de Goede, Ilpo Järvinen,
	Nitin Joshi, platform-driver-x86, ibm-acpi-devel, linux-kernel

Thanks Dmitry,

On Thu, Aug 6, 2026, at 2:19 AM, Dmitry Torokhov wrote:
> Use __free(kfree) for local pointer allocations in dispatch_proc_write(),
> tpacpi_brightness_get_ecnvram(), and auxmac_init().
>
> This ensures automatic memory cleanup when exiting function scope and
> removes explicit kfree() calls on exit paths.
>
> Assisted-by: Antigravity:gemini-3.6-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 40 +++++++--------------
>  1 file changed, 13 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c 
> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index 0d0d6fe7eecd..200e20f90a4b 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -885,7 +885,6 @@ static ssize_t dispatch_proc_write(struct file 
> *file,
>  			size_t count, loff_t *pos)
>  {
>  	struct ibm_struct *ibm = pde_data(file_inode(file));
> -	char *kernbuf;
>  	int ret;
> 
>  	if (!ibm || !ibm->write)
> @@ -893,16 +892,15 @@ static ssize_t dispatch_proc_write(struct file *file,
>  	if (count > PAGE_SIZE - 1)
>  		return -EINVAL;
> 
> -	kernbuf = memdup_user_nul(userbuf, count);
> +	char *kernbuf __free(kfree) = memdup_user_nul(userbuf, count);
>  	if (IS_ERR(kernbuf))
>  		return PTR_ERR(kernbuf);
> -	ret = ibm->write(kernbuf);
> -	if (ret == 0)
> -		ret = count;
> 
> -	kfree(kernbuf);
> +	ret = ibm->write(kernbuf);
> +	if (ret)
> +		return ret;
> 
> -	return ret;
> +	return count;
>  }
> 
>  static const struct proc_ops dispatch_proc_ops = {
> @@ -6628,26 +6626,21 @@ static const struct backlight_ops ibm_backlight_data = {
>  static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used)
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> -	union acpi_object *obj;
>  	acpi_status status;
> -	int rc;
> 
>  	status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer);
>  	if (ACPI_FAILURE(status))
>  		return 0;
> 
> -	obj = buffer.pointer;
> +	union acpi_object *obj __free(kfree) = buffer.pointer;
>  	if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
>  		acpi_handle_info(adev->handle,
>  				 "Unknown _BCL data, please report this to %s\n",
>  				 TPACPI_MAIL);
> -		rc = 0;
> -	} else {
> -		rc = obj->package.count;
> +		return 0;
>  	}
> -	kfree(obj);
> 
> -	return rc;
> +	return obj->package.count;
>  }
> 
>  /*
> @@ -10989,24 +10982,23 @@ static int auxmac_init(struct ibm_init_struct *iibm)
>  {
>  	acpi_status status;
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> -	union acpi_object *obj;
> 
> -	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
> +	strscpy(auxmac, "unavailable", sizeof(auxmac));
> 
> +	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> 
> -	obj = buffer.pointer;
> -
> +	union acpi_object *obj __free(kfree) = buffer.pointer;
>  	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
>  		pr_info("Invalid buffer for MAC address pass-through.\n");
> -		goto auxmacinvalid;
> +		return 0;
>  	}
> 
>  	if (obj->string.pointer[AUXMAC_BEGIN_MARKER] != '#' ||
>  	    obj->string.pointer[AUXMAC_END_MARKER] != '#') {
>  		pr_info("Invalid header for MAC address pass-through.\n");
> -		goto auxmacinvalid;
> +		return 0;
>  	}
> 
>  	if (strncmp(obj->string.pointer + AUXMAC_START, "XXXXXXXXXXXX", 
> AUXMAC_LEN) != 0)
> @@ -11014,13 +11006,7 @@ static int auxmac_init(struct ibm_init_struct 
> *iibm)
>  	else
>  		strscpy(auxmac, "disabled", sizeof(auxmac));
> 
> -free:
> -	kfree(obj);
>  	return 0;
> -
> -auxmacinvalid:
> -	strscpy(auxmac, "unavailable", sizeof(auxmac));
> -	goto free;
>  }
> 
>  static struct ibm_struct auxmac_data = {
> -- 
> 2.55.0.679.g6767b8d81c-goog

Another kernel implementation I didn't know about.
It all looks good to me, and gives some nice clean-up. Thanks

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Mark

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ibm-acpi-devel] [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex)
  2026-08-12 18:29 ` [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Mark Pearson
@ 2026-08-12 19:55   ` Mark Pearson
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Pearson @ 2026-08-12 19:55 UTC (permalink / raw)
  To: Dmitry Torokhov, Derek J . Clark
  Cc: linux-kernel, platform-driver-x86, ibm-acpi-devel,
	Henrique de Moraes Holschuh, Ilpo Järvinen, Nitin Joshi,
	Hans de Goede

On Wed, Aug 12, 2026, at 2:29 PM, Mark Pearson wrote:
> Thanks Dmitry,
>
> On Thu, Aug 6, 2026, at 2:19 AM, Dmitry Torokhov wrote:
>> Convert straightforward mutex_lock() and mutex_unlock() usages for
>> hotkey_mutex, tpacpi_inputdev_send_mutex, kbdlight_mutex, lcdshadow_dev
>> lock, and dytc_mutex to guard(mutex) and scoped_guard(mutex) helpers
>> from linux/cleanup.h.
>>
>> This improves code readability and ensures that mutexes are
>> automatically released when exiting their respective scopes.
>>
>> Assisted-by: Antigravity:gemini-3.6-flash
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 139 ++++++++------------
>>  1 file changed, 57 insertions(+), 82 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c 
>> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> index f8e116e8a65d..beb85ea1103b 100644
>> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> @@ -2164,7 +2164,7 @@ static int tpacpi_hotkey_driver_mask_set(const 
>> u32 mask)
>>  		return 0;
>>  	}
>> 
>> -	mutex_lock(&hotkey_mutex);
>> +	guard(mutex)(&hotkey_mutex);
>> 
>>  	HOTKEY_CONFIG_CRITICAL_START
>>  	hotkey_driver_mask = mask;
>> @@ -2177,8 +2177,6 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
>>  							~hotkey_source_mask);
>>  	hotkey_poll_setup(true);
>> 
>> -	mutex_unlock(&hotkey_mutex);
>> -
>>  	return rc;
>>  }
>> 
>> @@ -2202,15 +2200,12 @@ static void tpacpi_input_send_tabletsw(void)
>>  {
>>  	int state;
>> 
>> -	if (tp_features.hotkey_tablet &&
>> -	    !hotkey_get_tablet_mode(&state)) {
>> -		mutex_lock(&tpacpi_inputdev_send_mutex);
>> +	if (tp_features.hotkey_tablet && !hotkey_get_tablet_mode(&state)) {
>> +		guard(mutex)(&tpacpi_inputdev_send_mutex);
>> 
>>  		input_report_switch(tpacpi_inputdev,
>>  				    SW_TABLET_MODE, !!state);
>>  		input_sync(tpacpi_inputdev);
>> -
>> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>>  	}
>>  }
>> 
>> @@ -2235,7 +2230,6 @@ static int get_camera_shutter(void)
>> 
>>  static bool tpacpi_input_send_key(const u32 hkey, bool *send_acpi_ev)
>>  {
>> -	bool known_ev;
>>  	u32 scancode;
>> 
>>  	if (tpacpi_driver_event(hkey))
>> @@ -2278,11 +2272,8 @@ static bool tpacpi_input_send_key(const u32 
>> hkey, bool *send_acpi_ev)
>>  		scancode = hkey;
>>  	}
>> 
>> -	mutex_lock(&tpacpi_inputdev_send_mutex);
>> -	known_ev = sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
>> -	mutex_unlock(&tpacpi_inputdev_send_mutex);
>> -
>> -	return known_ev;
>> +	guard(mutex)(&tpacpi_inputdev_send_mutex);
>> +	return sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
>>  }
>> 
>>  #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
>> @@ -2572,9 +2563,8 @@ static void hotkey_poll_setup(const bool may_warn)
>> 
>>  static void hotkey_poll_setup_safe(const bool may_warn)
>>  {
>> -	mutex_lock(&hotkey_mutex);
>> +	guard(mutex)(&hotkey_mutex);
>>  	hotkey_poll_setup(may_warn);
>> -	mutex_unlock(&hotkey_mutex);
>>  }
>> 
>>  static void hotkey_poll_set_freq(unsigned int freq)
>> @@ -3077,13 +3067,11 @@ static void tpacpi_send_radiosw_update(void)
>> 
>>  	/* Issue rfkill input event for WLSW switch */
>>  	if (!(wlsw < 0)) {
>> -		mutex_lock(&tpacpi_inputdev_send_mutex);
>> +		guard(mutex)(&tpacpi_inputdev_send_mutex);
>> 
>>  		input_report_switch(tpacpi_inputdev,
>>  				    SW_RFKILL_ALL, (wlsw > 0));
>>  		input_sync(tpacpi_inputdev);
>> -
>> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>>  	}
>> 
>>  	/*
>> @@ -3095,7 +3083,7 @@ static void tpacpi_send_radiosw_update(void)
>> 
>>  static void hotkey_exit(void)
>>  {
>> -	mutex_lock(&hotkey_mutex);
>> +	guard(mutex)(&hotkey_mutex);
>>  	hotkey_poll_stop_sync();
>>  	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
>>  		   "restoring original HKEY status and mask\n");
>> @@ -3105,8 +3093,6 @@ static void hotkey_exit(void)
>>  	      hotkey_mask_set(hotkey_orig_mask)) |
>>  	     hotkey_status_set(false)) != 0)
>>  		pr_err("failed to restore hot key mask to BIOS defaults\n");
>> -
>> -	mutex_unlock(&hotkey_mutex);
>>  }
>> 
>>  /*
>> @@ -3423,11 +3409,11 @@ static int __init hotkey_init(struct 
>> ibm_init_struct *iibm)
>>  	if (tp_features.hotkey_mask) {
>>  		/* hotkey_source_mask *must* be zero for
>>  		 * the first hotkey_mask_get to return hotkey_orig_mask */
>> -		mutex_lock(&hotkey_mutex);
>> -		res = hotkey_mask_get();
>> -		mutex_unlock(&hotkey_mutex);
>> -		if (res)
>> -			return res;
>> +		scoped_guard(mutex, &hotkey_mutex) {
>> +			res = hotkey_mask_get();
>> +			if (res)
>> +				return res;
>> +		}
>> 
>>  		hotkey_orig_mask = hotkey_acpi_mask;
>>  	} else {
>> @@ -3526,11 +3512,11 @@ static int __init hotkey_init(struct 
>> ibm_init_struct *iibm)
>>  		hotkey_exit();
>>  		return res;
>>  	}
>> -	mutex_lock(&hotkey_mutex);
>> -	res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
>> -			       | hotkey_driver_mask)
>> -			      & ~hotkey_source_mask);
>> -	mutex_unlock(&hotkey_mutex);
>> +	scoped_guard(mutex, &hotkey_mutex) {
>> +		res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
>> +				       | hotkey_driver_mask)
>> +				      & ~hotkey_source_mask);
>> +	}
>>  	if (res < 0 && res != -ENXIO) {
>>  		hotkey_exit();
>>  		return res;
>> @@ -3977,11 +3963,11 @@ static void hotkey_resume(void)
>>  {
>>  	tpacpi_disable_brightness_delay();
>> 
>> -	mutex_lock(&hotkey_mutex);
>> -	if (hotkey_status_set(true) < 0 ||
>> -	    hotkey_mask_set(hotkey_acpi_mask) < 0)
>> -		pr_err("error while attempting to reset the event firmware interface\n");
>> -	mutex_unlock(&hotkey_mutex);
>> +	scoped_guard(mutex, &hotkey_mutex) {
>> +		if (hotkey_status_set(true) < 0 ||
>> +		    hotkey_mask_set(hotkey_acpi_mask) < 0)
>> +			pr_err("error while attempting to reset the event firmware interface\n");
>> +	}
>> 
>>  	tpacpi_send_radiosw_update();
>>  	tpacpi_input_send_tabletsw();
>> @@ -5034,21 +5020,16 @@ static DEFINE_MUTEX(kbdlight_mutex);
>> 
>>  static int kbdlight_set_level(int level)
>>  {
>> -	int ret = 0;
>> -
>>  	if (!hkey_handle)
>>  		return -ENXIO;
>> 
>> -	mutex_lock(&kbdlight_mutex);
>> +	guard(mutex)(&kbdlight_mutex);
>> 
>>  	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
>> -		ret = -EIO;
>> -	else
>> -		kbdlight_brightness = level;
>> -
>> -	mutex_unlock(&kbdlight_mutex);
>> +		return -EIO;
>> 
>> -	return ret;
>> +	kbdlight_brightness = level;
>> +	return 0;
>>  }
>> 
>>  static int kbdlight_get_level(void)
>> @@ -10103,9 +10084,8 @@ static void lcdshadow_resume(void)
>>  	if (!lcdshadow_dev)
>>  		return;
>> 
>> -	mutex_lock(&lcdshadow_dev->lock);
>> +	guard(mutex)(&lcdshadow_dev->lock);
>>  	lcdshadow_set_sw_state(lcdshadow_dev, lcdshadow_dev->sw_state);
>> -	mutex_unlock(&lcdshadow_dev->lock);
>>  }
>> 
>>  static int lcdshadow_read(struct seq_file *m)
>> @@ -10137,9 +10117,8 @@ static int lcdshadow_write(char *buf)
>>  	if (state >= 2 || state < 0)
>>  		return -EINVAL;
>> 
>> -	mutex_lock(&lcdshadow_dev->lock);
>> -	res = lcdshadow_set_sw_state(lcdshadow_dev, state);
>> -	mutex_unlock(&lcdshadow_dev->lock);
>> +	scoped_guard(mutex, &lcdshadow_dev->lock)
>> +		res = lcdshadow_set_sw_state(lcdshadow_dev, state);
>> 
>>  	drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
>> 
>> @@ -10603,26 +10582,26 @@ static const struct platform_profile_ops 
>> dytc_profile_ops = {
>>  static void dytc_profile_refresh(void)
>>  {
>>  	enum platform_profile_option profile;
>> -	int output = 0, err = 0;
>> +	int output = 0, err;
>>  	int perfmode, funcmode = 0;
>> 
>> -	mutex_lock(&dytc_mutex);
>> -	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>> -		if (dytc_mmc_get_available)
>> -			err = dytc_command(DYTC_CMD_MMC_GET, &output);
>> -		else
>> -			err = dytc_cql_command(DYTC_CMD_GET, &output);
>> -		funcmode = DYTC_FUNCTION_MMC;
>> -	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
>> -		err = dytc_command(DYTC_CMD_GET, &output);
>> -		/* Check if we are PSC mode, or have AMT enabled */
>> -		funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
>> -	} else { /* Unknown profile mode */
>> -		err = -ENODEV;
>> +	scoped_guard(mutex, &dytc_mutex) {
>> +		if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>> +			if (dytc_mmc_get_available)
>> +				err = dytc_command(DYTC_CMD_MMC_GET, &output);
>> +			else
>> +				err = dytc_cql_command(DYTC_CMD_GET, &output);
>> +			funcmode = DYTC_FUNCTION_MMC;
>> +		} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
>> +			err = dytc_command(DYTC_CMD_GET, &output);
>> +			/* Check if we are PSC mode, or have AMT enabled */
>> +			funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
>> +		} else { /* Unknown profile mode */
>> +			err = -ENODEV;
>> +		}
>> +		if (err)
>> +			return;
>>  	}
>> -	mutex_unlock(&dytc_mutex);
>> -	if (err)
>> -		return;
>> 
>>  	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>>  	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
>> @@ -11425,7 +11404,7 @@ static bool tpacpi_driver_event(const unsigned 
>> int hkey_event)
>>  		if (tp_features.kbdlight) {
>>  			enum led_brightness brightness;
>> 
>> -			mutex_lock(&kbdlight_mutex);
>> +			guard(mutex)(&kbdlight_mutex);
>> 
>>  			/*
>>  			 * Check the brightness actually changed, setting the brightness
>> @@ -11437,8 +11416,6 @@ static bool tpacpi_driver_event(const unsigned 
>> int hkey_event)
>>  				led_classdev_notify_brightness_hw_changed(
>>  					&tpacpi_led_kbdlight.led_classdev, brightness);
>>  			}
>> -
>> -			mutex_unlock(&kbdlight_mutex);
>>  		}
>>  		/* Key events are suppressed by default hotkey_user_mask */
>>  		return false;
>> @@ -11460,11 +11437,11 @@ static bool tpacpi_driver_event(const 
>> unsigned int hkey_event)
>>  			enum drm_privacy_screen_status old_hw_state;
>>  			bool changed;
>> 
>> -			mutex_lock(&lcdshadow_dev->lock);
>> -			old_hw_state = lcdshadow_dev->hw_state;
>> -			lcdshadow_get_hw_state(lcdshadow_dev);
>> -			changed = lcdshadow_dev->hw_state != old_hw_state;
>> -			mutex_unlock(&lcdshadow_dev->lock);
>> +			scoped_guard(mutex, &lcdshadow_dev->lock) {
>> +				old_hw_state = lcdshadow_dev->hw_state;
>> +				lcdshadow_get_hw_state(lcdshadow_dev);
>> +				changed = lcdshadow_dev->hw_state != old_hw_state;
>> +			}
>> 
>>  			if (changed)
>>  				drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
>> @@ -11485,12 +11462,10 @@ static bool tpacpi_driver_event(const 
>> unsigned int hkey_event)
>>  			pr_err("Error retrieving camera shutter state after shutter 
>> event\n");
>>  			return true;
>>  		}
>> -		mutex_lock(&tpacpi_inputdev_send_mutex);
>> -
>> -		input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, 
>> camera_shutter_state);
>> -		input_sync(tpacpi_inputdev);
>> -
>> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>> +		scoped_guard(mutex, &tpacpi_inputdev_send_mutex) {
>> +			input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, 
>> camera_shutter_state);
>> +			input_sync(tpacpi_inputdev);
>> +		}
>>  		return true;
>>  	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
>>  		/* Toggle kernel-level doubletap event filtering */
>> -- 
>> 2.55.0.679.g6767b8d81c-goog
>
> Sorry, took me a while to get to this one.
> Changes look good, nice cleanup.
>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>
> Mark
>
Ran a build with all 3 patches on a P14s G7 and couldn't see any problems.
So, for the series:
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex)
  2026-08-06  6:19 [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Dmitry Torokhov
                   ` (2 preceding siblings ...)
  2026-08-12 18:29 ` [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Mark Pearson
@ 2026-09-06 13:10 ` Ali Ahmet Memiş
  2026-09-15 11:33 ` Ilpo Järvinen
  4 siblings, 0 replies; 11+ messages in thread
From: Ali Ahmet Memiş @ 2026-09-06 13:10 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Pearson, Derek J. Clark, Henrique de Moraes Holschuh,
	Hans de Goede, Ilpo Järvinen, Nitin Joshi,
	platform-driver-x86, ibm-acpi-devel, linux-kernel

Hi Dmitry,

Tested the series on a ThinkPad T490, v7.2.3 with CONFIG_PROVE_LOCKING. The
patched and unpatched modules behaved identically and there were no lockdep
reports. debug_locks stayed 1 throughout.

Tested the hotkey, NVRAM polling, DYTC profile, keyboard backlight, fan
control, error paths, module reload and suspend/resume paths.

Tested-by: Ali Ahmet Memiş <aliamemis@disroot.org>

Thanks,
Ali

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR()
  2026-08-06  6:19 ` [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR() Dmitry Torokhov
  2026-08-12 18:53   ` Mark Pearson
@ 2026-09-06 13:10   ` Ali Ahmet Memiş
  1 sibling, 0 replies; 11+ messages in thread
From: Ali Ahmet Memiş @ 2026-09-06 13:10 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Pearson, Derek J. Clark, Henrique de Moraes Holschuh,
	Hans de Goede, Ilpo Järvinen, Nitin Joshi,
	platform-driver-x86, ibm-acpi-devel, linux-kernel

Tested on a ThinkPad T490 with lockdep enabled, details in my reply to 1/3.

One question on brightness_get(). It is the backlight_ops->get_brightness
callback and has returned 0 on failure so far, with the conversion it now
returns a negative errno instead. In backlight.c that changes what callers
see. actual_brightness_show() passes the negative value out to the sysfs
read rather than reporting 0, and backlight_force_update() logs "Could not
update brightness from device" when the EC read fails. Is that intended?

I couldn't test this, since the T490 backlight is owned by i915, so
thinkpad-acpi registers no backlight device on this machine.

Tested-by: Ali Ahmet Memiş <aliamemis@disroot.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup
  2026-08-06  6:19 ` [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup Dmitry Torokhov
  2026-08-12 19:02   ` Mark Pearson
@ 2026-09-06 13:10   ` Ali Ahmet Memiş
  1 sibling, 0 replies; 11+ messages in thread
From: Ali Ahmet Memiş @ 2026-09-06 13:10 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Pearson, Derek J. Clark, Henrique de Moraes Holschuh,
	Hans de Goede, Ilpo Järvinen, Nitin Joshi,
	platform-driver-x86, ibm-acpi-devel, linux-kernel

Tested on a ThinkPad T490 with lockdep enabled, details in my reply to 1/3.

Tested-by: Ali Ahmet Memiş <aliamemis@disroot.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex)
  2026-08-06  6:19 [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Dmitry Torokhov
                   ` (3 preceding siblings ...)
  2026-09-06 13:10 ` Ali Ahmet Memiş
@ 2026-09-15 11:33 ` Ilpo Järvinen
  4 siblings, 0 replies; 11+ messages in thread
From: Ilpo Järvinen @ 2026-09-15 11:33 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mark Pearson, Derek J. Clark, Henrique de Moraes Holschuh,
	Hans de Goede, Nitin Joshi, platform-driver-x86, ibm-acpi-devel,
	LKML

On Wed, 5 Aug 2026, Dmitry Torokhov wrote:

> Convert straightforward mutex_lock() and mutex_unlock() usages for
> hotkey_mutex, tpacpi_inputdev_send_mutex, kbdlight_mutex, lcdshadow_dev
> lock, and dytc_mutex to guard(mutex) and scoped_guard(mutex) helpers
> from linux/cleanup.h.
> 
> This improves code readability and ensures that mutexes are
> automatically released when exiting their respective scopes.
> 
> Assisted-by: Antigravity:gemini-3.6-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Hi,

I've now applied this patch 1 into the review-ilpo-next branch. I 
converted the newly added mutex_lock/unlock() pair in hotkey_poll_setup() 
while at it but it would have been nice if you'd have made them a series 
instead and done that for me as there was unclear dependency between the 
input_device_enabled() change and this one because of the newly added 
mutex_lock/unlock() pair. Hopefully the next time. :-)

Patch 2 seems contested and changes behavior without telling upfront. And 
a return value change shouldn't be hidden into otherwise mechanical 
conversion patch like that anyway. The change is generally good otherwise
so please resend it once the return value thing is addressed.

Sashiko complains about the strscpy() placement in patch 3 and that looks 
valid concern to me.

--
 i.

> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 139 ++++++++------------
>  1 file changed, 57 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index f8e116e8a65d..beb85ea1103b 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -2164,7 +2164,7 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
>  		return 0;
>  	}
>  
> -	mutex_lock(&hotkey_mutex);
> +	guard(mutex)(&hotkey_mutex);
>  
>  	HOTKEY_CONFIG_CRITICAL_START
>  	hotkey_driver_mask = mask;
> @@ -2177,8 +2177,6 @@ static int tpacpi_hotkey_driver_mask_set(const u32 mask)
>  							~hotkey_source_mask);
>  	hotkey_poll_setup(true);
>  
> -	mutex_unlock(&hotkey_mutex);
> -
>  	return rc;
>  }
>  
> @@ -2202,15 +2200,12 @@ static void tpacpi_input_send_tabletsw(void)
>  {
>  	int state;
>  
> -	if (tp_features.hotkey_tablet &&
> -	    !hotkey_get_tablet_mode(&state)) {
> -		mutex_lock(&tpacpi_inputdev_send_mutex);
> +	if (tp_features.hotkey_tablet && !hotkey_get_tablet_mode(&state)) {
> +		guard(mutex)(&tpacpi_inputdev_send_mutex);
>  
>  		input_report_switch(tpacpi_inputdev,
>  				    SW_TABLET_MODE, !!state);
>  		input_sync(tpacpi_inputdev);
> -
> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>  	}
>  }
>  
> @@ -2235,7 +2230,6 @@ static int get_camera_shutter(void)
>  
>  static bool tpacpi_input_send_key(const u32 hkey, bool *send_acpi_ev)
>  {
> -	bool known_ev;
>  	u32 scancode;
>  
>  	if (tpacpi_driver_event(hkey))
> @@ -2278,11 +2272,8 @@ static bool tpacpi_input_send_key(const u32 hkey, bool *send_acpi_ev)
>  		scancode = hkey;
>  	}
>  
> -	mutex_lock(&tpacpi_inputdev_send_mutex);
> -	known_ev = sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
> -	mutex_unlock(&tpacpi_inputdev_send_mutex);
> -
> -	return known_ev;
> +	guard(mutex)(&tpacpi_inputdev_send_mutex);
> +	return sparse_keymap_report_event(tpacpi_inputdev, scancode, 1, true);
>  }
>  
>  #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> @@ -2572,9 +2563,8 @@ static void hotkey_poll_setup(const bool may_warn)
>  
>  static void hotkey_poll_setup_safe(const bool may_warn)
>  {
> -	mutex_lock(&hotkey_mutex);
> +	guard(mutex)(&hotkey_mutex);
>  	hotkey_poll_setup(may_warn);
> -	mutex_unlock(&hotkey_mutex);
>  }
>  
>  static void hotkey_poll_set_freq(unsigned int freq)
> @@ -3077,13 +3067,11 @@ static void tpacpi_send_radiosw_update(void)
>  
>  	/* Issue rfkill input event for WLSW switch */
>  	if (!(wlsw < 0)) {
> -		mutex_lock(&tpacpi_inputdev_send_mutex);
> +		guard(mutex)(&tpacpi_inputdev_send_mutex);
>  
>  		input_report_switch(tpacpi_inputdev,
>  				    SW_RFKILL_ALL, (wlsw > 0));
>  		input_sync(tpacpi_inputdev);
> -
> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
>  	}
>  
>  	/*
> @@ -3095,7 +3083,7 @@ static void tpacpi_send_radiosw_update(void)
>  
>  static void hotkey_exit(void)
>  {
> -	mutex_lock(&hotkey_mutex);
> +	guard(mutex)(&hotkey_mutex);
>  	hotkey_poll_stop_sync();
>  	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
>  		   "restoring original HKEY status and mask\n");
> @@ -3105,8 +3093,6 @@ static void hotkey_exit(void)
>  	      hotkey_mask_set(hotkey_orig_mask)) |
>  	     hotkey_status_set(false)) != 0)
>  		pr_err("failed to restore hot key mask to BIOS defaults\n");
> -
> -	mutex_unlock(&hotkey_mutex);
>  }
>  
>  /*
> @@ -3423,11 +3409,11 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  	if (tp_features.hotkey_mask) {
>  		/* hotkey_source_mask *must* be zero for
>  		 * the first hotkey_mask_get to return hotkey_orig_mask */
> -		mutex_lock(&hotkey_mutex);
> -		res = hotkey_mask_get();
> -		mutex_unlock(&hotkey_mutex);
> -		if (res)
> -			return res;
> +		scoped_guard(mutex, &hotkey_mutex) {
> +			res = hotkey_mask_get();
> +			if (res)
> +				return res;
> +		}
>  
>  		hotkey_orig_mask = hotkey_acpi_mask;
>  	} else {
> @@ -3526,11 +3512,11 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  		hotkey_exit();
>  		return res;
>  	}
> -	mutex_lock(&hotkey_mutex);
> -	res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
> -			       | hotkey_driver_mask)
> -			      & ~hotkey_source_mask);
> -	mutex_unlock(&hotkey_mutex);
> +	scoped_guard(mutex, &hotkey_mutex) {
> +		res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
> +				       | hotkey_driver_mask)
> +				      & ~hotkey_source_mask);
> +	}
>  	if (res < 0 && res != -ENXIO) {
>  		hotkey_exit();
>  		return res;
> @@ -3977,11 +3963,11 @@ static void hotkey_resume(void)
>  {
>  	tpacpi_disable_brightness_delay();
>  
> -	mutex_lock(&hotkey_mutex);
> -	if (hotkey_status_set(true) < 0 ||
> -	    hotkey_mask_set(hotkey_acpi_mask) < 0)
> -		pr_err("error while attempting to reset the event firmware interface\n");
> -	mutex_unlock(&hotkey_mutex);
> +	scoped_guard(mutex, &hotkey_mutex) {
> +		if (hotkey_status_set(true) < 0 ||
> +		    hotkey_mask_set(hotkey_acpi_mask) < 0)
> +			pr_err("error while attempting to reset the event firmware interface\n");
> +	}
>  
>  	tpacpi_send_radiosw_update();
>  	tpacpi_input_send_tabletsw();
> @@ -5034,21 +5020,16 @@ static DEFINE_MUTEX(kbdlight_mutex);
>  
>  static int kbdlight_set_level(int level)
>  {
> -	int ret = 0;
> -
>  	if (!hkey_handle)
>  		return -ENXIO;
>  
> -	mutex_lock(&kbdlight_mutex);
> +	guard(mutex)(&kbdlight_mutex);
>  
>  	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
> -		ret = -EIO;
> -	else
> -		kbdlight_brightness = level;
> -
> -	mutex_unlock(&kbdlight_mutex);
> +		return -EIO;
>  
> -	return ret;
> +	kbdlight_brightness = level;
> +	return 0;
>  }
>  
>  static int kbdlight_get_level(void)
> @@ -10103,9 +10084,8 @@ static void lcdshadow_resume(void)
>  	if (!lcdshadow_dev)
>  		return;
>  
> -	mutex_lock(&lcdshadow_dev->lock);
> +	guard(mutex)(&lcdshadow_dev->lock);
>  	lcdshadow_set_sw_state(lcdshadow_dev, lcdshadow_dev->sw_state);
> -	mutex_unlock(&lcdshadow_dev->lock);
>  }
>  
>  static int lcdshadow_read(struct seq_file *m)
> @@ -10137,9 +10117,8 @@ static int lcdshadow_write(char *buf)
>  	if (state >= 2 || state < 0)
>  		return -EINVAL;
>  
> -	mutex_lock(&lcdshadow_dev->lock);
> -	res = lcdshadow_set_sw_state(lcdshadow_dev, state);
> -	mutex_unlock(&lcdshadow_dev->lock);
> +	scoped_guard(mutex, &lcdshadow_dev->lock)
> +		res = lcdshadow_set_sw_state(lcdshadow_dev, state);
>  
>  	drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
>  
> @@ -10603,26 +10582,26 @@ static const struct platform_profile_ops dytc_profile_ops = {
>  static void dytc_profile_refresh(void)
>  {
>  	enum platform_profile_option profile;
> -	int output = 0, err = 0;
> +	int output = 0, err;
>  	int perfmode, funcmode = 0;
>  
> -	mutex_lock(&dytc_mutex);
> -	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
> -		if (dytc_mmc_get_available)
> -			err = dytc_command(DYTC_CMD_MMC_GET, &output);
> -		else
> -			err = dytc_cql_command(DYTC_CMD_GET, &output);
> -		funcmode = DYTC_FUNCTION_MMC;
> -	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
> -		err = dytc_command(DYTC_CMD_GET, &output);
> -		/* Check if we are PSC mode, or have AMT enabled */
> -		funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> -	} else { /* Unknown profile mode */
> -		err = -ENODEV;
> +	scoped_guard(mutex, &dytc_mutex) {
> +		if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
> +			if (dytc_mmc_get_available)
> +				err = dytc_command(DYTC_CMD_MMC_GET, &output);
> +			else
> +				err = dytc_cql_command(DYTC_CMD_GET, &output);
> +			funcmode = DYTC_FUNCTION_MMC;
> +		} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) {
> +			err = dytc_command(DYTC_CMD_GET, &output);
> +			/* Check if we are PSC mode, or have AMT enabled */
> +			funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> +		} else { /* Unknown profile mode */
> +			err = -ENODEV;
> +		}
> +		if (err)
> +			return;
>  	}
> -	mutex_unlock(&dytc_mutex);
> -	if (err)
> -		return;
>  
>  	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>  	err = convert_dytc_to_profile(funcmode, perfmode, &profile);
> @@ -11425,7 +11404,7 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
>  		if (tp_features.kbdlight) {
>  			enum led_brightness brightness;
>  
> -			mutex_lock(&kbdlight_mutex);
> +			guard(mutex)(&kbdlight_mutex);
>  
>  			/*
>  			 * Check the brightness actually changed, setting the brightness
> @@ -11437,8 +11416,6 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
>  				led_classdev_notify_brightness_hw_changed(
>  					&tpacpi_led_kbdlight.led_classdev, brightness);
>  			}
> -
> -			mutex_unlock(&kbdlight_mutex);
>  		}
>  		/* Key events are suppressed by default hotkey_user_mask */
>  		return false;
> @@ -11460,11 +11437,11 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
>  			enum drm_privacy_screen_status old_hw_state;
>  			bool changed;
>  
> -			mutex_lock(&lcdshadow_dev->lock);
> -			old_hw_state = lcdshadow_dev->hw_state;
> -			lcdshadow_get_hw_state(lcdshadow_dev);
> -			changed = lcdshadow_dev->hw_state != old_hw_state;
> -			mutex_unlock(&lcdshadow_dev->lock);
> +			scoped_guard(mutex, &lcdshadow_dev->lock) {
> +				old_hw_state = lcdshadow_dev->hw_state;
> +				lcdshadow_get_hw_state(lcdshadow_dev);
> +				changed = lcdshadow_dev->hw_state != old_hw_state;
> +			}
>  
>  			if (changed)
>  				drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
> @@ -11485,12 +11462,10 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
>  			pr_err("Error retrieving camera shutter state after shutter event\n");
>  			return true;
>  		}
> -		mutex_lock(&tpacpi_inputdev_send_mutex);
> -
> -		input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, camera_shutter_state);
> -		input_sync(tpacpi_inputdev);
> -
> -		mutex_unlock(&tpacpi_inputdev_send_mutex);
> +		scoped_guard(mutex, &tpacpi_inputdev_send_mutex) {
> +			input_report_switch(tpacpi_inputdev, SW_CAMERA_LENS_COVER, camera_shutter_state);
> +			input_sync(tpacpi_inputdev);
> +		}
>  		return true;
>  	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
>  		/* Toggle kernel-level doubletap event filtering */
> 

-- 
 i.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-09-15 11:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-06  6:19 [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Dmitry Torokhov
2026-08-06  6:19 ` [PATCH 2/3] platform/x86: thinkpad_acpi: convert conditional mutex locks to ACQUIRE_ERR() Dmitry Torokhov
2026-08-12 18:53   ` Mark Pearson
2026-09-06 13:10   ` Ali Ahmet Memiş
2026-08-06  6:19 ` [PATCH 3/3] platform/x86: thinkpad_acpi: use __free(kfree) for automatic cleanup Dmitry Torokhov
2026-08-12 19:02   ` Mark Pearson
2026-09-06 13:10   ` Ali Ahmet Memiş
2026-08-12 18:29 ` [PATCH 1/3] platform/x86: thinkpad_acpi: convert mutex_lock() to guard(mutex) Mark Pearson
2026-08-12 19:55   ` [ibm-acpi-devel] " Mark Pearson
2026-09-06 13:10 ` Ali Ahmet Memiş
2026-09-15 11:33 ` Ilpo Järvinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®