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