[PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Shyam Sundar S K]

At present, the PMF driver employs custom system state codes to update
system states. It is recommended to replace these with existing input
event codes (KEY_SLEEP, KEY_SUSPEND, and KEY_SCREENLOCK) to align system
updates with the PMF-TA output actions.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/pmf.h    |  2 +
 drivers/platform/x86/amd/pmf/tee-if.c | 62 +++++++++++++++++++++------
 2 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index eeedd0c0395a..753d5662c080 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -12,6 +12,7 @@
 #define PMF_H
 
 #include <linux/acpi.h>
+#include <linux/input.h>
 #include <linux/platform_profile.h>
 
 #define POLICY_BUF_MAX_SZ		0x4b000
@@ -300,6 +301,7 @@ struct amd_pmf_dev {
 	void __iomem *policy_base;
 	bool smart_pc_enabled;
 	u16 pmf_if_version;
+	struct input_dev *pmf_idev;
 };
 
 struct apmf_sps_prop_granular_v2 {
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index b438de4d6bfc..b0449f912048 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -62,18 +62,12 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
 	param[0].u.memref.shm_offs = 0;
 }
 
-static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
+static void amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
 {
-	char *envp[2] = {};
-
-	envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
-	if (!envp[0])
-		return -EINVAL;
-
-	kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
-
-	kfree(envp[0]);
-	return 0;
+	input_report_key(dev->pmf_idev, event, 1); /* key press */
+	input_sync(dev->pmf_idev);
+	input_report_key(dev->pmf_idev, event, 0); /* key release */
+	input_sync(dev->pmf_idev);
 }
 
 static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
@@ -149,7 +143,20 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
 			break;
 
 		case PMF_POLICY_SYSTEM_STATE:
-			amd_pmf_update_uevents(dev, val);
+			switch (val) {
+			case 0:
+				amd_pmf_update_uevents(dev, KEY_SLEEP);
+				break;
+			case 1:
+				amd_pmf_update_uevents(dev, KEY_SUSPEND);
+				break;
+			case 2:
+				amd_pmf_update_uevents(dev, KEY_SCREENLOCK);
+				break;
+			default:
+				dev_err(dev->dev, "Invalid PMF policy system state: %d\n", val);
+			}
+
 			dev_dbg(dev->dev, "update SYSTEM_STATE: %s\n",
 				amd_pmf_uevent_as_str(val));
 			break;
@@ -368,6 +375,30 @@ static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
 	return rc;
 }
 
