[PATCH v2 0/4] platform/x86: ideapad-laptop: synchronize VPC commands

[PATCH v2 0/4] platform/x86: ideapad-laptop: synchronize VPC commands

[Gergo Koteles]

Hi,

Sometimes the Yoga mode control switch did not work properly on my 
laptop, and sometimes even caused a platform profile switch. It turned 
out that it was caused by a race situation, the lenovo-ymc wmi notify 
handler was running at the same time as the ideapad-laptop acpi notify 
handler, and the partial results of the VPC calls in the two were mixed 
up.

This series introduces a notification chain to receive YMC or other 
events, moves the EC triggering into ideapad-laptop, and then introduces 
a mutex to eliminate the problem.

Best regards,
Gergo Koteles

---
Changes in v2:
- Skip the already merged "platform/x86: ideapad-laptop: use cleanup.h" commit
- Based on the WMI driver development guide and the dell drivers,
  introduce a generic notification chain
- Use the notification chain to send the YMC event from lenovo-ymc to
  the ideapad-laptop module
- Move the ec_trigger module parameter to the ideapad-laptop as well
- Mutex changes go into one patch
- Link to v1: https://lore.kernel.org/all/cover.1720515666.git.soyer@irl.hu/
---

Gergo Koteles (4):
  platform/x86: ideapad-laptop: introduce a generic notification chain
  platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc
  platform/x86: ideapad-laptop: move ACPI helpers from header to source
    file
  platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands

 drivers/platform/x86/Kconfig          |   1 +
 drivers/platform/x86/ideapad-laptop.c | 282 ++++++++++++++++++++++++--
 drivers/platform/x86/ideapad-laptop.h | 142 +------------
 drivers/platform/x86/lenovo-ymc.c     |  60 +-----
 4 files changed, 275 insertions(+), 210 deletions(-)

-- 
2.45.2

[PATCH v2 1/4] platform/x86: ideapad-laptop: introduce a generic notification chain

[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 88eefccb6ed2..e4d32a788339 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1498,6 +1498,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;
@@ -1869,6 +1902,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:
@@ -1900,6 +1935,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

[PATCH v2 2/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[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>
---
 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 6dbd40e2aeda..4af0a060cd47 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -464,6 +464,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 e4d32a788339..1d61bd921528 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -145,6 +145,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;
@@ -188,6 +189,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
  */
@@ -1498,10 +1505,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;
@@ -1667,6 +1714,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

Re: [PATCH v2 2/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8879 bytes --]

On Thu, 18 Jul 2024, Gergo Koteles wrote:

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

I certainly like this approch more than the one in v1.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> ---
>  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 6dbd40e2aeda..4af0a060cd47 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -464,6 +464,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 e4d32a788339..1d61bd921528 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -145,6 +145,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;
> @@ -188,6 +189,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
>   */
> @@ -1498,10 +1505,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;
> @@ -1667,6 +1714,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);
> 

[PATCH v2 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file

[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>
---
 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 1d61bd921528..66b34e99147e 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -21,6 +21,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>
@@ -86,6 +87,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
@@ -234,6 +263,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)
 {
@@ -249,6 +279,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, &params, &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);
@@ -291,6 +344,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", &params, 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, &params, &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", &params, 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

Re: [PATCH v2 3/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 9231 bytes --]

On Thu, 18 Jul 2024, 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>

-- 
 i.

> ---
>  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 1d61bd921528..66b34e99147e 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -21,6 +21,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>
> @@ -86,6 +87,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
> @@ -234,6 +263,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)
>  {
> @@ -249,6 +279,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, &params, &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);
> @@ -291,6 +344,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", &params, 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, &params, &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", &params, 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_ */
> 

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

[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 | 62 +++++++++++++++++++--------
 1 file changed, 45 insertions(+), 17 deletions(-)

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

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

[Ilpo Järvinen]

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

-- 
 i.

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

[Hans de Goede]

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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

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... :-/

-- 
 i.