Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[Bastien Nocera <hadess@hadess.net>]

Hey Dan,

On Thu, 2023-09-07 at 12:55 +0300, Dan Carpenter wrote:
> The hid_hw_raw_request() function returns negative error codes or the
> number bytes transferred.  If it returns negative error codes, then
> the
> problem is that when we check if "ret <
> sizeof(arctis_1_battery_request)",
> negative values are type promoted to high unsigned values and treated
> as
> success.  Add an explicit check for negatives to address this issue.

I would be grateful if you could either add the compiler's error
message or explain in greater details the integer type promotion (or
demotion from signed to unsigned in this case) in the commit message.

> Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1
> XBox")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>



> ---
>  drivers/hid/hid-steelseries.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index 43d2cf7153d7..485d2287d58a 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -390,7 +390,7 @@ static int
> steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev)
>         ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0],
>                                  write_buf,
> sizeof(arctis_1_battery_request),
>                                  HID_OUTPUT_REPORT,
> HID_REQ_SET_REPORT);
> -       if (ret < sizeof(arctis_1_battery_request)) {
> +       if (ret < 0 || ret < sizeof(arctis_1_battery_request)) {

I prefer:
-       if (ret < sizeof(arctis_1_battery_request)) {
+       if (ret < (int) sizeof(arctis_1_battery_request)) {

although I'm not sure that's the kernel-style for this sort of checks.

>                 hid_err(hdev, "hid_hw_raw_request() failed with
> %d\n", ret);
>                 ret = -ENODATA;
>         }