On 2/4/2023 3:38 AM, Mario Limonciello wrote:
> It was reported that commit b300667b33b2 ("HID: amd_sfh: Disable the
> interrupt for all command") had caused increased resume time on HP Envy
> x360.
>
> Before this commit 3 sensors were reported, but they were not actually
> functional. After this commit the sensors are no longer reported, but
> also the resume time increased.
>
> To avoid this problem explicitly look for the number of disabled sensors.
> If all the sensors are disabled, clean everything up.
>
> Fixes: b300667b33b2 ("HID: amd_sfh: Disable the interrupt for all command")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2115
> Reported-by: Xaver Hugl <xaver.hugl@xxxxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v1->v2:
> * Use a boolean parameter instead
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_client.c | 13 +++++++++++--
> drivers/hid/amd-sfh-hid/amd_sfh_hid.h | 1 +
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> index 1fb0f7105fb21..c751d12f5df89 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> @@ -227,6 +227,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
> cl_data->num_hid_devices = amd_mp2_get_sensor_num(privdata, &cl_data->sensor_idx[0]);
> if (cl_data->num_hid_devices == 0)
> return -ENODEV;
> + cl_data->is_any_sensor_enabled = false;
>
> INIT_DELAYED_WORK(&cl_data->work, amd_sfh_work);
> INIT_DELAYED_WORK(&cl_data->work_buffer, amd_sfh_work_buffer);
> @@ -287,6 +288,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
> status = amd_sfh_wait_for_response
> (privdata, cl_data->sensor_idx[i], SENSOR_ENABLED);
> if (status == SENSOR_ENABLED) {
> + cl_data->is_any_sensor_enabled = true;
> cl_data->sensor_sts[i] = SENSOR_ENABLED;
> rc = amdtp_hid_probe(cl_data->cur_hid_dev, cl_data);
> if (rc) {
> @@ -301,19 +303,26 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
> cl_data->sensor_sts[i]);
> goto cleanup;
> }
> + } else {
> + cl_data->sensor_sts[i] = SENSOR_DISABLED;
> + dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> + cl_data->sensor_idx[i],
> + get_sensor_name(cl_data->sensor_idx[i]),
> + cl_data->sensor_sts[i]);
> }
> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]),
> cl_data->sensor_sts[i]);
> }
> - if (mp2_ops->discovery_status && mp2_ops->discovery_status(privdata) == 0) {
> + if (!cl_data->is_any_sensor_enabled ||
> + (mp2_ops->discovery_status && mp2_ops->discovery_status(privdata) == 0)) {
> amd_sfh_hid_client_deinit(privdata);
> for (i = 0; i < cl_data->num_hid_devices; i++) {
> devm_kfree(dev, cl_data->feature_report[i]);
> devm_kfree(dev, in_data->input_report[i]);
> devm_kfree(dev, cl_data->report_descr[i]);
> }
> - dev_warn(dev, "Failed to discover, sensors not enabled\n");
> + dev_warn(dev, "Failed to discover, sensors not enabled is %d\n", cl_data->is_any_sensor_enabled);
> return -EOPNOTSUPP;
> }
> schedule_delayed_work(&cl_data->work_buffer, msecs_to_jiffies(AMD_SFH_IDLE_LOOP));
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
> index 3754fb423e3ae..528036892c9d2 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
> @@ -32,6 +32,7 @@ struct amd_input_data {
> struct amdtp_cl_data {
> u8 init_done;
> u32 cur_hid_dev;
> + bool is_any_sensor_enabled;
> u32 hid_dev_count;
> u32 num_hid_devices;
> struct device_info *hid_devices;
Changes looks good to me
Acked-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
Thanks,
--
Basavaraj
