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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6561b9c1-4256-46c4-68de-bc9d06c7a9f9@amd.com>
Date:   Fri, 3 Feb 2023 12:44:02 +0530
From:   Basavaraj Natikar <bnatikar@....com>
To:     Mario Limonciello <mario.limonciello@....com>,
        Basavaraj Natikar <basavaraj.natikar@....com>,
        Jiri Kosina <jkosina@...e.cz>
Cc:     Xaver Hugl <xaver.hugl@...il.com>, Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: amd_sfh: if no sensors are enabled, clean up


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@...il.com>
> Tested-by: Xaver Hugl <xaver.hugl@...il.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@....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 
);