diff mbox series

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

Message ID f26782fa1194ad11ed5d9ba121a804e59b58b026.1721898747.git.soyer@irl.hu (mailing list archive)
State Accepted, archived
Delegated to: Ilpo Järvinen
Headers show
Series platform/x86: ideapad-laptop: synchronize VPC commands | expand

Commit Message

Gergo Koteles July 25, 2024, 9:21 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 | 64 ++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 17 deletions(-)

Comments

Ilpo Järvinen July 30, 2024, 1:37 p.m. UTC | #1 
On Thu, 25 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>

What commit does this fix? I was going to add a Fixes tag myself while 
applying this but wasn't sure if it should be the ACPI concurrency commit 
e2ffcda16290 or the change introducing lenovo-ymc driver?

Also, I'd prefer to not take the move patch (PATCH 3/4) now so I could 
take this through fixes branch since it causes a real issue if I remember 
the earlier discussions right? Do you think there's any issue if I take 
only patches 1, 2, and 4? They seemed to apply without conflicts when I 
tried to apply the entire series and then cherrypicked the last patch 
dropping the third patch.

The code movement patch could go through for-next fixes branch is then 
merged into it (or after one kernel cycle).
Gergo Koteles July 30, 2024, 4 p.m. UTC | #2 
Hi Ilpo,

On Tue, 2024-07-30 at 16:37 +0300, Ilpo Järvinen wrote:
> On Thu, 25 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>
> 
> What commit does this fix? I was going to add a Fixes tag myself while 
> applying this but wasn't sure if it should be the ACPI concurrency commit 
> e2ffcda16290 or the change introducing lenovo-ymc driver?
> 

YMC triggering works in 6.7, but not reliably in 6.8. So I assume the
culprit is e2ffcda16290.

But in theory debugfs, sysfs, acpi notify handler can race with each
other in the same way for 10+ years. Technically, probably not.

> Also, I'd prefer to not take the move patch (PATCH 3/4) now so I could 
> take this through fixes branch since it causes a real issue if I remember 
> the earlier discussions right? Do you think there's any issue if I take 
> only patches 1, 2, and 4? They seemed to apply without conflicts when I 
> tried to apply the entire series and then cherrypicked the last patch 
> dropping the third patch.
> 

Yes, this is a real issue.

You can skip the third patch. The series compiles and works fine
without it.

> The code movement patch could go through for-next fixes branch is then 
> merged into it (or after one kernel cycle).
> 
> 

Fine.

Thanks,
Gergo
Ilpo Järvinen Aug. 8, 2024, 12:16 p.m. UTC | #3 
On Tue, 30 Jul 2024, Gergo Koteles wrote:
> On Tue, 2024-07-30 at 16:37 +0300, Ilpo Järvinen wrote:
> > On Thu, 25 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>
> > 
> > What commit does this fix? I was going to add a Fixes tag myself while 
> > applying this but wasn't sure if it should be the ACPI concurrency commit 
> > e2ffcda16290 or the change introducing lenovo-ymc driver?
> > 
> 
> YMC triggering works in 6.7, but not reliably in 6.8. So I assume the
> culprit is e2ffcda16290.
>
> But in theory debugfs, sysfs, acpi notify handler can race with each
> other in the same way for 10+ years. Technically, probably not.

Okay, I decided to put both commits then as the ACPI thing made it much 
worse so it's proper to assign some "blame" to it for the additional 
problems it caused ;-). I also wrote an additional paragraph about it
into the commit message.

> > Also, I'd prefer to not take the move patch (PATCH 3/4) now so I could 
> > take this through fixes branch since it causes a real issue if I remember 
> > the earlier discussions right? Do you think there's any issue if I take 
> > only patches 1, 2, and 4? They seemed to apply without conflicts when I 
> > tried to apply the entire series and then cherrypicked the last patch 
> > dropping the third patch.
> > 
> 
> Yes, this is a real issue.
> 
> You can skip the third patch. The series compiles and works fine
> without it.
> 
> > The code movement patch could go through for-next fixes branch is then 
> > merged into it (or after one kernel cycle).
> > 
> > 
> 
> Fine.

I've taken patches 1, 2, and 4 into review-ilpo and will propagate them 
into fixes branch once LKP has build tested the branch.

Thanks for the patches!


As mentioned patch 3 should go to for-next which is handled by Hans in 
this cycle. It requires merging the fixes branch (or the fixes PR tag) 
into for-next once the other commits have migrated into fixes. I'll 
reassign patch 3 to Hans in patchwork once I've tagged the PR to Linus.
diff mbox series

Patch

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 8398774cdfe2..3c24e3d99cd2 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -155,6 +155,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;
@@ -437,6 +438,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))
@@ -555,7 +558,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;
 
@@ -574,7 +578,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;
 
@@ -627,7 +632,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;
 
@@ -649,7 +655,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;
 
@@ -734,7 +741,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;
 
@@ -755,7 +763,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;
 
@@ -1148,6 +1157,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);
 }
 
@@ -1161,6 +1172,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;
@@ -1334,8 +1347,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);
@@ -1347,8 +1361,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) {
@@ -1381,6 +1396,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;
@@ -1393,6 +1410,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)
@@ -1470,6 +1489,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;
 
@@ -1482,7 +1503,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);
 }
@@ -1707,7 +1729,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;
 
@@ -1767,7 +1790,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);
 }
@@ -1813,11 +1837,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;
 
@@ -2124,6 +2150,10 @@  static int ideapad_acpi_add(struct platform_device *pdev)
 	priv->adev = adev;
 	priv->platform_device = pdev;
 
+	err = devm_mutex_init(&pdev->dev, &priv->vpc_mutex);
+	if (err)
+		return err;
+
 	ideapad_check_features(priv);
 
 	err = ideapad_sysfs_init(priv);