| Message ID | 20240723132451.3488326-2-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Ilpo Järvinen |
| Headers | show |
| Series | [1/2] platform/x86/amd/pmf: Add new ACPI ID AMDI0107 | expand |
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@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/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? ...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.
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@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/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
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"); } - - return 0; } void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
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@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/spc.c | 33 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 17 deletions(-)