On 7/29/2024 17:25, Ilpo Järvinen wrote:
> On Tue, 23 Jul 2024, Shyam Sundar S K wrote:
>
>> If the Ambient Light Sensor (ALS) is disabled, the current code in the PMF
>> driver does not query for Human Presence Detection (HPD) data in
>> amd_pmf_get_sensor_info(). As a result, stale HPD data is used by PMF-TA
>> to evaluate policy conditions, leading to unexpected behavior in the policy
>> output actions.
>>
>> To resolve this issue, modify the PMF driver to query HPD data
>> independently of ALS.
>>
>> With this change, amd_pmf_get_sensor_info() now returns void instead of
>> int.
>>
>> Fixes: cedecdba60f4 ("platform/x86/amd/pmf: Get ambient light information from AMD SFH driver")
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>> ---
>> drivers/platform/x86/amd/pmf/spc.c | 33 +++++++++++++++---------------
>> 1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
>> index a3dec14c3004..d9e60d63553c 100644
>> --- a/drivers/platform/x86/amd/pmf/spc.c
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -150,7 +150,7 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>> return 0;
>> }
>>
>> -static int amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
>> +static void amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
>> {
>> struct amd_sfh_info sfh_info;
>> int ret;
>> @@ -160,26 +160,25 @@ static int amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>> if (!ret)
>> in->ev_info.ambient_light = sfh_info.ambient_light;
>> else
>> - return ret;
>> + dev_dbg(dev->dev, "ALS is not enabled\n");
>>
>> /* get HPD data */
>> ret = amd_get_sfh_info(&sfh_info, MT_HPD);
>> - if (ret)
>> - return ret;
>> -
>> - switch (sfh_info.user_present) {
>> - case SFH_NOT_DETECTED:
>> - in->ev_info.user_present = 0xff; /* assume no sensors connected */
>> - break;
>> - case SFH_USER_PRESENT:
>> - in->ev_info.user_present = 1;
>> - break;
>> - case SFH_USER_AWAY:
>> - in->ev_info.user_present = 0;
>> - break;
>> + if (!ret) {
>> + switch (sfh_info.user_present) {
>> + case SFH_NOT_DETECTED:
>> + in->ev_info.user_present = 0xff; /* assume no sensors connected */
>> + break;
>> + case SFH_USER_PRESENT:
>> + in->ev_info.user_present = 1;
>> + break;
>> + case SFH_USER_AWAY:
>> + in->ev_info.user_present = 0;
>> + break;
>> + }
>> + } else {
>> + dev_dbg(dev->dev, "HPD is not enabled\n");
>
> Is it okay to leave in->ev_info.user_present as 0 this case? Should it be
> set to same as with SFH_NOT_DETECTED?
>
Valid point. Realized that the PMF TA only accepts a boolean and it
only looks for information on if the user is present or not.
I have adjusted this behavior in v2. Please take a look.
> ...I now realize your print out doesn't cover that case at all and
> user_present is not used for anything else than debug printing.
>
entire populated "ev_info" is passed to the PMF TA to evaluate the
policy conditions. So, its just not limited to debug message.
Thanks,
Shyam
