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

[Basavaraj Natikar <bnatikar@amd.com>]

On 1/13/2023 12:31 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>
> Tested-by: Xaver Hugl <xaver.hugl@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/hid/amd-sfh-hid/amd_sfh_client.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> index ab125f79408f2..7469691b4a633 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->hid_dev_count = cl_data->num_hid_devices;

to manage more effectively. we can remove above statement by 
declaring bool is_any_sensors_enabled = false; in declaration block or in 
cl_data

>  
>  	INIT_DELAYED_WORK(&cl_data->work, amd_sfh_work);
>  	INIT_DELAYED_WORK(&cl_data->work_buffer, amd_sfh_work_buffer);
> @@ -301,12 +302,18 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
>  					cl_data->sensor_sts[i]);
>  				goto cleanup;
>  			}

we can add else or just beginning of this block as
	is_any_sensors_enabled = true;

> +		} else {
> +			mp2_ops->stop(privdata, cl_data->sensor_idx[i]);
> +			status = amd_sfh_wait_for_response
> +				(privdata, cl_data->sensor_idx[i], SENSOR_DISABLED);

slightly more checks even not required but good to have to make sure in right path
i.e. return status is not used so can we make it like below
if (status != SENSOR_ENABLED) {
	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]);
}

> +			cl_data->hid_dev_count--;

not needed we can use is_any_sensors_enabled    

>  		}
>  		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->hid_dev_count ||

can be changed if (is_any_sensors_enabled == false ||

> +	   (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++) {
>
>
more debug information like
dev_warn(dev, "Failed to discover, sensors not enabled is_any_sensors_enabled %d\n", is_any_sensors_enabled 
);