+static int amd_pmf_register_input_device(struct amd_pmf_dev *dev)
+{
+	int err;
+
+	dev->pmf_idev = devm_input_allocate_device(dev->dev);
+	if (!dev->pmf_idev)
+		return -ENOMEM;
+
+	dev->pmf_idev->name = "PMF-TA output events";
+	dev->pmf_idev->phys = "amd-pmf/input0";
+
+	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SLEEP);
+	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SCREENLOCK);
+	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SUSPEND);
+
+	err = input_register_device(dev->pmf_idev);
+	if (err) {
+		dev_err(dev->dev, "Failed to register input device: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
 static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
 {
 	u32 size;
@@ -475,6 +506,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	if (pb_side_load)
 		amd_pmf_open_pb(dev, dev->dbgfs_dir);
 
+	ret = amd_pmf_register_input_device(dev);
+	if (ret)
+		goto error;
+
 	return 0;
 
 error:
@@ -488,6 +523,9 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
 	if (pb_side_load && dev->esbin)
 		amd_pmf_remove_pb(dev);
 
+	if (dev->pmf_idev)
+		input_unregister_device(dev->pmf_idev);
+
 	cancel_delayed_work_sync(&dev->pb_work);
 	kfree(dev->prev_data);
 	dev->prev_data = NULL;
-- 
2.25.1

[PATCH v1 2/2] platform/x86/amd/pmf: Remove update system state document

[Shyam Sundar S K]

This commit removes the "pmf.rst" document, which was associated with
the PMF driver that enabled system state updates based on TA output
actions.

The driver now uses existing input events (KEY_SCREENLOCK, KEY_SLEEP,
and KEY_SUSPEND) instead of defining new udev rules in the
"/etc/udev/rules.d/" directory. Consequently, the pmf.rst document is no
longer necessary. Therefore, the pmf.rst documentation is being removed.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 Documentation/admin-guide/pmf.rst | 24 ------------------------
 1 file changed, 24 deletions(-)
 delete mode 100644 Documentation/admin-guide/pmf.rst

diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
deleted file mode 100644
index 9ee729ffc19b..000000000000
--- a/Documentation/admin-guide/pmf.rst
+++ /dev/null
@@ -1,24 +0,0 @@
-.. SPDX-License-Identifier: GPL-2.0
-
-Set udev rules for PMF Smart PC Builder
----------------------------------------
-
-AMD PMF(Platform Management Framework) Smart PC Solution builder has to set the system states
-like S0i3, Screen lock, hibernate etc, based on the output actions provided by the PMF
-TA (Trusted Application).
-
-In order for this to work the PMF driver generates a uevent for userspace to react to. Below are
-sample udev rules that can facilitate this experience when a machine has PMF Smart PC solution builder
-enabled.
-
-Please add the following line(s) to
-``/etc/udev/rules.d/99-local.rules``::
-
-        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="0", RUN+="/usr/bin/systemctl suspend"
-        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1", RUN+="/usr/bin/systemctl hibernate"
-        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/bin/loginctl lock-sessions"
-
-EVENT_ID values:
-0= Put the system to S0i3/S2Idle
-1= Put the system to hibernate
-2= Lock the screen
-- 
2.25.1

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Mario Limonciello]

On 7/2/2024 3:06, Shyam Sundar S K wrote:
> At present, the PMF driver employs custom system state codes to update
> system states. It is recommended to replace these with existing input
> event codes (KEY_SLEEP, KEY_SUSPEND, and KEY_SCREENLOCK) to align system
> updates with the PMF-TA output actions.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/pmf.h    |  2 +
>   drivers/platform/x86/amd/pmf/tee-if.c | 62 +++++++++++++++++++++------
>   2 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index eeedd0c0395a..753d5662c080 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -12,6 +12,7 @@
>   #define PMF_H
>   
>   #include <linux/acpi.h>
> +#include <linux/input.h>
>   #include <linux/platform_profile.h>
>   
>   #define POLICY_BUF_MAX_SZ		0x4b000
> @@ -300,6 +301,7 @@ struct amd_pmf_dev {
>   	void __iomem *policy_base;
>   	bool smart_pc_enabled;
>   	u16 pmf_if_version;
> +	struct input_dev *pmf_idev;
>   };
>   
>   struct apmf_sps_prop_granular_v2 {
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index b438de4d6bfc..b0449f912048 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -62,18 +62,12 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>   	param[0].u.memref.shm_offs = 0;
>   }
>   
> -static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
> +static void amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
>   {
> -	char *envp[2] = {};
> -
> -	envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
> -	if (!envp[0])
> -		return -EINVAL;
> -
> -	kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
> -
> -	kfree(envp[0]);
> -	return 0;
> +	input_report_key(dev->pmf_idev, event, 1); /* key press */
> +	input_sync(dev->pmf_idev);
> +	input_report_key(dev->pmf_idev, event, 0); /* key release */
> +	input_sync(dev->pmf_idev);
>   }
>   
>   static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> @@ -149,7 +143,20 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>   			break;
>   
>   		case PMF_POLICY_SYSTEM_STATE:
> -			amd_pmf_update_uevents(dev, val);
> +			switch (val) {
> +			case 0:
> +				amd_pmf_update_uevents(dev, KEY_SLEEP);
> +				break;
> +			case 1:
> +				amd_pmf_update_uevents(dev, KEY_SUSPEND);
> +				break;
> +			case 2:
> +				amd_pmf_update_uevents(dev, KEY_SCREENLOCK);
> +				break;
> +			default:
> +				dev_err(dev->dev, "Invalid PMF policy system state: %d\n", val);
> +			}
> +
>   			dev_dbg(dev->dev, "update SYSTEM_STATE: %s\n",
>   				amd_pmf_uevent_as_str(val));
>   			break;
> @@ -368,6 +375,30 @@ static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
>   	return rc;
>   }
>   
> +static int amd_pmf_register_input_device(struct amd_pmf_dev *dev)
> +{
> +	int err;
> +
> +	dev->pmf_idev = devm_input_allocate_device(dev->dev);
> +	if (!dev->pmf_idev)
> +		return -ENOMEM;
> +
> +	dev->pmf_idev->name = "PMF-TA output events";
> +	dev->pmf_idev->phys = "amd-pmf/input0";
> +
> +	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SLEEP);
> +	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SCREENLOCK);
> +	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SUSPEND);
> +
> +	err = input_register_device(dev->pmf_idev);
> +	if (err) {
> +		dev_err(dev->dev, "Failed to register input device: %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>   static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
>   {
>   	u32 size;
> @@ -475,6 +506,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>   	if (pb_side_load)
>   		amd_pmf_open_pb(dev, dev->dbgfs_dir);
>   
> +	ret = amd_pmf_register_input_device(dev);
> +	if (ret)
> +		goto error;
> +
>   	return 0;
>   
>   error:
> @@ -488,6 +523,9 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>   	if (pb_side_load && dev->esbin)
>   		amd_pmf_remove_pb(dev);
>   
> +	if (dev->pmf_idev)
> +		input_unregister_device(dev->pmf_idev);
> +
>   	cancel_delayed_work_sync(&dev->pb_work);
>   	kfree(dev->prev_data);
>   	dev->prev_data = NULL;

Re: [PATCH v1 2/2] platform/x86/amd/pmf: Remove update system state document

[Mario Limonciello]

On 7/2/2024 3:06, Shyam Sundar S K wrote:
> This commit removes the "pmf.rst" document, which was associated with
> the PMF driver that enabled system state updates based on TA output
> actions.
> 
> The driver now uses existing input events (KEY_SCREENLOCK, KEY_SLEEP,
> and KEY_SUSPEND) instead of defining new udev rules in the
> "/etc/udev/rules.d/" directory. Consequently, the pmf.rst document is no
> longer necessary. Therefore, the pmf.rst documentation is being removed.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   Documentation/admin-guide/pmf.rst | 24 ------------------------
>   1 file changed, 24 deletions(-)
>   delete mode 100644 Documentation/admin-guide/pmf.rst
> 
> diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
> deleted file mode 100644
> index 9ee729ffc19b..000000000000
> --- a/Documentation/admin-guide/pmf.rst
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -.. SPDX-License-Identifier: GPL-2.0
> -
> -Set udev rules for PMF Smart PC Builder
> ----------------------------------------
> -
> -AMD PMF(Platform Management Framework) Smart PC Solution builder has to set the system states
> -like S0i3, Screen lock, hibernate etc, based on the output actions provided by the PMF
> -TA (Trusted Application).
> -
> -In order for this to work the PMF driver generates a uevent for userspace to react to. Below are
> -sample udev rules that can facilitate this experience when a machine has PMF Smart PC solution builder
> -enabled.
> -
> -Please add the following line(s) to
> -``/etc/udev/rules.d/99-local.rules``::
> -
> -        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="0", RUN+="/usr/bin/systemctl suspend"
> -        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1", RUN+="/usr/bin/systemctl hibernate"
> -        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/bin/loginctl lock-sessions"
> -
> -EVENT_ID values:
> -0= Put the system to S0i3/S2Idle
> -1= Put the system to hibernate
> -2= Lock the screen

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Ilpo Järvinen]

On Tue, 2 Jul 2024, Shyam Sundar S K wrote:

> At present, the PMF driver employs custom system state codes to update
> system states. It is recommended to replace these with existing input

This change entirely removes the way userspace worked before this change?
We cannot take userspace functionality away like this.

-- 
 i.

> event codes (KEY_SLEEP, KEY_SUSPEND, and KEY_SCREENLOCK) to align system
> updates with the PMF-TA output actions.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/pmf.h    |  2 +
>  drivers/platform/x86/amd/pmf/tee-if.c | 62 +++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index eeedd0c0395a..753d5662c080 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -12,6 +12,7 @@
>  #define PMF_H
>  
>  #include <linux/acpi.h>
> +#include <linux/input.h>
>  #include <linux/platform_profile.h>
>  
>  #define POLICY_BUF_MAX_SZ		0x4b000
> @@ -300,6 +301,7 @@ struct amd_pmf_dev {
>  	void __iomem *policy_base;
>  	bool smart_pc_enabled;
>  	u16 pmf_if_version;
> +	struct input_dev *pmf_idev;
>  };
>  
>  struct apmf_sps_prop_granular_v2 {
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index b438de4d6bfc..b0449f912048 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -62,18 +62,12 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>  	param[0].u.memref.shm_offs = 0;
>  }
>  
> -static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
> +static void amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
>  {
> -	char *envp[2] = {};
> -
> -	envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
> -	if (!envp[0])
> -		return -EINVAL;
> -
> -	kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
> -
> -	kfree(envp[0]);
> -	return 0;
> +	input_report_key(dev->pmf_idev, event, 1); /* key press */
> +	input_sync(dev->pmf_idev);
> +	input_report_key(dev->pmf_idev, event, 0); /* key release */
> +	input_sync(dev->pmf_idev);
>  }
>  
>  static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> @@ -149,7 +143,20 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>  			break;
>  
>  		case PMF_POLICY_SYSTEM_STATE:
> -			amd_pmf_update_uevents(dev, val);
> +			switch (val) {
> +			case 0:
> +				amd_pmf_update_uevents(dev, KEY_SLEEP);
> +				break;
> +			case 1:
> +				amd_pmf_update_uevents(dev, KEY_SUSPEND);
> +				break;
> +			case 2:
> +				amd_pmf_update_uevents(dev, KEY_SCREENLOCK);
> +				break;
> +			default:
> +				dev_err(dev->dev, "Invalid PMF policy system state: %d\n", val);
> +			}
> +
>  			dev_dbg(dev->dev, "update SYSTEM_STATE: %s\n",
>  				amd_pmf_uevent_as_str(val));
>  			break;
> @@ -368,6 +375,30 @@ static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
>  	return rc;
>  }
>  
> +static int amd_pmf_register_input_device(struct amd_pmf_dev *dev)
> +{
> +	int err;
> +
> +	dev->pmf_idev = devm_input_allocate_device(dev->dev);
> +	if (!dev->pmf_idev)
> +		return -ENOMEM;
> +
> +	dev->pmf_idev->name = "PMF-TA output events";
> +	dev->pmf_idev->phys = "amd-pmf/input0";
> +
> +	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SLEEP);
> +	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SCREENLOCK);
> +	input_set_capability(dev->pmf_idev, EV_KEY, KEY_SUSPEND);
> +
> +	err = input_register_device(dev->pmf_idev);
> +	if (err) {
> +		dev_err(dev->dev, "Failed to register input device: %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
>  {
>  	u32 size;
> @@ -475,6 +506,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	if (pb_side_load)
>  		amd_pmf_open_pb(dev, dev->dbgfs_dir);
>  
> +	ret = amd_pmf_register_input_device(dev);
> +	if (ret)
> +		goto error;
> +
>  	return 0;
>  
>  error:
> @@ -488,6 +523,9 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>  	if (pb_side_load && dev->esbin)
>  		amd_pmf_remove_pb(dev);
>  
> +	if (dev->pmf_idev)
> +		input_unregister_device(dev->pmf_idev);
> +
>  	cancel_delayed_work_sync(&dev->pb_work);
>  	kfree(dev->prev_data);
>  	dev->prev_data = NULL;
> 

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Shyam Sundar S K]

On 7/6/2024 18:39, Ilpo Järvinen wrote:
> On Tue, 2 Jul 2024, Shyam Sundar S K wrote:
> 
>> At present, the PMF driver employs custom system state codes to update
>> system states. It is recommended to replace these with existing input
> 
> This change entirely removes the way userspace worked before this change?

No. It's still the same way it worked like before; just that we are
removing an additional technical debt of maintaining a separate udev
rules file for kicking in the user-space action.

GNOME folks told that they cannot have a custom actions defined in
udev rules, instead asked to use KEY_SCREENLOCK, KEY_SLEEP,
and KEY_SUSPEND so it entirely matches the behavior.

> We cannot take userspace functionality away like this.
> 

Can you please take a look at PATCH 2/2, that may help to clarify on
why PATCH 1/2 is required.

Thanks,
Shyam

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Hans de Goede]

Hi Ilpo,

On 7/6/24 3:09 PM, Ilpo Järvinen wrote:
> On Tue, 2 Jul 2024, Shyam Sundar S K wrote:
> 
>> At present, the PMF driver employs custom system state codes to update
>> system states. It is recommended to replace these with existing input
> 
> This change entirely removes the way userspace worked before this change?
> We cannot take userspace functionality away like this.

I completely agree with you that we cannot just go and remove existing
userspace API.

But AFAICT in this case no known userspace code has ever actually started
relying on these custom udev events. The docs suggest creating a custom
udev rules files which I don't believe any distributions have actually
done, not has this been made part of the default udev rules shipped
with systemd.

So I think in this case we can get away with removing the udev event
generation and the sooner we do so, the smaller the chance something
does actually start depending on it.

And if I'm wrong it should be easy to add back the udev event generation
and send both the udev events and the key-presses.

Regards,

Hans

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Ilpo Järvinen]

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

