[PATCH v2 0/2] Fixes for SRA sensor handling

[PATCH v2 0/2] Fixes for SRA sensor handling

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

On systems with an SRA sensor there are some problems with both init
and failure paths.  This series accounts for both.

Mario Limonciello (2):
  HID: amd_sfh: Fix SRA sensor when it's the only sensor
  HID: amd_sfh: Cleanup all sensors when init fails

 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] HID: amd_sfh: Fix SRA sensor when it's the only sensor

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

On systems that only have an SRA sensor connected to SFH the sensor
doesn't get enabled due to a bad optimization condition of breaking
the sensor walk loop.

This optimization is unnecessary in the first place because if there
is only one device then the loop only runs once. Drop the condition
and explicitly mark sensor as enabled.

Reported-by: Yijun Shen <Yijun.Shen@dell.com>
Tested-By: Yijun Shen <Yijun_Shen@Dell.com>
Fixes: d1c444b47100d ("HID: amd_sfh: Add support to export device operating states")
Cc: stable@vger.kernel.org
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Add tag
---
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index 25f0ebfcbd5f5..c1bdf1e0d44af 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -134,9 +134,6 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
 	for (i = 0; i < cl_data->num_hid_devices; i++) {
 		cl_data->sensor_sts[i] = SENSOR_DISABLED;
 
-		if (cl_data->num_hid_devices == 1 && cl_data->sensor_idx[0] == SRA_IDX)
-			break;
-
 		if (cl_data->sensor_idx[i] == SRA_IDX) {
 			info.sensor_idx = cl_data->sensor_idx[i];
 			writel(0, privdata->mmio + amd_get_p2c_val(privdata, 0));
@@ -145,8 +142,10 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
 				(privdata, cl_data->sensor_idx[i], ENABLE_SENSOR);
 
 			cl_data->sensor_sts[i] = (status == 0) ? SENSOR_ENABLED : SENSOR_DISABLED;
-			if (cl_data->sensor_sts[i] == SENSOR_ENABLED)
+			if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
+				cl_data->is_any_sensor_enabled = true;
 				privdata->dev_en.is_sra_present = true;
+			}
 			continue;
 		}
 
-- 
2.43.0

[PATCH v2 2/2] HID: amd_sfh: Cleanup all sensors when init fails

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

If any sensors have been started when a failure occurs during client
init they are left enabled by the firmware.

As part of the cleanup routine check all sensors and if they're enabled
stop them.

Suggested-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * New patch
---
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index c1bdf1e0d44af..c61cc42fa7a33 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -235,12 +235,20 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
 	return 0;
 
 cleanup:
-	amd_sfh_hid_client_deinit(privdata);
 	for (i = 0; i < cl_data->num_hid_devices; i++) {
+		if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
+			mp2_ops->stop(privdata, cl_data->sensor_idx[i]);
+			status = amd_sfh_wait_for_response
+					(privdata, cl_data->sensor_idx[i], DISABLE_SENSOR);
+			cl_data->sensor_sts[i] = (status == 0) ? SENSOR_ENABLED : SENSOR_DISABLED;
+		}
+		if (cl_data->sensor_idx[i] == SRA_IDX)
+			continue;
 		devm_kfree(dev, cl_data->feature_report[i]);
 		devm_kfree(dev, in_data->input_report[i]);
 		devm_kfree(dev, cl_data->report_descr[i]);
 	}
+	amd_sfh_hid_client_deinit(privdata);
 	return rc;
 }
 
-- 
2.43.0

Re: [PATCH v2 2/2] HID: amd_sfh: Cleanup all sensors when init fails

[Basavaraj Natikar]

On 4/3/2025 8:29 PM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> If any sensors have been started when a failure occurs during client
> init they are left enabled by the firmware.
>
> As part of the cleanup routine check all sensors and if they're enabled
> stop them.
>
> Suggested-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>   * New patch
> ---
>   drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> index c1bdf1e0d44af..c61cc42fa7a33 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> @@ -235,12 +235,20 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
>   	return 0;
>   
>   cleanup:
> -	amd_sfh_hid_client_deinit(privdata);
>   	for (i = 0; i < cl_data->num_hid_devices; i++) {
> +		if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
> +			mp2_ops->stop(privdata, cl_data->sensor_idx[i]);
> +			status = amd_sfh_wait_for_response
> +					(privdata, cl_data->sensor_idx[i], DISABLE_SENSOR);
> +			cl_data->sensor_sts[i] = (status == 0) ? SENSOR_ENABLED : SENSOR_DISABLED;
> +		}

can we only add below condition as above case is already taken care in
amd_sfh_hid_client_deinit

Thanks,
--
Basavaraj

> +		if (cl_data->sensor_idx[i] == SRA_IDX)
> +			continue;
>   		devm_kfree(dev, cl_data->feature_report[i]);
>   		devm_kfree(dev, in_data->input_report[i]);
>   		devm_kfree(dev, cl_data->report_descr[i]);
>   	}
> +	amd_sfh_hid_client_deinit(privdata);
>   	return rc;
>   }
>