Re: [PATCH 01/12] HID: asus: reinitialize the device after exiting a sleep state

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added error handling blocks in `asus_probe()` and `asus_reset_resume()` are completely dead code because `asus_initialize_reports()` unconditionally returns 0.
--

--- Patch [1]: [PATCH 01/12] HID: asus: reinitialize the device after exiting a sleep state ---
commit 0ca493a924cc77202a10af1860055debe0b725bc
Author: Denis Benato <[email protected]>

HID: asus: reinitialize the device after exiting a sleep state

This patch extracts keyboard initialization into a helper function and
calls it during reset resume, ensuring devices like the ROG ally have
their EC string sent back after s2idle.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1360,6 +1360,22 @@ static int asus_start_multitouch(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static int asus_initialize_reports(struct hid_device *hdev)
> +{
> +	int ret;
> +
> +	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
> +		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
> +			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
> +			if (ret < 0)
> +				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
> +					 asus_report_id_init[r], ret);
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Does this helper function need to return the error code? 

Because it unconditionally returns 0, the error codes captured in the ret
variable from asus_kbd_init() are dropped after the warning is printed.

>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
> +
> +	ret = asus_initialize_reports(hdev);
> +	if (ret) {
> +		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
> +		goto asus_reset_resume_err;
> +	}

[Severity: Low]
Since asus_initialize_reports() unconditionally returns 0, does that make
this error path dead code? 

If the intent was to abort on initialization failure, should the helper return
the error code instead?

[ ... ]

> @@ -1493,13 +1518,10 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> -	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
> -		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
> -			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
> -			if (ret < 0)
> -				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
> -					 asus_report_id_init[r], ret);
> -		}
> +	ret = asus_initialize_reports(hdev);
> +	if (ret) {
> +		hid_err(hdev, "Asus initialize reports failed: %d\n", ret);
> +		goto err_stop_hw;
>  	}

[Severity: Low]
Similar to the check in asus_reset_resume(), is this error handling reachable?

The original code in asus_probe() only printed a warning on failure and
continued, so returning 0 preserves the old behavior, but it leaves this new
goto err_stop_hw path inaccessible.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1