diff mbox series

[v2,4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands

Message ID 70d3957b315815085cdd8cb04b002cdb4a372ddc.1721258854.git.soyer@irl.hu (mailing list archive)
State Superseded, archived
Headers show
Series platform/x86: ideapad-laptop: synchronize VPC commands | expand

Commit Message

Gergo Koteles July 18, 2024, 7:27 a.m. UTC 
Calling VPC commands consists of several VPCW and VPCR ACPI calls.
These calls and their results can get mixed up if they are called
simultaneously from different threads, like acpi notify handler,
sysfs, debugfs, notification chain.

Add a mutex to synchronize VPC commands.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/platform/x86/ideapad-laptop.c | 62 +++++++++++++++++++--------
 1 file changed, 45 insertions(+), 17 deletions(-)

Comments

Ilpo Järvinen July 18, 2024, 8:06 a.m. UTC | #1 
On Thu, 18 Jul 2024, Gergo Koteles wrote:

> Calling VPC commands consists of several VPCW and VPCR ACPI calls.
> These calls and their results can get mixed up if they are called
> simultaneously from different threads, like acpi notify handler,
> sysfs, debugfs, notification chain.
> 
> Add a mutex to synchronize VPC commands.
> 
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> ---

> @@ -2027,6 +2053,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  	priv->adev = adev;
>  	priv->platform_device = pdev;
>  
> +	mutex_init(&priv->vpc_mutex);
> +
>  	ideapad_check_features(priv);
>  
>  	err = ideapad_sysfs_init(priv);

mutex_destroy() missing from rollback and ideapad_acpi_remove().
Hans de Goede July 18, 2024, 8:12 a.m. UTC | #2 
Hi,

On 7/18/24 10:06 AM, Ilpo Järvinen wrote:
> On Thu, 18 Jul 2024, Gergo Koteles wrote:
> 
>> Calling VPC commands consists of several VPCW and VPCR ACPI calls.
>> These calls and their results can get mixed up if they are called
>> simultaneously from different threads, like acpi notify handler,
>> sysfs, debugfs, notification chain.
>>
>> Add a mutex to synchronize VPC commands.
>>
>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
>> ---
> 
>> @@ -2027,6 +2053,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>>  	priv->adev = adev;
>>  	priv->platform_device = pdev;
>>  
>> +	mutex_init(&priv->vpc_mutex);
>> +
>>  	ideapad_check_features(priv);
>>  
>>  	err = ideapad_sysfs_init(priv);
> 
> mutex_destroy() missing from rollback and ideapad_acpi_remove().

Right, note the easiest way to fix this is to use the new devm_mutex_init()
instead of plain mutex_init() that will also take care of destroying the mutex
on any exit-on-error cases from probe().

Regards,

Hans
Ilpo Järvinen July 18, 2024, 8:17 a.m. UTC | #3 
On Thu, 18 Jul 2024, Hans de Goede wrote:

> Hi,
> 
> On 7/18/24 10:06 AM, Ilpo Järvinen wrote:
> > On Thu, 18 Jul 2024, Gergo Koteles wrote:
> > 
> >> Calling VPC commands consists of several VPCW and VPCR ACPI calls.
> >> These calls and their results can get mixed up if they are called
> >> simultaneously from different threads, like acpi notify handler,
> >> sysfs, debugfs, notification chain.
> >>
> >> Add a mutex to synchronize VPC commands.
> >>
> >> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> >> ---
> > 
> >> @@ -2027,6 +2053,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> >>  	priv->adev = adev;
> >>  	priv->platform_device = pdev;
> >>  
> >> +	mutex_init(&priv->vpc_mutex);
> >> +
> >>  	ideapad_check_features(priv);
> >>  
> >>  	err = ideapad_sysfs_init(priv);
> > 
> > mutex_destroy() missing from rollback and ideapad_acpi_remove().
> 
> Right, note the easiest way to fix this is to use the new devm_mutex_init()
> instead of plain mutex_init() that will also take care of destroying the mutex
> on any exit-on-error cases from probe().

Right, of course. I though one existed but it seems I tried to grep for 
dev_mutex_init() while searching for it... :-/
diff mbox series

Patch

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 66b34e99147e..d2e7dd5027b8 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -154,6 +154,7 @@  struct ideapad_rfk_priv {
 
 struct ideapad_private {
 	struct acpi_device *adev;
+	struct mutex vpc_mutex; /* protects the VPC calls */
 	struct rfkill *rfk[IDEAPAD_RFKILL_DEV_NUM];
 	struct ideapad_rfk_priv rfk_priv[IDEAPAD_RFKILL_DEV_NUM];
 	struct platform_device *platform_device;
@@ -435,6 +436,8 @@  static int debugfs_status_show(struct seq_file *s, void *data)
 	struct ideapad_private *priv = s->private;
 	unsigned long value;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	if (!read_ec_data(priv->adev->handle, VPCCMD_R_BL_MAX, &value))
 		seq_printf(s, "Backlight max:  %lu\n", value);
 	if (!read_ec_data(priv->adev->handle, VPCCMD_R_BL, &value))
@@ -553,7 +556,8 @@  static ssize_t camera_power_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
 	if (err)
 		return err;
 
@@ -572,7 +576,8 @@  static ssize_t camera_power_store(struct device *dev,
 	if (err)
 		return err;
 
-	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
 	if (err)
 		return err;
 
@@ -625,7 +630,8 @@  static ssize_t fan_mode_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
 	if (err)
 		return err;
 
@@ -647,7 +653,8 @@  static ssize_t fan_mode_store(struct device *dev,
 	if (state > 4 || state == 3)
 		return -EINVAL;
 
-	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
 	if (err)
 		return err;
 
@@ -700,7 +707,8 @@  static ssize_t touchpad_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
 	if (err)
 		return err;
 
@@ -721,7 +729,8 @@  static ssize_t touchpad_store(struct device *dev,
 	if (err)
 		return err;
 
-	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
 	if (err)
 		return err;
 
@@ -1118,6 +1127,8 @@  static int ideapad_rfk_set(void *data, bool blocked)
 	struct ideapad_rfk_priv *priv = data;
 	int opcode = ideapad_rfk_data[priv->dev].opcode;
 
+	guard(mutex)(&priv->priv->vpc_mutex);
+
 	return write_ec_cmd(priv->priv->adev->handle, opcode, !blocked);
 }
 
@@ -1131,6 +1142,8 @@  static void ideapad_sync_rfk_state(struct ideapad_private *priv)
 	int i;
 
 	if (priv->features.hw_rfkill_switch) {
+		guard(mutex)(&priv->vpc_mutex);
+
 		if (read_ec_data(priv->adev->handle, VPCCMD_R_RF, &hw_blocked))
 			return;
 		hw_blocked = !hw_blocked;
@@ -1302,8 +1315,9 @@  static void ideapad_input_novokey(struct ideapad_private *priv)
 {
 	unsigned long long_pressed;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_NOVO, &long_pressed))
-		return;
+	scoped_guard(mutex, &priv->vpc_mutex)
+		if (read_ec_data(priv->adev->handle, VPCCMD_R_NOVO, &long_pressed))
+			return;
 
 	if (long_pressed)
 		ideapad_input_report(priv, 17);
@@ -1315,8 +1329,9 @@  static void ideapad_check_special_buttons(struct ideapad_private *priv)
 {
 	unsigned long bit, value;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value))
-		return;
+	scoped_guard(mutex, &priv->vpc_mutex)
+		if (read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value))
+			return;
 
 	for_each_set_bit (bit, &value, 16) {
 		switch (bit) {
@@ -1346,6 +1361,8 @@  static int ideapad_backlight_get_brightness(struct backlight_device *blightdev)
 	unsigned long now;
 	int err;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	err = read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
 	if (err)
 		return err;
@@ -1358,6 +1375,8 @@  static int ideapad_backlight_update_status(struct backlight_device *blightdev)
 	struct ideapad_private *priv = bl_get_data(blightdev);
 	int err;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_BL,
 			   blightdev->props.brightness);
 	if (err)
@@ -1435,6 +1454,8 @@  static void ideapad_backlight_notify_power(struct ideapad_private *priv)
 	if (!blightdev)
 		return;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	if (read_ec_data(priv->adev->handle, VPCCMD_R_BL_POWER, &power))
 		return;
 
@@ -1447,7 +1468,8 @@  static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
 
 	/* if we control brightness via acpi video driver */
 	if (!priv->blightdev)
-		read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
+		scoped_guard(mutex, &priv->vpc_mutex)
+			read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
 	else
 		backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY);
 }
@@ -1613,7 +1635,8 @@  static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
 	int ret;
 
 	/* Without reading from EC touchpad LED doesn't switch state */
-	ret = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		ret = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value);
 	if (ret)
 		return;
 
@@ -1673,7 +1696,8 @@  static void ideapad_laptop_trigger_ec(void)
 	if (!priv->features.ymc_ec_trigger)
 		return;
 
-	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
 	if (ret)
 		dev_warn(&priv->platform_device->dev, "Could not write YMC: %d\n", ret);
 }
@@ -1719,11 +1743,13 @@  static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 	struct ideapad_private *priv = data;
 	unsigned long vpc1, vpc2, bit;
 
-	if (read_ec_data(handle, VPCCMD_R_VPC1, &vpc1))
-		return;
+	scoped_guard(mutex, &priv->vpc_mutex) {
+		if (read_ec_data(handle, VPCCMD_R_VPC1, &vpc1))
+			return;
 
-	if (read_ec_data(handle, VPCCMD_R_VPC2, &vpc2))
-		return;
+		if (read_ec_data(handle, VPCCMD_R_VPC2, &vpc2))
+			return;
+	}
 
 	vpc1 = (vpc2 << 8) | vpc1;
 
@@ -2027,6 +2053,8 @@  static int ideapad_acpi_add(struct platform_device *pdev)
 	priv->adev = adev;
 	priv->platform_device = pdev;
 
+	mutex_init(&priv->vpc_mutex);
+
 	ideapad_check_features(priv);
 
 	err = ideapad_sysfs_init(priv);