On Wed, 10 Jul 2024, Hans de Goede wrote:
> On 7/6/24 3:09 PM, Ilpo Järvinen wrote:
> > On Tue, 2 Jul 2024, Shyam Sundar S K wrote:
> > 
> >> At present, the PMF driver employs custom system state codes to update
> >> system states. It is recommended to replace these with existing input
> > 
> > This change entirely removes the way userspace worked before this change?
> > We cannot take userspace functionality away like this.
> 
> I completely agree with you that we cannot just go and remove existing
> userspace API.
> 
> But AFAICT in this case no known userspace code has ever actually started
> relying on these custom udev events. The docs suggest creating a custom
> udev rules files which I don't believe any distributions have actually
> done, not has this been made part of the default udev rules shipped
> with systemd.
> 
> So I think in this case we can get away with removing the udev event
> generation and the sooner we do so, the smaller the chance something
> does actually start depending on it.
> 
> And if I'm wrong it should be easy to add back the udev event generation
> and send both the udev events and the key-presses.

Okay, thanks for chimming in.

-- 
 i.

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Ilpo Järvinen]

On Tue, 2 Jul 2024, Shyam Sundar S K wrote:

> At present, the PMF driver employs custom system state codes to update
> system states. It is recommended to replace these with existing input
> event codes (KEY_SLEEP, KEY_SUSPEND, and KEY_SCREENLOCK) to align system
> updates with the PMF-TA output actions.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---

