* [PATCH v4 1/4] platform/x86: ideapad-laptop: introduce a generic notification chain
2024-07-25 9:21 [PATCH v4 0/4] platform/x86: ideapad-laptop: synchronize VPC commands Gergo Koteles
@ 2024-07-25 9:21 ` Gergo Koteles
2024-07-25 9:21 ` [PATCH v4 2/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc Gergo Koteles
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Gergo Koteles @ 2024-07-25 9:21 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Ike Panhc
Cc: platform-driver-x86, linux-kernel, Gergo Koteles
There are several cases where a notification chain can simplify Lenovo
WMI drivers.
Add a generic notification chain into ideapad-laptop.
Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
drivers/platform/x86/ideapad-laptop.c | 37 +++++++++++++++++++++++++++
drivers/platform/x86/ideapad-laptop.h | 5 ++++
2 files changed, 42 insertions(+)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 1ace711f7442..866b32bfe2c9 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1592,6 +1592,39 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
priv->r_touchpad_val = value;
}
+static int ideapad_laptop_nb_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ switch (action) {
+ }
+
+ return 0;
+}
+
+static struct notifier_block ideapad_laptop_notifier = {
+ .notifier_call = ideapad_laptop_nb_notify,
+};
+
+static BLOCKING_NOTIFIER_HEAD(ideapad_laptop_chain_head);
+
+int ideapad_laptop_register_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&ideapad_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_NS_GPL(ideapad_laptop_register_notifier, IDEAPAD_LAPTOP);
+
+int ideapad_laptop_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&ideapad_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_NS_GPL(ideapad_laptop_unregister_notifier, IDEAPAD_LAPTOP);
+
+void ideapad_laptop_call_notifier(unsigned long action, void *data)
+{
+ blocking_notifier_call_chain(&ideapad_laptop_chain_head, action, data);
+}
+EXPORT_SYMBOL_NS_GPL(ideapad_laptop_call_notifier, IDEAPAD_LAPTOP);
+
static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
{
struct ideapad_private *priv = data;
@@ -1974,6 +2007,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
if (err)
goto shared_init_failed;
+ ideapad_laptop_register_notifier(&ideapad_laptop_notifier);
+
return 0;
shared_init_failed:
@@ -2006,6 +2041,8 @@ static void ideapad_acpi_remove(struct platform_device *pdev)
struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
int i;
+ ideapad_laptop_unregister_notifier(&ideapad_laptop_notifier);
+
ideapad_shared_exit(priv);
acpi_remove_notify_handler(priv->adev->handle,
diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
index 4498a96de597..3eb0dcd6bf7b 100644
--- a/drivers/platform/x86/ideapad-laptop.h
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -12,6 +12,11 @@
#include <linux/acpi.h>
#include <linux/jiffies.h>
#include <linux/errno.h>
+#include <linux/notifier.h>
+
+int ideapad_laptop_register_notifier(struct notifier_block *nb);
+int ideapad_laptop_unregister_notifier(struct notifier_block *nb);
+void ideapad_laptop_call_notifier(unsigned long action, void *data);
enum {
VPCCMD_R_VPC1 = 0x10,
--
2.45.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v4 2/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc
2024-07-25 9:21 [PATCH v4 0/4] platform/x86: ideapad-laptop: synchronize VPC commands Gergo Koteles
2024-07-25 9:21 ` [PATCH v4 1/4] platform/x86: ideapad-laptop: introduce a generic notification chain Gergo Koteles
@ 2024-07-25 9:21 ` Gergo Koteles
2024-07-25 9:21 ` [PATCH v4 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file Gergo Koteles
2024-07-25 9:21 ` [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands Gergo Koteles
3 siblings, 0 replies; 9+ messages in thread
From: Gergo Koteles @ 2024-07-25 9:21 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Ike Panhc
Cc: platform-driver-x86, linux-kernel, Gergo Koteles
Some models need to trigger the EC after each YMC event for the yoga
mode control to work properly. EC triggering consist of a VPC call from
the lenovo-ymc module. Except for this, all VPC calls are in the
ideapad-laptop module.
Since ideapad-laptop has a notification chain, a new YMC_EVENT action
can be added and triggered from the lenovo-ymc module. Then the
ideapad-laptop can trigger the EC.
If the triggering is in the ideapad-laptop module, then the ec_trigger
module parameter should be there as well.
Move the ymc_trigger_ec functionality and the ec_trigger module
parameter to the ideapad-laptop module.
Signed-off-by: Gergo Koteles <soyer@irl.hu>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/platform/x86/Kconfig | 1 +
drivers/platform/x86/ideapad-laptop.c | 49 ++++++++++++++++++++++
drivers/platform/x86/ideapad-laptop.h | 4 ++
drivers/platform/x86/lenovo-ymc.c | 60 +--------------------------
4 files changed, 56 insertions(+), 58 deletions(-)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0ec952b5d03e..e5e61b8f8adc 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -476,6 +476,7 @@ config LENOVO_YMC
tristate "Lenovo Yoga Tablet Mode Control"
depends on ACPI_WMI
depends on INPUT
+ depends on IDEAPAD_LAPTOP
select INPUT_SPARSEKMAP
help
This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 866b32bfe2c9..9fc1bb990e47 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -146,6 +146,7 @@ struct ideapad_private {
bool touchpad_ctrl_via_ec : 1;
bool ctrl_ps2_aux_port : 1;
bool usb_charging : 1;
+ bool ymc_ec_trigger : 1;
} features;
struct {
bool initialized;
@@ -194,6 +195,12 @@ MODULE_PARM_DESC(touchpad_ctrl_via_ec,
"Enable registering a 'touchpad' sysfs-attribute which can be used to manually "
"tell the EC to enable/disable the touchpad. This may not work on all models.");
+static bool ymc_ec_trigger __read_mostly;
+module_param(ymc_ec_trigger, bool, 0444);
+MODULE_PARM_DESC(ymc_ec_trigger,
+ "Enable EC triggering work-around to force emitting tablet mode events. "
+ "If you need this please report this to: platform-driver-x86@vger.kernel.org");
+
/*
* shared data
*/
@@ -1592,10 +1599,50 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
priv->r_touchpad_val = value;
}
+static const struct dmi_system_id ymc_ec_trigger_quirk_dmi_table[] = {
+ {
+ /* Lenovo Yoga 7 14ARB7 */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
+ },
+ },
+ {
+ /* Lenovo Yoga 7 14ACN6 */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "82N7"),
+ },
+ },
+ { }
+};
+
+static void ideapad_laptop_trigger_ec(void)
+{
+ struct ideapad_private *priv;
+ int ret;
+
+ guard(mutex)(&ideapad_shared_mutex);
+
+ priv = ideapad_shared;
+ if (!priv)
+ return;
+
+ if (!priv->features.ymc_ec_trigger)
+ return;
+
+ 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);
+}
+
static int ideapad_laptop_nb_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
switch (action) {
+ case IDEAPAD_LAPTOP_YMC_EVENT:
+ ideapad_laptop_trigger_ec();
+ break;
}
return 0;
@@ -1761,6 +1808,8 @@ static void ideapad_check_features(struct ideapad_private *priv)
priv->features.ctrl_ps2_aux_port =
ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list);
priv->features.touchpad_ctrl_via_ec = touchpad_ctrl_via_ec;
+ priv->features.ymc_ec_trigger =
+ ymc_ec_trigger || dmi_check_system(ymc_ec_trigger_quirk_dmi_table);
if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
priv->features.fan_mode = true;
diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
index 3eb0dcd6bf7b..948cc61800a9 100644
--- a/drivers/platform/x86/ideapad-laptop.h
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -14,6 +14,10 @@
#include <linux/errno.h>
#include <linux/notifier.h>
+enum ideapad_laptop_notifier_actions {
+ IDEAPAD_LAPTOP_YMC_EVENT,
+};
+
int ideapad_laptop_register_notifier(struct notifier_block *nb);
int ideapad_laptop_unregister_notifier(struct notifier_block *nb);
void ideapad_laptop_call_notifier(unsigned long action, void *data);
diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
index e1fbc35504d4..e0bbd6a14a89 100644
--- a/drivers/platform/x86/lenovo-ymc.c
+++ b/drivers/platform/x86/lenovo-ymc.c
@@ -20,32 +20,10 @@
#define LENOVO_YMC_QUERY_INSTANCE 0
#define LENOVO_YMC_QUERY_METHOD 0x01
-static bool ec_trigger __read_mostly;
-module_param(ec_trigger, bool, 0444);
-MODULE_PARM_DESC(ec_trigger, "Enable EC triggering work-around to force emitting tablet mode events");
-
static bool force;
module_param(force, bool, 0444);
MODULE_PARM_DESC(force, "Force loading on boards without a convertible DMI chassis-type");
-static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
- {
- /* Lenovo Yoga 7 14ARB7 */
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
- DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
- },
- },
- {
- /* Lenovo Yoga 7 14ACN6 */
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
- DMI_MATCH(DMI_PRODUCT_NAME, "82N7"),
- },
- },
- { }
-};
-
static const struct dmi_system_id allowed_chasis_types_dmi_table[] = {
{
.matches = {
@@ -62,21 +40,8 @@ static const struct dmi_system_id allowed_chasis_types_dmi_table[] = {
struct lenovo_ymc_private {
struct input_dev *input_dev;
- struct acpi_device *ec_acpi_dev;
};
-static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
-{
- int err;
-
- if (!priv->ec_acpi_dev)
- return;
-
- err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
- if (err)
- dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
-}
-
static const struct key_entry lenovo_ymc_keymap[] = {
/* Laptop */
{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
@@ -125,11 +90,9 @@ static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
free_obj:
kfree(obj);
- lenovo_ymc_trigger_ec(wdev, priv);
+ ideapad_laptop_call_notifier(IDEAPAD_LAPTOP_YMC_EVENT, &code);
}
-static void acpi_dev_put_helper(void *p) { acpi_dev_put(p); }
-
static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
{
struct lenovo_ymc_private *priv;
@@ -143,29 +106,10 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
return -ENODEV;
}
- ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
-
priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
- if (ec_trigger) {
- pr_debug("Lenovo YMC enable EC triggering.\n");
- priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VPC2004", NULL, -1);
-
- if (!priv->ec_acpi_dev) {
- dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
- return -ENODEV;
- }
- err = devm_add_action_or_reset(&wdev->dev,
- acpi_dev_put_helper, priv->ec_acpi_dev);
- if (err) {
- dev_err(&wdev->dev,
- "Could not clean up EC ACPI device: %d\n", err);
- return err;
- }
- }
-
input_dev = devm_input_allocate_device(&wdev->dev);
if (!input_dev)
return -ENOMEM;
@@ -192,7 +136,6 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
dev_set_drvdata(&wdev->dev, priv);
/* Report the state for the first time on probe */
- lenovo_ymc_trigger_ec(wdev, priv);
lenovo_ymc_notify(wdev, NULL);
return 0;
}
@@ -217,3 +160,4 @@ module_wmi_driver(lenovo_ymc_driver);
MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IDEAPAD_LAPTOP);
--
2.45.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v4 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file
2024-07-25 9:21 [PATCH v4 0/4] platform/x86: ideapad-laptop: synchronize VPC commands Gergo Koteles
2024-07-25 9:21 ` [PATCH v4 1/4] platform/x86: ideapad-laptop: introduce a generic notification chain Gergo Koteles
2024-07-25 9:21 ` [PATCH v4 2/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc Gergo Koteles
@ 2024-07-25 9:21 ` Gergo Koteles
2024-08-12 15:38 ` Hans de Goede
2024-07-25 9:21 ` [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands Gergo Koteles
3 siblings, 1 reply; 9+ messages in thread
From: Gergo Koteles @ 2024-07-25 9:21 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Ike Panhc
Cc: platform-driver-x86, linux-kernel, Gergo Koteles
Since moving ymc_trigger_ec from lenovo-ymc to ideapad-laptop, only the
latter uses these definitions and functions.
Move them back to source file.
Signed-off-by: Gergo Koteles <soyer@irl.hu>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/platform/x86/ideapad-laptop.c | 136 +++++++++++++++++++++++++
drivers/platform/x86/ideapad-laptop.h | 139 --------------------------
2 files changed, 136 insertions(+), 139 deletions(-)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 9fc1bb990e47..8398774cdfe2 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -22,6 +22,7 @@
#include <linux/init.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/leds.h>
#include <linux/module.h>
@@ -87,6 +88,34 @@ enum {
SALS_FNLOCK_OFF = 0xf,
};
+enum {
+ VPCCMD_R_VPC1 = 0x10,
+ VPCCMD_R_BL_MAX,
+ VPCCMD_R_BL,
+ VPCCMD_W_BL,
+ VPCCMD_R_WIFI,
+ VPCCMD_W_WIFI,
+ VPCCMD_R_BT,
+ VPCCMD_W_BT,
+ VPCCMD_R_BL_POWER,
+ VPCCMD_R_NOVO,
+ VPCCMD_R_VPC2,
+ VPCCMD_R_TOUCHPAD,
+ VPCCMD_W_TOUCHPAD,
+ VPCCMD_R_CAMERA,
+ VPCCMD_W_CAMERA,
+ VPCCMD_R_3G,
+ VPCCMD_W_3G,
+ VPCCMD_R_ODD, /* 0x21 */
+ VPCCMD_W_FAN,
+ VPCCMD_R_RF,
+ VPCCMD_W_RF,
+ VPCCMD_W_YMC = 0x2A,
+ VPCCMD_R_FAN = 0x2B,
+ VPCCMD_R_SPECIAL_BUTTONS = 0x31,
+ VPCCMD_W_BL_POWER = 0x33,
+};
+
/*
* These correspond to the number of supported states - 1
* Future keyboard types may need a new system, if there's a collision
@@ -236,6 +265,7 @@ static void ideapad_shared_exit(struct ideapad_private *priv)
/*
* ACPI Helpers
*/
+#define IDEAPAD_EC_TIMEOUT 200 /* in ms */
static int eval_int(acpi_handle handle, const char *name, unsigned long *res)
{
@@ -251,6 +281,29 @@ static int eval_int(acpi_handle handle, const char *name, unsigned long *res)
return 0;
}
+static int eval_int_with_arg(acpi_handle handle, const char *name, unsigned long arg,
+ unsigned long *res)
+{
+ struct acpi_object_list params;
+ unsigned long long result;
+ union acpi_object in_obj;
+ acpi_status status;
+
+ params.count = 1;
+ params.pointer = &in_obj;
+ in_obj.type = ACPI_TYPE_INTEGER;
+ in_obj.integer.value = arg;
+
+ status = acpi_evaluate_integer(handle, (char *)name, ¶ms, &result);
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ if (res)
+ *res = result;
+
+ return 0;
+}
+
static int exec_simple_method(acpi_handle handle, const char *name, unsigned long arg)
{
acpi_status status = acpi_execute_simple_method(handle, (char *)name, arg);
@@ -293,6 +346,89 @@ static int eval_dytc(acpi_handle handle, unsigned long cmd, unsigned long *res)
return eval_int_with_arg(handle, "DYTC", cmd, res);
}
+static int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *res)
+{
+ return eval_int_with_arg(handle, "VPCR", cmd, res);
+}
+
+static int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
+{
+ struct acpi_object_list params;
+ union acpi_object in_obj[2];
+ acpi_status status;
+
+ params.count = 2;
+ params.pointer = in_obj;
+ in_obj[0].type = ACPI_TYPE_INTEGER;
+ in_obj[0].integer.value = cmd;
+ in_obj[1].type = ACPI_TYPE_INTEGER;
+ in_obj[1].integer.value = data;
+
+ status = acpi_evaluate_object(handle, "VPCW", ¶ms, NULL);
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ return 0;
+}
+
+static int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
+{
+ unsigned long end_jiffies, val;
+ int err;
+
+ err = eval_vpcw(handle, 1, cmd);
+ if (err)
+ return err;
+
+ end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
+
+ while (time_before(jiffies, end_jiffies)) {
+ schedule();
+
+ err = eval_vpcr(handle, 1, &val);
+ if (err)
+ return err;
+
+ if (val == 0)
+ return eval_vpcr(handle, 0, data);
+ }
+
+ acpi_handle_err(handle, "timeout in %s\n", __func__);
+
+ return -ETIMEDOUT;
+}
+
+static int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
+{
+ unsigned long end_jiffies, val;
+ int err;
+
+ err = eval_vpcw(handle, 0, data);
+ if (err)
+ return err;
+
+ err = eval_vpcw(handle, 1, cmd);
+ if (err)
+ return err;
+
+ end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
+
+ while (time_before(jiffies, end_jiffies)) {
+ schedule();
+
+ err = eval_vpcr(handle, 1, &val);
+ if (err)
+ return err;
+
+ if (val == 0)
+ return 0;
+ }
+
+ acpi_handle_err(handle, "timeout in %s\n", __func__);
+
+ return -ETIMEDOUT;
+}
+
/*
* debugfs
*/
diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
index 948cc61800a9..1e52f2aa0aac 100644
--- a/drivers/platform/x86/ideapad-laptop.h
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -9,9 +9,6 @@
#ifndef _IDEAPAD_LAPTOP_H_
#define _IDEAPAD_LAPTOP_H_
-#include <linux/acpi.h>
-#include <linux/jiffies.h>
-#include <linux/errno.h>
#include <linux/notifier.h>
enum ideapad_laptop_notifier_actions {
@@ -22,140 +19,4 @@ int ideapad_laptop_register_notifier(struct notifier_block *nb);
int ideapad_laptop_unregister_notifier(struct notifier_block *nb);
void ideapad_laptop_call_notifier(unsigned long action, void *data);
-enum {
- VPCCMD_R_VPC1 = 0x10,
- VPCCMD_R_BL_MAX,
- VPCCMD_R_BL,
- VPCCMD_W_BL,
- VPCCMD_R_WIFI,
- VPCCMD_W_WIFI,
- VPCCMD_R_BT,
- VPCCMD_W_BT,
- VPCCMD_R_BL_POWER,
- VPCCMD_R_NOVO,
- VPCCMD_R_VPC2,
- VPCCMD_R_TOUCHPAD,
- VPCCMD_W_TOUCHPAD,
- VPCCMD_R_CAMERA,
- VPCCMD_W_CAMERA,
- VPCCMD_R_3G,
- VPCCMD_W_3G,
- VPCCMD_R_ODD, /* 0x21 */
- VPCCMD_W_FAN,
- VPCCMD_R_RF,
- VPCCMD_W_RF,
- VPCCMD_W_YMC = 0x2A,
- VPCCMD_R_FAN = 0x2B,
- VPCCMD_R_SPECIAL_BUTTONS = 0x31,
- VPCCMD_W_BL_POWER = 0x33,
-};
-
-static inline int eval_int_with_arg(acpi_handle handle, const char *name, unsigned long arg, unsigned long *res)
-{
- struct acpi_object_list params;
- unsigned long long result;
- union acpi_object in_obj;
- acpi_status status;
-
- params.count = 1;
- params.pointer = &in_obj;
- in_obj.type = ACPI_TYPE_INTEGER;
- in_obj.integer.value = arg;
-
- status = acpi_evaluate_integer(handle, (char *)name, ¶ms, &result);
- if (ACPI_FAILURE(status))
- return -EIO;
-
- if (res)
- *res = result;
-
- return 0;
-}
-
-static inline int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *res)
-{
- return eval_int_with_arg(handle, "VPCR", cmd, res);
-}
-
-static inline int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
-{
- struct acpi_object_list params;
- union acpi_object in_obj[2];
- acpi_status status;
-
- params.count = 2;
- params.pointer = in_obj;
- in_obj[0].type = ACPI_TYPE_INTEGER;
- in_obj[0].integer.value = cmd;
- in_obj[1].type = ACPI_TYPE_INTEGER;
- in_obj[1].integer.value = data;
-
- status = acpi_evaluate_object(handle, "VPCW", ¶ms, NULL);
- if (ACPI_FAILURE(status))
- return -EIO;
-
- return 0;
-}
-
-#define IDEAPAD_EC_TIMEOUT 200 /* in ms */
-
-static inline int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
-{
- unsigned long end_jiffies, val;
- int err;
-
- err = eval_vpcw(handle, 1, cmd);
- if (err)
- return err;
-
- end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
-
- while (time_before(jiffies, end_jiffies)) {
- schedule();
-
- err = eval_vpcr(handle, 1, &val);
- if (err)
- return err;
-
- if (val == 0)
- return eval_vpcr(handle, 0, data);
- }
-
- acpi_handle_err(handle, "timeout in %s\n", __func__);
-
- return -ETIMEDOUT;
-}
-
-static inline int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
-{
- unsigned long end_jiffies, val;
- int err;
-
- err = eval_vpcw(handle, 0, data);
- if (err)
- return err;
-
- err = eval_vpcw(handle, 1, cmd);
- if (err)
- return err;
-
- end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
-
- while (time_before(jiffies, end_jiffies)) {
- schedule();
-
- err = eval_vpcr(handle, 1, &val);
- if (err)
- return err;
-
- if (val == 0)
- return 0;
- }
-
- acpi_handle_err(handle, "timeout in %s\n", __func__);
-
- return -ETIMEDOUT;
-}
-
-#undef IDEAPAD_EC_TIMEOUT
#endif /* !_IDEAPAD_LAPTOP_H_ */
--
2.45.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v4 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file
2024-07-25 9:21 ` [PATCH v4 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file Gergo Koteles
@ 2024-08-12 15:38 ` Hans de Goede
0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2024-08-12 15:38 UTC (permalink / raw)
To: Gergo Koteles, Ilpo Järvinen, Ike Panhc
Cc: platform-driver-x86, linux-kernel
Hi,
On 7/25/24 11:21 AM, Gergo Koteles wrote:
> Since moving ymc_trigger_ec from lenovo-ymc to ideapad-laptop, only the
> latter uses these definitions and functions.
>
> Move them back to source file.
>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Thank you for your patch, I've applied this patch 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
> ---
> drivers/platform/x86/ideapad-laptop.c | 136 +++++++++++++++++++++++++
> drivers/platform/x86/ideapad-laptop.h | 139 --------------------------
> 2 files changed, 136 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 9fc1bb990e47..8398774cdfe2 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -22,6 +22,7 @@
> #include <linux/init.h>
> #include <linux/input.h>
> #include <linux/input/sparse-keymap.h>
> +#include <linux/jiffies.h>
> #include <linux/kernel.h>
> #include <linux/leds.h>
> #include <linux/module.h>
> @@ -87,6 +88,34 @@ enum {
> SALS_FNLOCK_OFF = 0xf,
> };
>
> +enum {
> + VPCCMD_R_VPC1 = 0x10,
> + VPCCMD_R_BL_MAX,
> + VPCCMD_R_BL,
> + VPCCMD_W_BL,
> + VPCCMD_R_WIFI,
> + VPCCMD_W_WIFI,
> + VPCCMD_R_BT,
> + VPCCMD_W_BT,
> + VPCCMD_R_BL_POWER,
> + VPCCMD_R_NOVO,
> + VPCCMD_R_VPC2,
> + VPCCMD_R_TOUCHPAD,
> + VPCCMD_W_TOUCHPAD,
> + VPCCMD_R_CAMERA,
> + VPCCMD_W_CAMERA,
> + VPCCMD_R_3G,
> + VPCCMD_W_3G,
> + VPCCMD_R_ODD, /* 0x21 */
> + VPCCMD_W_FAN,
> + VPCCMD_R_RF,
> + VPCCMD_W_RF,
> + VPCCMD_W_YMC = 0x2A,
> + VPCCMD_R_FAN = 0x2B,
> + VPCCMD_R_SPECIAL_BUTTONS = 0x31,
> + VPCCMD_W_BL_POWER = 0x33,
> +};
> +
> /*
> * These correspond to the number of supported states - 1
> * Future keyboard types may need a new system, if there's a collision
> @@ -236,6 +265,7 @@ static void ideapad_shared_exit(struct ideapad_private *priv)
> /*
> * ACPI Helpers
> */
> +#define IDEAPAD_EC_TIMEOUT 200 /* in ms */
>
> static int eval_int(acpi_handle handle, const char *name, unsigned long *res)
> {
> @@ -251,6 +281,29 @@ static int eval_int(acpi_handle handle, const char *name, unsigned long *res)
> return 0;
> }
>
> +static int eval_int_with_arg(acpi_handle handle, const char *name, unsigned long arg,
> + unsigned long *res)
> +{
> + struct acpi_object_list params;
> + unsigned long long result;
> + union acpi_object in_obj;
> + acpi_status status;
> +
> + params.count = 1;
> + params.pointer = &in_obj;
> + in_obj.type = ACPI_TYPE_INTEGER;
> + in_obj.integer.value = arg;
> +
> + status = acpi_evaluate_integer(handle, (char *)name, ¶ms, &result);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + if (res)
> + *res = result;
> +
> + return 0;
> +}
> +
> static int exec_simple_method(acpi_handle handle, const char *name, unsigned long arg)
> {
> acpi_status status = acpi_execute_simple_method(handle, (char *)name, arg);
> @@ -293,6 +346,89 @@ static int eval_dytc(acpi_handle handle, unsigned long cmd, unsigned long *res)
> return eval_int_with_arg(handle, "DYTC", cmd, res);
> }
>
> +static int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *res)
> +{
> + return eval_int_with_arg(handle, "VPCR", cmd, res);
> +}
> +
> +static int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
> +{
> + struct acpi_object_list params;
> + union acpi_object in_obj[2];
> + acpi_status status;
> +
> + params.count = 2;
> + params.pointer = in_obj;
> + in_obj[0].type = ACPI_TYPE_INTEGER;
> + in_obj[0].integer.value = cmd;
> + in_obj[1].type = ACPI_TYPE_INTEGER;
> + in_obj[1].integer.value = data;
> +
> + status = acpi_evaluate_object(handle, "VPCW", ¶ms, NULL);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
> +{
> + unsigned long end_jiffies, val;
> + int err;
> +
> + err = eval_vpcw(handle, 1, cmd);
> + if (err)
> + return err;
> +
> + end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
> +
> + while (time_before(jiffies, end_jiffies)) {
> + schedule();
> +
> + err = eval_vpcr(handle, 1, &val);
> + if (err)
> + return err;
> +
> + if (val == 0)
> + return eval_vpcr(handle, 0, data);
> + }
> +
> + acpi_handle_err(handle, "timeout in %s\n", __func__);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
> +{
> + unsigned long end_jiffies, val;
> + int err;
> +
> + err = eval_vpcw(handle, 0, data);
> + if (err)
> + return err;
> +
> + err = eval_vpcw(handle, 1, cmd);
> + if (err)
> + return err;
> +
> + end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
> +
> + while (time_before(jiffies, end_jiffies)) {
> + schedule();
> +
> + err = eval_vpcr(handle, 1, &val);
> + if (err)
> + return err;
> +
> + if (val == 0)
> + return 0;
> + }
> +
> + acpi_handle_err(handle, "timeout in %s\n", __func__);
> +
> + return -ETIMEDOUT;
> +}
> +
> /*
> * debugfs
> */
> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
> index 948cc61800a9..1e52f2aa0aac 100644
> --- a/drivers/platform/x86/ideapad-laptop.h
> +++ b/drivers/platform/x86/ideapad-laptop.h
> @@ -9,9 +9,6 @@
> #ifndef _IDEAPAD_LAPTOP_H_
> #define _IDEAPAD_LAPTOP_H_
>
> -#include <linux/acpi.h>
> -#include <linux/jiffies.h>
> -#include <linux/errno.h>
> #include <linux/notifier.h>
>
> enum ideapad_laptop_notifier_actions {
> @@ -22,140 +19,4 @@ int ideapad_laptop_register_notifier(struct notifier_block *nb);
> int ideapad_laptop_unregister_notifier(struct notifier_block *nb);
> void ideapad_laptop_call_notifier(unsigned long action, void *data);
>
> -enum {
> - VPCCMD_R_VPC1 = 0x10,
> - VPCCMD_R_BL_MAX,
> - VPCCMD_R_BL,
> - VPCCMD_W_BL,
> - VPCCMD_R_WIFI,
> - VPCCMD_W_WIFI,
> - VPCCMD_R_BT,
> - VPCCMD_W_BT,
> - VPCCMD_R_BL_POWER,
> - VPCCMD_R_NOVO,
> - VPCCMD_R_VPC2,
> - VPCCMD_R_TOUCHPAD,
> - VPCCMD_W_TOUCHPAD,
> - VPCCMD_R_CAMERA,
> - VPCCMD_W_CAMERA,
> - VPCCMD_R_3G,
> - VPCCMD_W_3G,
> - VPCCMD_R_ODD, /* 0x21 */
> - VPCCMD_W_FAN,
> - VPCCMD_R_RF,
> - VPCCMD_W_RF,
> - VPCCMD_W_YMC = 0x2A,
> - VPCCMD_R_FAN = 0x2B,
> - VPCCMD_R_SPECIAL_BUTTONS = 0x31,
> - VPCCMD_W_BL_POWER = 0x33,
> -};
> -
> -static inline int eval_int_with_arg(acpi_handle handle, const char *name, unsigned long arg, unsigned long *res)
> -{
> - struct acpi_object_list params;
> - unsigned long long result;
> - union acpi_object in_obj;
> - acpi_status status;
> -
> - params.count = 1;
> - params.pointer = &in_obj;
> - in_obj.type = ACPI_TYPE_INTEGER;
> - in_obj.integer.value = arg;
> -
> - status = acpi_evaluate_integer(handle, (char *)name, ¶ms, &result);
> - if (ACPI_FAILURE(status))
> - return -EIO;
> -
> - if (res)
> - *res = result;
> -
> - return 0;
> -}
> -
> -static inline int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *res)
> -{
> - return eval_int_with_arg(handle, "VPCR", cmd, res);
> -}
> -
> -static inline int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
> -{
> - struct acpi_object_list params;
> - union acpi_object in_obj[2];
> - acpi_status status;
> -
> - params.count = 2;
> - params.pointer = in_obj;
> - in_obj[0].type = ACPI_TYPE_INTEGER;
> - in_obj[0].integer.value = cmd;
> - in_obj[1].type = ACPI_TYPE_INTEGER;
> - in_obj[1].integer.value = data;
> -
> - status = acpi_evaluate_object(handle, "VPCW", ¶ms, NULL);
> - if (ACPI_FAILURE(status))
> - return -EIO;
> -
> - return 0;
> -}
> -
> -#define IDEAPAD_EC_TIMEOUT 200 /* in ms */
> -
> -static inline int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
> -{
> - unsigned long end_jiffies, val;
> - int err;
> -
> - err = eval_vpcw(handle, 1, cmd);
> - if (err)
> - return err;
> -
> - end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
> -
> - while (time_before(jiffies, end_jiffies)) {
> - schedule();
> -
> - err = eval_vpcr(handle, 1, &val);
> - if (err)
> - return err;
> -
> - if (val == 0)
> - return eval_vpcr(handle, 0, data);
> - }
> -
> - acpi_handle_err(handle, "timeout in %s\n", __func__);
> -
> - return -ETIMEDOUT;
> -}
> -
> -static inline int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
> -{
> - unsigned long end_jiffies, val;
> - int err;
> -
> - err = eval_vpcw(handle, 0, data);
> - if (err)
> - return err;
> -
> - err = eval_vpcw(handle, 1, cmd);
> - if (err)
> - return err;
> -
> - end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
> -
> - while (time_before(jiffies, end_jiffies)) {
> - schedule();
> -
> - err = eval_vpcr(handle, 1, &val);
> - if (err)
> - return err;
> -
> - if (val == 0)
> - return 0;
> - }
> -
> - acpi_handle_err(handle, "timeout in %s\n", __func__);
> -
> - return -ETIMEDOUT;
> -}
> -
> -#undef IDEAPAD_EC_TIMEOUT
> #endif /* !_IDEAPAD_LAPTOP_H_ */
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands
2024-07-25 9:21 [PATCH v4 0/4] platform/x86: ideapad-laptop: synchronize VPC commands Gergo Koteles
` (2 preceding siblings ...)
2024-07-25 9:21 ` [PATCH v4 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file Gergo Koteles
@ 2024-07-25 9:21 ` Gergo Koteles
2024-07-30 13:37 ` Ilpo Järvinen
3 siblings, 1 reply; 9+ messages in thread
From: Gergo Koteles @ 2024-07-25 9:21 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Ike Panhc
Cc: platform-driver-x86, linux-kernel, Gergo Koteles
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(-)
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);
--
2.45.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands
2024-07-25 9:21 ` [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands Gergo Koteles
@ 2024-07-30 13:37 ` Ilpo Järvinen
2024-07-30 16:00 ` Gergo Koteles
0 siblings, 1 reply; 9+ messages in thread
From: Ilpo Järvinen @ 2024-07-30 13:37 UTC (permalink / raw)
To: Gergo Koteles; +Cc: Hans de Goede, Ike Panhc, platform-driver-x86, LKML
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).
--
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands
2024-07-30 13:37 ` Ilpo Järvinen
@ 2024-07-30 16:00 ` Gergo Koteles
2024-08-08 12:16 ` Ilpo Järvinen
0 siblings, 1 reply; 9+ messages in thread
From: Gergo Koteles @ 2024-07-30 16:00 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Hans de Goede, Ike Panhc, platform-driver-x86, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands
2024-07-30 16:00 ` Gergo Koteles
@ 2024-08-08 12:16 ` Ilpo Järvinen
0 siblings, 0 replies; 9+ messages in thread
From: Ilpo Järvinen @ 2024-08-08 12:16 UTC (permalink / raw)
To: Gergo Koteles, Hans de Goede; +Cc: Ike Panhc, platform-driver-x86, LKML
[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]
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.
--
i.
^ permalink raw reply [flat|nested] 9+ messages in thread