Re: HID: Introduce hidpp, a module to handle Logitech hid++ devices

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

Hi Dan,

On Oct 31 2014 or thereabouts, Dan Carpenter wrote:
> Hello Benjamin Tissoires,
> 
> The patch 2f31c5252910: "HID: Introduce hidpp, a module to handle
> Logitech hid++ devices" from Sep 30, 2014, leads to the following
> static checker warning:
> 
> 	drivers/hid/hid-logitech-hidpp.c:359 hidpp_root_get_protocol_version()
> 	warn: should this return really be negated?
> 
> drivers/hid/hid-logitech-hidpp.c
>    342  static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
>    343  {
>    344          struct hidpp_report response;
>    345          int ret;
>    346  
>    347          ret = hidpp_send_fap_command_sync(hidpp,
>    348                          HIDPP_PAGE_ROOT_IDX,
>    349                          CMD_ROOT_GET_PROTOCOL_VERSION,
>    350                          NULL, 0, &response);
>    351  
>    352          if (ret == 1) {
>                     ^^^^^^^^
> What does the "1" mean?  Magic numbers are bad, yada yada yada.

Indeed. It should be HIDPP_ERROR_INVALID_SUBID.

> 
>    353                  hidpp->protocol_major = 1;
>    354                  hidpp->protocol_minor = 0;
>    355                  return 0;
>    356          }
>    357  
>    358          if (ret)
>    359                  return -ret;
>                         ^^^^^^^^^^^
> This is wrong.  The real problem is that hidpp_send_fap_command_sync()
> mixes normal and custom error codes.  The callers are inconsistent in
> how they deal with it.
> 

Yep :/

So, to sum up, positive errors are errors handled by the protocol.
Negative ones are the normal error codes.
I guess if the error is positive, we should drop an hid_err in the
syslog and convert it into -EPROTO.

>    360  
>    361          hidpp->protocol_major = response.fap.params[0];
>    362          hidpp->protocol_minor = response.fap.params[1];
>    363  
>    364          return ret;
>    365  }
> 
> See also:
> 
> drivers/hid/hid-logitech-hidpp.c:398 hidpp_devicenametype_get_count() warn: should this return really be negated?
> drivers/hid/hid-logitech-hidpp.c:417 hidpp_devicenametype_get_device_name() warn: should this return really be negated?
> drivers/hid/hid-logitech-hidpp.c:524 hidpp_touchpad_get_raw_info() warn: should this return really be negated?

Same will apply for these 3 others negated err.

Thanks for the reports!

Cheers,
Benjamin