[PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[Mario Limonciello]

A crash was reported in amd-sfh related to hid core initialization
before SFH initialization has run.

```
   amdtp_hid_request+0x36/0x50 [amd_sfh
2e3095779aada9fdb1764f08ca578ccb14e41fe4]
   sensor_hub_get_feature+0xad/0x170 [hid_sensor_hub
d6157999c9d260a1bfa6f27d4a0dc2c3e2c5654e]
   hid_sensor_parse_common_attributes+0x217/0x310 [hid_sensor_iio_common
07a7935272aa9c7a28193b574580b3e953a64ec4]
   hid_gyro_3d_probe+0x7f/0x2e0 [hid_sensor_gyro_3d
9f2eb51294a1f0c0315b365f335617cbaef01eab]
   platform_probe+0x44/0xa0
   really_probe+0x19e/0x3e0
```

Ensure that sensors have been set up before calling into
amd_sfh_get_report() or amd_sfh_set_report().

Cc: stable@vger.kernel.org
Cc: Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info>
Fixes: 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors are enabled, clean up")
Reported-by: Haochen Tong <linux@hexchain.org>
Link: https://lore.kernel.org/all/3250319.ancTxkQ2z5@zen/T/
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/amd-sfh-hid/amd_sfh_client.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
index d9b7b01900b5..88f3d913eaa1 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
@@ -25,6 +25,9 @@ void amd_sfh_set_report(struct hid_device *hid, int report_id,
 	struct amdtp_cl_data *cli_data = hid_data->cli_data;
 	int i;
 
+	if (!cli_data->is_any_sensor_enabled)
+		return;
+
 	for (i = 0; i < cli_data->num_hid_devices; i++) {
 		if (cli_data->hid_sensor_hubs[i] == hid) {
 			cli_data->cur_hid_dev = i;
@@ -41,6 +44,9 @@ int amd_sfh_get_report(struct hid_device *hid, int report_id, int report_type)
 	struct request_list *req_list = &cli_data->req_list;
 	int i;
 
+	if (!cli_data->is_any_sensor_enabled)
+		return -ENODEV;
+
 	for (i = 0; i < cli_data->num_hid_devices; i++) {
 		if (cli_data->hid_sensor_hubs[i] == hid) {
 			struct request_list *new = kzalloc(sizeof(*new), GFP_KERNEL);
-- 
2.34.1

Re: [PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[Basavaraj Natikar]

On 6/21/2023 1:09 AM, Mario Limonciello wrote:
> A crash was reported in amd-sfh related to hid core initialization
> before SFH initialization has run.
>
> ```
>    amdtp_hid_request+0x36/0x50 [amd_sfh
> 2e3095779aada9fdb1764f08ca578ccb14e41fe4]
>    sensor_hub_get_feature+0xad/0x170 [hid_sensor_hub
> d6157999c9d260a1bfa6f27d4a0dc2c3e2c5654e]
>    hid_sensor_parse_common_attributes+0x217/0x310 [hid_sensor_iio_common
> 07a7935272aa9c7a28193b574580b3e953a64ec4]
>    hid_gyro_3d_probe+0x7f/0x2e0 [hid_sensor_gyro_3d
> 9f2eb51294a1f0c0315b365f335617cbaef01eab]
>    platform_probe+0x44/0xa0
>    really_probe+0x19e/0x3e0
> ```
>
> Ensure that sensors have been set up before calling into
> amd_sfh_get_report() or amd_sfh_set_report().
>
> Cc: stable@vger.kernel.org
> Cc: Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info>
> Fixes: 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors are enabled, clean up")
> Reported-by: Haochen Tong <linux@hexchain.org>
> Link: https://lore.kernel.org/all/3250319.ancTxkQ2z5@zen/T/
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/hid/amd-sfh-hid/amd_sfh_client.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> index d9b7b01900b5..88f3d913eaa1 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> @@ -25,6 +25,9 @@ void amd_sfh_set_report(struct hid_device *hid, int report_id,
>  	struct amdtp_cl_data *cli_data = hid_data->cli_data;
>  	int i;
>  
> +	if (!cli_data->is_any_sensor_enabled)
> +		return;
> +

can we check below patch series which solves this issue by initializing HID only
if is_any_sensor_enabled
https://lore.kernel.org/all/nycvar.YFH.7.76.2305231559000.29760@cbobk.fhfr.pm/

>  	for (i = 0; i < cli_data->num_hid_devices; i++) {
>  		if (cli_data->hid_sensor_hubs[i] == hid) {
>  			cli_data->cur_hid_dev = i;
> @@ -41,6 +44,9 @@ int amd_sfh_get_report(struct hid_device *hid, int report_id, int report_type)
>  	struct request_list *req_list = &cli_data->req_list;
>  	int i;
>  
> +	if (!cli_data->is_any_sensor_enabled)
> +		return -ENODEV;
> +

can we check below patch series which solves this issue by initializing HID only
if is_any_sensor_enabled.
https://lore.kernel.org/all/nycvar.YFH.7.76.2305231559000.29760@cbobk.fhfr.pm/

Thanks,
--
Basavaraj

>  	for (i = 0; i < cli_data->num_hid_devices; i++) {
>  		if (cli_data->hid_sensor_hubs[i] == hid) {
>  			struct request_list *new = kzalloc(sizeof(*new), GFP_KERNEL);

RE: [PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[Limonciello, Mario]

[Public]

> can we check below patch series which solves this issue by initializing HID only
> if is_any_sensor_enabled.
> https://lore.kernel.org/all/nycvar.YFH.7.76.2305231559000.29760@cbobk.
> fhfr.pm/
>

The original reporter won't be able to test it because they've upgraded their
firmware and SFH is disabled in the new firmware.

But yeah it seems plausible this series could help.  If it comes back up again
we should point anyone affected to this series.

Thanks!

Re: [PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[Thorsten Leemhuis]

On 05.07.23 18:07, Limonciello, Mario wrote:
> [Public]
> 
>> can we check below patch series which solves this issue by initializing HID only
>> if is_any_sensor_enabled.
>> https://lore.kernel.org/all/nycvar.YFH.7.76.2305231559000.29760@cbobk.
>> fhfr.pm/
>>
> 
> The original reporter won't be able to test it because they've upgraded their
> firmware and SFH is disabled in the new firmware.
> 
> But yeah it seems plausible this series could help.  If it comes back up again
> we should point anyone affected to this series.
> 
> Thanks!

Hmmm. So this won't be fixed in 6.3.y. and 6.4.y then, as none of those
patches afaics looks like they will be picked up by the stable team?

Hmmm. That doesn't completely feel right to me, unless we consider the
problem Haochen Ton ran into an extremely unlikely bug (reminder: only a
few of those that encounter a problem will report it). Do we? If not: is
backporting that patch-set to 6.4.y an option once this was in mainline
for a while without causing trouble?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

Re: [PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[Limonciello, Mario]

On 7/6/2023 06:28, Thorsten Leemhuis wrote:
> On 05.07.23 18:07, Limonciello, Mario wrote:
>> [Public]
>>
>>> can we check below patch series which solves this issue by initializing HID only
>>> if is_any_sensor_enabled.
>>> https://lore.kernel.org/all/nycvar.YFH.7.76.2305231559000.29760@cbobk.
>>> fhfr.pm/
>>>
>>
>> The original reporter won't be able to test it because they've upgraded their
>> firmware and SFH is disabled in the new firmware.
>>
>> But yeah it seems plausible this series could help.  If it comes back up again
>> we should point anyone affected to this series.
>>
>> Thanks!
> 
> Hmmm. So this won't be fixed in 6.3.y. and 6.4.y then, as none of those
> patches afaics looks like they will be picked up by the stable team?
> 
> Hmmm. That doesn't completely feel right to me, unless we consider the
> problem Haochen Ton ran into an extremely unlikely bug (reminder: only a
> few of those that encounter a problem will report it). Do we? If not: is
> backporting that patch-set to 6.4.y an option once this was in mainline
> for a while without causing trouble?

So the problem that was found is the following set of circumstances:
1) System that doesn't have sensors connected to SFH
2) System that has SFH enabled in BIOS

As this is already fixed in BIOS update for affected laptop that 
disabled SFH and the vendor publishes the BIOS to LVFS, I think it's 
unlikely to crop back up on this model.

If anything it might crop up on a different model that meets what I said 
above.  If that happens, I suggest we ask any potential future reporters 
to test that series.

We can always backport it to remaining stable channels if it comes back.

> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.

[PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report

[Mario Limonciello]

A crash was reported in amd-sfh related to hid core initialization
before SFH initialization has run.

```
   amdtp_hid_request+0x36/0x50 [amd_sfh
2e3095779aada9fdb1764f08ca578ccb14e41fe4]
   sensor_hub_get_feature+0xad/0x170 [hid_sensor_hub
d6157999c9d260a1bfa6f27d4a0dc2c3e2c5654e]
   hid_sensor_parse_common_attributes+0x217/0x310 [hid_sensor_iio_common
07a7935272aa9c7a28193b574580b3e953a64ec4]
   hid_gyro_3d_probe+0x7f/0x2e0 [hid_sensor_gyro_3d
9f2eb51294a1f0c0315b365f335617cbaef01eab]
   platform_probe+0x44/0xa0
   really_probe+0x19e/0x3e0
```

Ensure that sensors have been set up before calling into
amd_sfh_get_report() or amd_sfh_set_report().

Cc: stable@vger.kernel.org
Cc: Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info>
Fixes: 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors are enabled, clean up")
Reported-by: Haochen Tong <linux@hexchain.org>
Link: https://lore.kernel.org/all/3250319.ancTxkQ2z5@zen/T/
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/amd-sfh-hid/amd_sfh_client.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
index d9b7b01900b5..88f3d913eaa1 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
@@ -25,6 +25,9 @@ void amd_sfh_set_report(struct hid_device *hid, int report_id,
 	struct amdtp_cl_data *cli_data = hid_data->cli_data;
 	int i;
 
+	if (!cli_data->is_any_sensor_enabled)
+		return;
+
 	for (i = 0; i < cli_data->num_hid_devices; i++) {
 		if (cli_data->hid_sensor_hubs[i] == hid) {
 			cli_data->cur_hid_dev = i;
@@ -41,6 +44,9 @@ int amd_sfh_get_report(struct hid_device *hid, int report_id, int report_type)
 	struct request_list *req_list = &cli_data->req_list;
 	int i;
 
+	if (!cli_data->is_any_sensor_enabled)
+		return -ENODEV;
+
 	for (i = 0; i < cli_data->num_hid_devices; i++) {
 		if (cli_data->hid_sensor_hubs[i] == hid) {
 			struct request_list *new = kzalloc(sizeof(*new), GFP_KERNEL);
-- 
2.34.1