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

[Basavaraj Natikar]

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