[PATCH 0/2] platform/x86: thinkpad_acpi: lockdep support

[PATCH 0/2] platform/x86: thinkpad_acpi: lockdep support

[Thomas Weißschuh]

Validate locking requirements in thinkpad_acpi through lockdep.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (2):
      platform/x86: thinkpad_acpi: take mutex for hotkey_mask_{set,get}
      platform/x86: thinkpad_acpi: use lockdep annotations

 drivers/platform/x86/thinkpad_acpi.c | 45 ++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)
---
base-commit: 03275585cabd0240944f19f33d7584a1b099a3a8
change-id: 20230704-thinkpad_acpi-lockdep-7def9d7f91b5

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/2] platform/x86: thinkpad_acpi: take mutex for hotkey_mask_{set,get}

[Thomas Weißschuh]

hotkey_mask_set() and hotkey_mask_get() expect hotkey_mutex to be held.
While it should not matter during initialization a following patch will
enable lockdep for hotkey_mutex which would produce warnings here.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/thinkpad_acpi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 187018ffb068..b796e65c99e0 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3478,7 +3478,9 @@ 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;
 
@@ -3577,9 +3579,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);
 	if (res < 0 && res != -ENXIO) {
 		hotkey_exit();
 		return res;

-- 
2.41.0

[PATCH 2/2] platform/x86: thinkpad_acpi: use lockdep annotations

[Thomas Weißschuh]

Instead of relying on free-form comments to document locking
requirements use well-known lockdep annotations.
These can also be validated at runtime make sure the invariants are not
violated.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/thinkpad_acpi.c | 41 +++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index b796e65c99e0..25cc86a4a36a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -50,6 +50,7 @@
 #include <linux/kthread.h>
 #include <linux/leds.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/nvram.h>
@@ -2071,11 +2072,11 @@ static int hotkey_get_tablet_mode(int *status)
  * hotkey_acpi_mask accordingly.  Also resets any bits
  * from hotkey_user_mask that are unavailable to be
  * delivered (shadow requirement of the userspace ABI).
- *
- * Call with hotkey_mutex held
  */
 static int hotkey_mask_get(void)
 {
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (tp_features.hotkey_mask) {
 		u32 m = 0;
 
@@ -2111,8 +2112,6 @@ static void hotkey_mask_warn_incomplete_mask(void)
  * Also calls hotkey_mask_get to update hotkey_acpi_mask.
  *
  * NOTE: does not set bits in hotkey_user_mask, but may reset them.
- *
- * Call with hotkey_mutex held
  */
 static int hotkey_mask_set(u32 mask)
 {
@@ -2121,6 +2120,8 @@ static int hotkey_mask_set(u32 mask)
 
 	const u32 fwmask = mask & ~hotkey_source_mask;
 
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (tp_features.hotkey_mask) {
 		for (i = 0; i < 32; i++) {
 			if (!acpi_evalf(hkey_handle,
@@ -2152,13 +2153,13 @@ static int hotkey_mask_set(u32 mask)
 
 /*
  * Sets hotkey_user_mask and tries to set the firmware mask
- *
- * Call with hotkey_mutex held
  */
 static int hotkey_user_mask_set(const u32 mask)
 {
 	int rc;
 
+	lockdep_assert_held(&hotkey_mutex);
+
 	/* Give people a chance to notice they are doing something that
 	 * is bound to go boom on their users sooner or later */
 	if (!tp_warned.hotkey_mask_ff &&
@@ -2519,21 +2520,23 @@ static int hotkey_kthread(void *data)
 	return 0;
 }
 
-/* call with hotkey_mutex held */
 static void hotkey_poll_stop_sync(void)
 {
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (tpacpi_hotkey_task) {
 		kthread_stop(tpacpi_hotkey_task);
 		tpacpi_hotkey_task = NULL;
 	}
 }
 
-/* call with hotkey_mutex held */
 static void hotkey_poll_setup(const bool may_warn)
 {
 	const u32 poll_driver_mask = hotkey_driver_mask & hotkey_source_mask;
 	const u32 poll_user_mask = hotkey_user_mask & hotkey_source_mask;
 
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (hotkey_poll_freq > 0 &&
 	    (poll_driver_mask ||
 	     (poll_user_mask && tpacpi_inputdev->users > 0))) {
@@ -2562,9 +2565,10 @@ static void hotkey_poll_setup_safe(const bool may_warn)
 	mutex_unlock(&hotkey_mutex);
 }
 
-/* call with hotkey_mutex held */
 static void hotkey_poll_set_freq(unsigned int freq)
 {
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (!freq)
 		hotkey_poll_stop_sync();
 
@@ -6664,12 +6668,13 @@ static unsigned int brightness_enable = 2; /* 2 = auto, 0 = no, 1 = yes */
 
 static struct mutex brightness_mutex;
 
-/* NVRAM brightness access,
- * call with brightness_mutex held! */
+/* NVRAM brightness access */
 static unsigned int tpacpi_brightness_nvram_get(void)
 {
 	u8 lnvram;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	lnvram = (nvram_read_byte(TP_NVRAM_ADDR_BRIGHTNESS)
 		  & TP_NVRAM_MASK_LEVEL_BRIGHTNESS)
 		  >> TP_NVRAM_POS_LEVEL_BRIGHTNESS;
@@ -6717,11 +6722,12 @@ static void tpacpi_brightness_checkpoint_nvram(void)
 }
 
 
-/* call with brightness_mutex held! */
 static int tpacpi_brightness_get_raw(int *status)
 {
 	u8 lec = 0;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	switch (brightness_mode) {
 	case TPACPI_BRGHT_MODE_UCMS_STEP:
 		*status = tpacpi_brightness_nvram_get();
@@ -6737,12 +6743,13 @@ static int tpacpi_brightness_get_raw(int *status)
 	}
 }
 
-/* call with brightness_mutex held! */
 /* do NOT call with illegal backlight level value */
 static int tpacpi_brightness_set_ec(unsigned int value)
 {
 	u8 lec = 0;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	if (unlikely(!acpi_ec_read(TP_EC_BACKLIGHT, &lec)))
 		return -EIO;
 
@@ -6754,12 +6761,13 @@ static int tpacpi_brightness_set_ec(unsigned int value)
 	return 0;
 }
 
-/* call with brightness_mutex held! */
 static int tpacpi_brightness_set_ucmsstep(unsigned int value)
 {
 	int cmos_cmd, inc;
 	unsigned int current_value, i;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	current_value = tpacpi_brightness_nvram_get();
 
 	if (value == current_value)
@@ -8208,11 +8216,10 @@ static bool fan_select_fan2(void)
 	return true;
 }
 
-/*
- * Call with fan_mutex held
- */
 static void fan_update_desired_level(u8 status)
 {
+	lockdep_assert_held(&fan_mutex);
+
 	if ((status &
 	     (TP_EC_FAN_AUTO | TP_EC_FAN_FULLSPEED)) == 0) {
 		if (status > 7)

-- 
2.41.0

Re: [PATCH 0/2] platform/x86: thinkpad_acpi: lockdep support

[Hans de Goede]

Hi,

On 7/4/23 23:03, Thomas Weißschuh wrote:
> Validate locking requirements in thinkpad_acpi through lockdep.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
> Thomas Weißschuh (2):
>       platform/x86: thinkpad_acpi: take mutex for hotkey_mask_{set,get}
>       platform/x86: thinkpad_acpi: use lockdep annotations
> 
>  drivers/platform/x86/thinkpad_acpi.c | 45 ++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> ---
> base-commit: 03275585cabd0240944f19f33d7584a1b099a3a8
> change-id: 20230704-thinkpad_acpi-lockdep-7def9d7f91b5
> 
> Best regards,