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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
