* re: HID: Introduce hidpp, a module to handle Logitech hid++ devices
@ 2014-10-31 9:15 Dan Carpenter
2014-10-31 13:55 ` Benjamin Tissoires
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2014-10-31 9:15 UTC (permalink / raw)
To: benjamin.tissoires; +Cc: linux-input
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.
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.
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?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: HID: Introduce hidpp, a module to handle Logitech hid++ devices
2014-10-31 9:15 HID: Introduce hidpp, a module to handle Logitech hid++ devices Dan Carpenter
@ 2014-10-31 13:55 ` Benjamin Tissoires
0 siblings, 0 replies; 2+ messages in thread
From: Benjamin Tissoires @ 2014-10-31 13:55 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-input, Jiri Kosina
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-10-31 13:55 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-31 9:15 HID: Introduce hidpp, a module to handle Logitech hid++ devices Dan Carpenter
2014-10-31 13:55 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).