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

[Ilpo Järvinen]

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@xxxxxx>

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).


-- 
 i.


> ---
>  drivers/platform/x86/ideapad-laptop.c | 64 ++++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 17 deletions(-)
> 
> 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);
>