> @@ -475,6 +506,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	if (pb_side_load)
>  		amd_pmf_open_pb(dev, dev->dbgfs_dir);
>  
> +	ret = amd_pmf_register_input_device(dev);
> +	if (ret)
> +		goto error;
> +
>  	return 0;
>  
>  error:
> @@ -488,6 +523,9 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>  	if (pb_side_load && dev->esbin)
>  		amd_pmf_remove_pb(dev);
>  
> +	if (dev->pmf_idev)
> +		input_unregister_device(dev->pmf_idev);
> +

Why is the ordering in the init and deinit asymmetric? Is that 
intentional?

-- 
 i.


>  	cancel_delayed_work_sync(&dev->pb_work);
>  	kfree(dev->prev_data);
>  	dev->prev_data = NULL;

Re: [PATCH v1 1/2] platform/x86/amd/pmf: Use existing input event codes to update system states

[Shyam Sundar S K]

Hi Ilpo,

On 7/10/2024 16:36, Ilpo Järvinen wrote:
> On Tue, 2 Jul 2024, Shyam Sundar S K wrote:
> 
>> At present, the PMF driver employs custom system state codes to update
>> system states. It is recommended to replace these with existing input
>> event codes (KEY_SLEEP, KEY_SUSPEND, and KEY_SCREENLOCK) to align system
>> updates with the PMF-TA output actions.
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
> 
>> @@ -475,6 +506,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>  	if (pb_side_load)
>>  		amd_pmf_open_pb(dev, dev->dbgfs_dir);
>>  
>> +	ret = amd_pmf_register_input_device(dev);
>> +	if (ret)
>> +		goto error;
>> +
>>  	return 0;
>>  
>>  error:
>> @@ -488,6 +523,9 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>>  	if (pb_side_load && dev->esbin)
>>  		amd_pmf_remove_pb(dev);
>>  
>> +	if (dev->pmf_idev)
>> +		input_unregister_device(dev->pmf_idev);
>> +
> 
> Why is the ordering in the init and deinit asymmetric? Is that 
> intentional?
> 

No. This is not intentional. I will respin a new version to make it
symmetric.

Thanks,
Shyam