diff mbox series

[v2] HID: amd_sfh: if no sensors are enabled, clean up

Message ID 20230203220850.13924-1-mario.limonciello@amd.com (mailing list archive)
State Mainlined
Commit 7bcfdab3f0c6672ca52be3cb65a0550d8b99554b
Delegated to: Jiri Kosina
Headers show
Series [v2] HID: amd_sfh: if no sensors are enabled, clean up | expand

Commit Message

Mario Limonciello Feb. 3, 2023, 10:08 p.m. UTC 
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@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
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(-)

Comments

Basavaraj Natikar Feb. 6, 2023, 5:52 a.m. UTC | #1 
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@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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@amd.com>

Thanks,
--
Basavaraj
Benjamin Tissoires Feb. 6, 2023, 1:31 p.m. UTC | #2 
On Fri, 03 Feb 2023 16:08:49 -0600, 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.
> 
> [...]

Applied to hid/hid.git (for-6.2/upstream-fixes), thanks!

[1/1] HID: amd_sfh: if no sensors are enabled, clean up
      https://git.kernel.org/hid/hid/c/7bcfdab3f0c6

Cheers,
diff mbox series

Patch

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;