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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 10/4/2023 5:53 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, 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.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202309260515.5XbR6N0g-lkp@intel.com/
> 
> Please don't put lkp tags for patches that are still under development 
> (even if the email you get misleadingly instructs you to). Only use them 
> when you fix code that's already in tree based on LKP's report.

Agreed.

> 
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@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 | 41 ++++++++++++++++++++++++++-
>>  4 files changed, 75 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..90072add511e
>> --- /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}=="1", RUN+="/usr/bin/systemctl suspend"
>> +        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/usr/bin/systemctl hibernate"
>> +        DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="3", RUN+="/bin/loginctl lock-sessions"
>> +
>> +EVENT_ID values:
>> +1= Put the system to S0i3/S2Idle
>> +2= Put the system to hibernate
>> +3= Lock the screen
>> +
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index d5e410c41e81..34778192432e 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 */
>> @@ -439,6 +440,13 @@ struct apmf_dyn_slider_output {
>>  } __packed;
>>  
>>  /* Smart PC - TA internals */
>> +enum system_state {
>> +	SYSTEM_STATE__S0i3 = 1,
>> +	SYSTEM_STATE__S4,
>> +	SYSTEM_STATE__SCREEN_LOCK,
>> +	SYSTEM_STATE__MAX
>> +};
>> +
>>  enum ta_slider {
>>  	TA_BEST_BATTERY, /* Best Battery */
>>  	TA_BETTER_BATTERY, /* Better Battery */
>> @@ -470,6 +478,7 @@ enum ta_pmf_error_type {
>>  };
>>  
>>  struct pmf_action_table {
>> +	enum system_state system_state;
>>  	unsigned long spl; /* in mW */
>>  	unsigned long sppt; /* in mW */
>>  	unsigned long 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 315e3d2eacdf..961011530c1b 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)
>>  {
>> -	unsigned long val;
>> +	unsigned long val, event = 0;
>>  	int idx;
>>  
>>  	for (idx = 0; idx < out->actions_count; idx++) {
>> @@ -113,6 +141,17 @@ 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:
>> +			event = val + 1;
>> +			if (dev->prev_data->system_state != event) {
>> +				amd_pmf_update_uevents(dev, event);
>> +				dev_dbg(dev->dev, "update SYSTEM_STATE : %s\n",
>> +					amd_pmf_uevent_as_str(event));
>> +				/* reset the previous recorded state to zero */
>> +				dev->prev_data->system_state = 0;
> 
> No, a comment won't help you here. As is, system_state is constant 0 so 
> it's entirely unnecessary to keep that value at all. In fact, the comment 
> is wrong because there never was "previously recorder state" in 
> ->system_state to begin with since it's either initialized to zero on 
> alloc or reset to zero by this line.
> 
> So what are you trying to achieve here with this ->system_state variable?

Sorry I misunderstood your previous remark. This was a test code which
was supposed to be cleaned up before sending.

I will fix this in v3.

Thanks,
Shyam


>