Re: [PATCH v3 08/16] platform/x86/amd/pmf: Add support to update system state

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/10/2023 07:59, Shyam Sundar S K wrote:
> PMF driver based on the output actions from the TA can request to update
> the system states like entering s0i3, lock screen etc. by generating
> an uevent. Based on the udev rules set in the userspace the event id
> matching the uevent shall get updated accordingly using the systemctl.
> 
> Sample udev rules under Documentation/admin-guide/pmf.rst.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

One minor nit below.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   Documentation/admin-guide/index.rst   |  1 +
>   Documentation/admin-guide/pmf.rst     | 25 +++++++++++++++++++
>   drivers/platform/x86/amd/pmf/pmf.h    |  9 +++++++
>   drivers/platform/x86/amd/pmf/tee-if.c | 36 ++++++++++++++++++++++++++-
>   4 files changed, 70 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/admin-guide/pmf.rst
> 
> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
> index 43ea35613dfc..fb40a1f6f79e 100644
> --- a/Documentation/admin-guide/index.rst
> +++ b/Documentation/admin-guide/index.rst
> @@ -119,6 +119,7 @@ configure specific aspects of kernel behavior to your liking.
>      parport
>      perf-security
>      pm/index
> +   pmf
>      pnp
>      rapidio
>      ras
> diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
> new file mode 100644
> index 000000000000..6985bb5b9452
> --- /dev/null
> +++ b/Documentation/admin-guide/pmf.rst
> @@ -0,0 +1,25 @@
> +.. 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
> +
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 20f3e16b0a32..67f11113d5a7 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -73,6 +73,7 @@
>   #define PMF_POLICY_STT_MIN					6
>   #define PMF_POLICY_STT_SKINTEMP_APU				7
>   #define PMF_POLICY_STT_SKINTEMP_HS2				8
> +#define PMF_POLICY_SYSTEM_STATE					9
>   #define PMF_POLICY_P3T						38
>   
>   /* TA macros */
> @@ -440,6 +441,13 @@ struct apmf_dyn_slider_output {
>   } __packed;
>   
>   /* Smart PC - TA internals */

I know that Ilpo had a comment about this in an earlier version that 
there is a "__" instead of "_".  I know this is intended behavior for 
consistency with internal usage, but maybe it's worth having a comment 
somewhere mentioning it's intended behavior?  I'm not sure where.

> +enum system_state {
> +	SYSTEM_STATE__S0i3,
> +	SYSTEM_STATE__S4,
> +	SYSTEM_STATE__SCREEN_LOCK,
> +	SYSTEM_STATE__MAX
> +};
> +
>   enum ta_slider {
>   	TA_BEST_BATTERY,	/* Best Battery */
>   	TA_BETTER_BATTERY,	/* Better Battery */
> @@ -471,6 +479,7 @@ enum ta_pmf_error_type {
>   };
>   
>   struct pmf_action_table {
> +	enum system_state system_state;
>   	u32 spl;		/* in mW */
>   	u32 sppt;		/* in mW */
>   	u32 sppt_apuonly;	/* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 92879ae4f8f0..c08ef13a1494 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -24,6 +24,20 @@ MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (defau
>   static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
>   						0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>   
> +static const char *amd_pmf_uevent_as_str(unsigned int state)
> +{
> +	switch (state) {
> +	case SYSTEM_STATE__S0i3:
> +		return "S0i3";
> +	case SYSTEM_STATE__S4:
> +		return "S4";
> +	case SYSTEM_STATE__SCREEN_LOCK:
> +		return "SCREEN_LOCK";
> +	default:
> +		return "Unknown Smart PC event";
> +	}
> +}
> +
>   static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>   				 struct tee_ioctl_invoke_arg *arg,
>   				 struct tee_param *param)
> @@ -42,9 +56,23 @@ 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)
> +{
> +	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;
> +}
> +
>   static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>   {
> -	u32 val;
> +	u32 val, event = 0;
>   	int idx;
>   
>   	for (idx = 0; idx < out->actions_count; idx++) {
> @@ -113,6 +141,12 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>   				dev->prev_data->p3t_limit = val;
>   			}
>   			break;
> +
> +		case PMF_POLICY_SYSTEM_STATE:
> +			amd_pmf_update_uevents(dev, event);
> +			dev_dbg(dev->dev, "update SYSTEM_STATE : %s\n",
> +				amd_pmf_uevent_as_str(event));
> +			break;
>   		}
>   	}
>   }