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 <l...@intel.com>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/202309260515.5xbr6n0g-...@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....@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
>
