Re: [PATCH] HID: amd_sfh: return an error when response wait times out

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The timeout error codes returned by the patch are silently discarded, rendering the fix ineffective.

Pre-existing issues:
- [High] Data race on the shared cli_data->cur_hid_dev variable causes spurious timeouts and missed wakeups.
- [High] A timeout in amdtp_wait_for_response leaves a stalled request in the queue, permanently desynchronizing the state machine.
--

commit e8a7b9fc56953c32e53def387e9ca9eac4750f78
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

HID: amd_sfh: return an error when response wait times out
    
amdtp_wait_for_response() waits for request_done before completing a
report request. wait_event_interruptible_timeout() returns 0 when the
wait expires, but the current code treats only negative values as errors
and returns success on timeout. This commit returns -ETIMEDOUT when the
response wait expires.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> index b04f675d49b0d..8f88f965fbd55 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> @@ -87,16 +87,17 @@ static int amdtp_wait_for_response(struct hid_device *hid)
>  			break;
>  	}
>  
> -	if (!cli_data->request_done[i])
> +	if (!cli_data->request_done[i]) {
>  		ret = wait_event_interruptible_timeout(hid_data->hid_wait,
>  						       cli_data->request_done[i],
>  						       msecs_to_jiffies(AMD_SFH_RESPONSE_TIMEOUT));
> -	if (ret == -ERESTARTSYS)
> -		return -ERESTARTSYS;
> -	else if (ret < 0)
> -		return -ETIMEDOUT;
> -	else
> -		return 0;
> +		if (ret == -ERESTARTSYS)
> +			return -ERESTARTSYS;
> +		if (ret <= 0)
> +			return -ETIMEDOUT;

[Severity: High]
The patch returns -ETIMEDOUT here, but amdtp_wait_for_response() is registered
as the .wait callback in struct hid_ll_driver.

When called via the wrapper hid_hw_wait(), the return value is discarded:

include/linux/hid.h:hid_hw_wait()
static inline void hid_hw_wait(struct hid_device *hdev)
{
        if (hdev->ll_driver->wait)
                hdev->ll_driver->wait(hdev);
}

Does this mean the caller (HID core) never sees the error code and proceeds as
if the wait succeeded?

[Severity: High]
This isn't a bug introduced by this patch, but if this times out, does it
leave a stalled request in the queue?

In amd_sfh_get_report(), requests are queued to cli_data->req_list. If we
exit here, the request node isn't removed. Later, amd_sfh_work() pops and
processes it:

drivers/hid/amd-sfh-hid/amd_sfh_client.c:amd_sfh_work()
        req_node = list_last_entry(&req_list->list, struct request_list, list);
        list_del(&req_node->list);

Will this cause the background worker to process timed-out requests and
prematurely wake up future requests with stale data, permanently
desynchronizing the request queue?

> +	}
> +
> +	return 0;
>  }
>  
>  void amdtp_hid_wakeup(struct hid_device *hid)
>  {
>  	struct amdtp_hid_data *hid_data;
>  	struct amdtp_cl_data *cli_data;
>  
>  	if (hid) {
>  		hid_data = hid->driver_data;
>  		cli_data = hid_data->cli_data;
>  		cli_data->request_done[cli_data->cur_hid_dev] = true;

[Severity: High]
This is a pre-existing issue, but is there a data race on
cli_data->cur_hid_dev?

amd_sfh_set_report() updates cur_hid_dev locklessly:

drivers/hid/amd-sfh-hid/amd_sfh_client.c:amd_sfh_set_report()
    cli_data->cur_hid_dev = i;

If a SET_REPORT request is issued concurrently with a GET_REPORT worker, could
cur_hid_dev be overwritten, causing amdtp_hid_wakeup() to set request_done for
the wrong device index and leaving the original device's waiter to permanently
time out?

>  		wake_up_interruptible(&hid_data->hid_wait);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624143558.39306-1-pengpeng@iscas.ac.cn?part=1