Reported by Dan Carpenter:
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: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?
The problem lies in hidpp_send_message_sync() which can return 2 types of
errors depending of their sign. Adding a comment there to clarify what is
happening.
To solve that, print an error in case of a protocol problem, and raise
-EPROTO instead.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 36 ++++++++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 8d2d54b..4793d4b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -151,6 +151,14 @@ static int __hidpp_send_report(struct hid_device *hdev,
return ret == fields_count ? 0 : -1;
}
+/**
+ * hidpp_send_message_sync() returns 0 in case of success, and something else
+ * in case of a failure.
+ * - If ' something else' is positive, that means that an error has been raised
+ * by the protocol itself.
+ * - If ' something else' is negative, that means that we had a classic error
+ * (-ENOMEM, -EPIPE, etc...)
+ */
static int hidpp_send_message_sync(struct hidpp_device *hidpp,
struct hidpp_report *message,
struct hidpp_report *response)
@@ -359,8 +367,11 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
return 0;
}
- if (ret)
- return -ret;
+ if (ret > 0) {
+ hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
hidpp->protocol_major = response.fap.params[0];
hidpp->protocol_minor = response.fap.params[1];
@@ -398,8 +409,11 @@ static int hidpp_devicenametype_get_count(struct hidpp_device *hidpp,
ret = hidpp_send_fap_command_sync(hidpp, feature_index,
CMD_GET_DEVICE_NAME_TYPE_GET_COUNT, NULL, 0, &response);
- if (ret)
- return -ret;
+ if (ret > 0) {
+ hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
*nameLength = response.fap.params[0];
@@ -417,8 +431,11 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME, &char_index, 1,
&response);
- if (ret)
- return -ret;
+ if (ret > 0) {
+ hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
if (response.report_id == REPORT_ID_HIDPP_LONG)
count = HIDPP_REPORT_LONG_LENGTH - 4;
@@ -524,8 +541,11 @@ static int hidpp_touchpad_get_raw_info(struct hidpp_device *hidpp,
ret = hidpp_send_fap_command_sync(hidpp, feature_index,
CMD_TOUCHPAD_GET_RAW_INFO, NULL, 0, &response);
- if (ret)
- return -ret;
+ if (ret > 0) {
+ hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
raw_info->x_size = get_unaligned_be16(¶ms[0]);
raw_info->y_size = get_unaligned_be16(¶ms[2]);
--
2.1.0
- remove the constant '1'
- when the device is not connected, the protocol error
HIDPP_ERROR_RESOURCE_ERROR is raised. We should not warn the user about
it because it is somewhat expected as an answer when we check if the
device is connected.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4793d4b..2683564 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -361,12 +361,16 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
CMD_ROOT_GET_PROTOCOL_VERSION,
NULL, 0, &response);
- if (ret == 1) {
+ if (ret == HIDPP_ERROR_INVALID_SUBID) {
hidpp->protocol_major = 1;
hidpp->protocol_minor = 0;
return 0;
}
+ /* the device might not be connected */
+ if (ret == HIDPP_ERROR_RESOURCE_ERROR)
+ return -EIO;
+
if (ret > 0) {
hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
__func__, ret);
--
2.1.0
On Mon, Nov 03, 2014 at 02:50:52PM -0500, Benjamin Tissoires wrote:
> @@ -524,8 +541,11 @@ static int hidpp_touchpad_get_raw_info(struct hidpp_device *hidpp,
> ret = hidpp_send_fap_command_sync(hidpp, feature_index,
> CMD_TOUCHPAD_GET_RAW_INFO, NULL, 0, &response);
>
> - if (ret)
> - return -ret;
> + if (ret > 0) {
> + hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
> + __func__, ret);
> + return -EPROTO;
> + }
We should handle -ENOMEM and -EINVAL here as well. Something like:
if (ret > 0) {
hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
__func__, ret);
ret = -EPROTO;
}
if (ret)
return ret;
regards,
dan carpenter
On Mon, Nov 3, 2014 at 3:32 PM, Dan Carpenter <[email protected]> wrote:
> On Mon, Nov 03, 2014 at 02:50:52PM -0500, Benjamin Tissoires wrote:
>> @@ -524,8 +541,11 @@ static int hidpp_touchpad_get_raw_info(struct hidpp_device *hidpp,
>> ret = hidpp_send_fap_command_sync(hidpp, feature_index,
>> CMD_TOUCHPAD_GET_RAW_INFO, NULL, 0, &response);
>>
>> - if (ret)
>> - return -ret;
>> + if (ret > 0) {
>> + hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
>> + __func__, ret);
>> + return -EPROTO;
>> + }
>
> We should handle -ENOMEM and -EINVAL here as well. Something like:
>
> if (ret > 0) {
> hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
> __func__, ret);
> ret = -EPROTO;
> }
> if (ret)
> return ret;
Ouch. my bad...
No, I don't have any excuses for that.
Thanks for the review.
Cheers,
Benjamin