| Message ID | 20230112190131.24460-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: amd_sfh: if no sensors are enabled, clean up | expand |
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 );
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; 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; } + } else { + mp2_ops->stop(privdata, cl_data->sensor_idx[i]); + status = amd_sfh_wait_for_response + (privdata, cl_data->sensor_idx[i], SENSOR_DISABLED); + cl_data->hid_dev_count--; } 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 || + (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]);
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(-)