From 61595012f28036a58293df5a2ab75f80ca15c327 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Tue, 1 Oct 2024 08:42:36 -0700 Subject: [PATCH 1/6] HID: simplify code in fetch_item() We can easily calculate the size of the item using arithmetic (shifts). This allows to pull duplicated code out of the switch statement, making it cleaner. Signed-off-by: Dmitry Torokhov Link: https://patch.msgid.link/ZvwYbESMZ667QZqY@google.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 30de92d0bf0f..0dc3018de2a6 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -754,35 +754,32 @@ static const u8 *fetch_item(const __u8 *start, const __u8 *end, struct hid_item } item->format = HID_ITEM_FORMAT_SHORT; - item->size = b & 3; + item->size = BIT(b & 3) >> 1; /* 0, 1, 2, 3 -> 0, 1, 2, 4 */ + + if (end - start < item->size) + return NULL; switch (item->size) { case 0: - return start; + break; case 1: - if ((end - start) < 1) - return NULL; - item->data.u8 = *start++; - return start; + item->data.u8 = *start; + break; case 2: - if ((end - start) < 2) - return NULL; item->data.u16 = get_unaligned_le16(start); - start = (__u8 *)((__le16 *)start + 1); - return start; + break; - case 3: - item->size++; - if ((end - start) < 4) - return NULL; + case 4: item->data.u32 = get_unaligned_le32(start); - start = (__u8 *)((__le32 *)start + 1); - return start; + break; + + default: + unreachable(); } - return NULL; + return start + item->size; } static void hid_scan_input_usage(struct hid_parser *parser, u32 usage) From ae9b956cb26c0fd5a365629f2d723ab2fb14df79 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Thu, 3 Oct 2024 07:46:50 -0700 Subject: [PATCH 2/6] HID: simplify snto32() snto32() does exactly what sign_extend32() does, but handles potentially malformed data coming from the device. Keep the checks, but then call sign_extend32() to perform the actual conversion. Signed-off-by: Dmitry Torokhov Link: https://patch.msgid.link/20241003144656.3786064-1-dmitry.torokhov@gmail.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 0dc3018de2a6..a6fe04e1c726 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1313,9 +1313,7 @@ alloc_err: EXPORT_SYMBOL_GPL(hid_open_report); /* - * Convert a signed n-bit integer to signed 32-bit integer. Common - * cases are done through the compiler, the screwed things has to be - * done by hand. + * Convert a signed n-bit integer to signed 32-bit integer. */ static s32 snto32(__u32 value, unsigned n) @@ -1326,12 +1324,7 @@ static s32 snto32(__u32 value, unsigned n) if (n > 32) n = 32; - switch (n) { - case 8: return ((__s8)value); - case 16: return ((__s16)value); - case 32: return ((__s32)value); - } - return value & (1 << (n - 1)) ? value | (~0U << n) : value; + return sign_extend32(value, n - 1); } s32 hid_snto32(__u32 value, unsigned n) From c653ffc283404a6c1c0e65143a833180c7ff799b Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Thu, 3 Oct 2024 07:46:51 -0700 Subject: [PATCH 3/6] HID: stop exporting hid_snto32() The only user of hid_snto32() is Logitech HID++ driver, which always calls hid_snto32() with valid size (constant, either 12 or 8) and therefore can simply use sign_extend32(). Make the switch and remove hid_snto32(). Move snto32() and s32ton() to avoid introducing forward declaration. Signed-off-by: Dmitry Torokhov Link: https://patch.msgid.link/20241003144656.3786064-2-dmitry.torokhov@gmail.com [bentiss: fix checkpatch warning] Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 63 +++++++++++++++----------------- drivers/hid/hid-logitech-hidpp.c | 6 +-- include/linux/hid.h | 1 - 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index a6fe04e1c726..87b2cdd7f0c4 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -45,6 +45,34 @@ static int hid_ignore_special_drivers = 0; module_param_named(ignore_special_drivers, hid_ignore_special_drivers, int, 0600); MODULE_PARM_DESC(ignore_special_drivers, "Ignore any special drivers and handle all devices by generic driver"); +/* + * Convert a signed n-bit integer to signed 32-bit integer. + */ + +static s32 snto32(__u32 value, unsigned int n) +{ + if (!value || !n) + return 0; + + if (n > 32) + n = 32; + + return sign_extend32(value, n - 1); +} + +/* + * Convert a signed 32-bit integer to a signed n-bit integer. + */ + +static u32 s32ton(__s32 value, unsigned int n) +{ + s32 a = value >> (n - 1); + + if (a && a != -1) + return value < 0 ? 1 << (n - 1) : (1 << (n - 1)) - 1; + return value & ((1 << n) - 1); +} + /* * Register a new report for a device. */ @@ -425,7 +453,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item) * both this and the standard encoding. */ raw_value = item_sdata(item); if (!(raw_value & 0xfffffff0)) - parser->global.unit_exponent = hid_snto32(raw_value, 4); + parser->global.unit_exponent = snto32(raw_value, 4); else parser->global.unit_exponent = raw_value; return 0; @@ -1312,39 +1340,6 @@ alloc_err: } EXPORT_SYMBOL_GPL(hid_open_report); -/* - * Convert a signed n-bit integer to signed 32-bit integer. - */ - -static s32 snto32(__u32 value, unsigned n) -{ - if (!value || !n) - return 0; - - if (n > 32) - n = 32; - - return sign_extend32(value, n - 1); -} - -s32 hid_snto32(__u32 value, unsigned n) -{ - return snto32(value, n); -} -EXPORT_SYMBOL_GPL(hid_snto32); - -/* - * Convert a signed 32-bit integer to a signed n-bit integer. - */ - -static u32 s32ton(__s32 value, unsigned n) -{ - s32 a = value >> (n - 1); - if (a && a != -1) - return value < 0 ? 1 << (n - 1) : (1 << (n - 1)) - 1; - return value & ((1 << n) - 1); -} - /* * Extract/implement a data field from/to a little endian report (bit array). * diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 0e33fa0eb8db..0d114268ade8 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3296,13 +3296,13 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size) 120); } - v = hid_snto32(hid_field_extract(hdev, data+3, 0, 12), 12); + v = sign_extend32(hid_field_extract(hdev, data + 3, 0, 12), 11); input_report_rel(hidpp->input, REL_X, v); - v = hid_snto32(hid_field_extract(hdev, data+3, 12, 12), 12); + v = sign_extend32(hid_field_extract(hdev, data + 3, 12, 12), 11); input_report_rel(hidpp->input, REL_Y, v); - v = hid_snto32(data[6], 8); + v = sign_extend32(data[6], 7); if (v != 0) hidpp_scroll_counter_handle_scroll(hidpp->input, &hidpp->vertical_wheel_counter, v); diff --git a/include/linux/hid.h b/include/linux/hid.h index 121d5b8bc867..f7c3a66647fd 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -974,7 +974,6 @@ const struct hid_device_id *hid_match_device(struct hid_device *hdev, struct hid_driver *hdrv); bool hid_compare_device_paths(struct hid_device *hdev_a, struct hid_device *hdev_b, char separator); -s32 hid_snto32(__u32 value, unsigned n); __u32 hid_field_extract(const struct hid_device *hid, __u8 *report, unsigned offset, unsigned n); From aa68d2bd9befe8615852b0ab41b3bd5abeae0979 Mon Sep 17 00:00:00 2001 From: Yan Zhen Date: Tue, 24 Sep 2024 19:50:05 +0800 Subject: [PATCH 4/6] HID: Fix typo in the comment Correctly spelled comments make it easier for the reader to understand the code. Fix typos: 'mninum' -> 'minimum', 'destoyed' -> 'destroyed', 'thridparty' -> 'thirdparty', 'lowcase' -> 'lowercase', 'idenitifiers' -> 'identifiers', 'exeuction' -> 'execution', 'fregments' -> 'fragments', 'devides' -> 'devices'. Signed-off-by: Yan Zhen Acked-by: Srinivas Pandruvada Signed-off-by: Jiri Kosina --- drivers/hid/hid-asus.c | 2 +- drivers/hid/hid-logitech-hidpp.c | 2 +- drivers/hid/hid-picolcd_fb.c | 2 +- drivers/hid/hid-sensor-custom.c | 2 +- drivers/hid/hid-steam.c | 2 +- drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 2 +- drivers/hid/intel-ish-hid/ishtp/client.c | 2 +- drivers/hid/usbhid/hid-core.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index a4b47319ad8e..506c6f377e7d 100644 --- a/drivers/hid/hid-asus.c +++ b/drivers/hid/hid-asus.c @@ -1183,7 +1183,7 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc, if (drvdata->quirks & QUIRK_G752_KEYBOARD && *rsize == 75 && rdesc[61] == 0x15 && rdesc[62] == 0x00) { - /* report is missing usage mninum and maximum */ + /* report is missing usage minimum and maximum */ __u8 *new_rdesc; size_t new_size = *rsize + sizeof(asus_g752_fixed_rdesc); diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 0d114268ade8..a5ae48c6f03b 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -2522,7 +2522,7 @@ static void hidpp_ff_work_handler(struct work_struct *w) /* regular effect destroyed */ data->effect_ids[wd->params[0]-1] = -1; else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER) - /* autocenter spring destoyed */ + /* autocenter spring destroyed */ data->slot_autocenter = 0; break; case HIDPP_FF_SET_GLOBAL_GAINS: diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c index 83e3409d170c..f8b16a82faef 100644 --- a/drivers/hid/hid-picolcd_fb.c +++ b/drivers/hid/hid-picolcd_fb.c @@ -296,7 +296,7 @@ static void picolcd_fb_destroy(struct fb_info *info) /* make sure no work is deferred */ fb_deferred_io_cleanup(info); - /* No thridparty should ever unregister our framebuffer! */ + /* No thirdparty should ever unregister our framebuffer! */ WARN_ON(fbdata->picolcd != NULL); vfree((u8 *)info->fix.smem_start); diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c index 66f0675df24b..617ae240396d 100644 --- a/drivers/hid/hid-sensor-custom.c +++ b/drivers/hid/hid-sensor-custom.c @@ -946,7 +946,7 @@ hid_sensor_register_platform_device(struct platform_device *pdev, memcpy(real_usage, match->luid, 4); - /* usage id are all lowcase */ + /* usage id are all lowercase */ for (c = real_usage; *c != '\0'; c++) *c = tolower(*c); diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index bf8b633114be..6439913372a8 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -253,7 +253,7 @@ enum ID_CONTROLLER_DECK_STATE = 9 }; -/* String attribute idenitifiers */ +/* String attribute identifiers */ enum { ATTRIB_STR_BOARD_SERIAL, ATTRIB_STR_UNIT_SERIAL, diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c index e157863a8b25..750bfdd26ddb 100644 --- a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c @@ -793,7 +793,7 @@ static int load_fw_from_host(struct ishtp_cl_data *client_data) if (rv < 0) goto end_err_fw_release; - /* Step 3: Start ISH main firmware exeuction */ + /* Step 3: Start ISH main firmware execution */ rv = ish_fw_start(client_data); if (rv < 0) diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c index 8a7f2f6a4f86..e61b01e9902e 100644 --- a/drivers/hid/intel-ish-hid/ishtp/client.c +++ b/drivers/hid/intel-ish-hid/ishtp/client.c @@ -863,7 +863,7 @@ static void ipc_tx_send(void *prm) /* Send ipc fragment */ ishtp_hdr.length = dev->mtu; ishtp_hdr.msg_complete = 0; - /* All fregments submitted to IPC queue with no callback */ + /* All fragments submitted to IPC queue with no callback */ ishtp_write_message(dev, &ishtp_hdr, pmsg); cl->tx_offs += dev->mtu; rem = cl_msg->send_buf.size - cl->tx_offs; diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index cb687ea7325c..956b86737b07 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1100,7 +1100,7 @@ static int usbhid_start(struct hid_device *hid) interval = endpoint->bInterval; - /* Some vendors give fullspeed interval on highspeed devides */ + /* Some vendors give fullspeed interval on highspeed devices */ if (hid->quirks & HID_QUIRK_FULLSPEED_INTERVAL && dev->speed == USB_SPEED_HIGH) { interval = fls(endpoint->bInterval*8); From b2b8a75e1d88c551a0b30d44d0be552210219eea Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Tue, 15 Oct 2024 12:23:47 -0700 Subject: [PATCH 5/6] HID: Remove default case statement in fetch_item() A default case statement with a bare unreachable() was recently added to fetch_item(), which by itself introduces undefined behavior. objtool points this out with a few different warnings, depending on configuration and compiler: vmlinux.o: warning: objtool: fetch_item() falls through to next function ... vmlinux.o: warning: objtool: hid_open_report() falls through to next function hid_parser_main() vmlinux.o: warning: objtool: hid_scan_report() falls through to next function hid_allocate_device() vmlinux.o: warning: objtool: hid_open_report+0x21b: can't find jump dest instruction at .text.hid_open_report+0x40f Replacing unreachable() with BUG() is a typical fix to eliminate the undefined behavior and make the default case well defined. However, in this case, all possible values are enumerated in the switch statement, so the default case can never actually happen, as proven with the comment next to the item->size assignment. Just remove the default case altogether, as the return statement would still be valid if the switch statement were ever to be skipped. Fixes: 61595012f280 ("HID: simplify code in fetch_item()") Suggested-by: Dmitry Torokhov Closes: https://lore.kernel.org/20241010222451.GA3571761@thelio-3990X/ Reported-by: Paul E. McKenney Closes: https://lore.kernel.org/fe8c909e-bf02-4466-b3eb-0a4747df32e3@paulmck-laptop/ Tested-by: Paul E. McKenney Signed-off-by: Nathan Chancellor Signed-off-by: Jiri Kosina --- drivers/hid/hid-core.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 87b2cdd7f0c4..bdd19bce11eb 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -802,9 +802,6 @@ static const u8 *fetch_item(const __u8 *start, const __u8 *end, struct hid_item case 4: item->data.u32 = get_unaligned_le32(start); break; - - default: - unreachable(); } return start + item->size; From 7b2daa648eb75ec75a6ebf5fce5629b758ea06df Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Wed, 16 Oct 2024 16:32:40 +0300 Subject: [PATCH 6/6] HID: debug: Remove duplicates from 'keys' Duplicates in 'keys prevents kernel builds with clang, `make W=1` and CONFIG_WERROR=y, for example: drivers/hid/hid-debug.c:3443:18: error: initializer overrides prior initialization of this subobject [-Werror,-Winitializer-overrides] 3443 | [KEY_HANGEUL] = "HanGeul", [KEY_HANGUP_PHONE] = "HangUpPhone", | ^~~~~~~~~ drivers/hid/hid-debug.c:3217:18: note: previous initialization is here 3217 | [KEY_HANGUEL] = "Hangeul", [KEY_HANJA] = "Hanja", | ^~~~~~~~~ Fix this by removing them. The logic of removal is that, remove... 1) if there is a constant that uses another defined constant, OR 2) the one that appears later in the list. Signed-off-by: Andy Shevchenko Signed-off-by: Jiri Kosina --- drivers/hid/hid-debug.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c index d5abfe652fb5..541d682af15a 100644 --- a/drivers/hid/hid-debug.c +++ b/drivers/hid/hid-debug.c @@ -3309,9 +3309,9 @@ static const char *keys[KEY_MAX + 1] = { [KEY_EPG] = "EPG", [KEY_PVR] = "PVR", [KEY_MHP] = "MHP", [KEY_LANGUAGE] = "Language", [KEY_TITLE] = "Title", [KEY_SUBTITLE] = "Subtitle", - [KEY_ANGLE] = "Angle", [KEY_ZOOM] = "Zoom", + [KEY_ANGLE] = "Angle", [KEY_MODE] = "Mode", [KEY_KEYBOARD] = "Keyboard", - [KEY_SCREEN] = "Screen", [KEY_PC] = "PC", + [KEY_PC] = "PC", [KEY_TV] = "TV", [KEY_TV2] = "TV2", [KEY_VCR] = "VCR", [KEY_VCR2] = "VCR2", [KEY_SAT] = "Sat", [KEY_SAT2] = "Sat2", @@ -3409,8 +3409,7 @@ static const char *keys[KEY_MAX + 1] = { [BTN_TRIGGER_HAPPY35] = "TriggerHappy35", [BTN_TRIGGER_HAPPY36] = "TriggerHappy36", [BTN_TRIGGER_HAPPY37] = "TriggerHappy37", [BTN_TRIGGER_HAPPY38] = "TriggerHappy38", [BTN_TRIGGER_HAPPY39] = "TriggerHappy39", [BTN_TRIGGER_HAPPY40] = "TriggerHappy40", - [BTN_DIGI] = "Digi", [BTN_STYLUS3] = "Stylus3", - [BTN_TOOL_QUINTTAP] = "ToolQuintTap", [BTN_WHEEL] = "Wheel", + [BTN_STYLUS3] = "Stylus3", [BTN_TOOL_QUINTTAP] = "ToolQuintTap", [KEY_10CHANNELSDOWN] = "10ChannelsDown", [KEY_10CHANNELSUP] = "10ChannelsUp", [KEY_3D_MODE] = "3DMode", [KEY_ADDRESSBOOK] = "Addressbook", @@ -3440,7 +3439,7 @@ static const char *keys[KEY_MAX + 1] = { [KEY_FN_RIGHT_SHIFT] = "FnRightShift", [KEY_FRAMEBACK] = "FrameBack", [KEY_FRAMEFORWARD] = "FrameForward", [KEY_FULL_SCREEN] = "FullScreen", [KEY_GAMES] = "Games", [KEY_GRAPHICSEDITOR] = "GraphicsEditor", - [KEY_HANGEUL] = "HanGeul", [KEY_HANGUP_PHONE] = "HangUpPhone", + [KEY_HANGUP_PHONE] = "HangUpPhone", [KEY_IMAGES] = "Images", [KEY_KBD_LCD_MENU1] = "KbdLcdMenu1", [KEY_KBD_LCD_MENU2] = "KbdLcdMenu2", [KEY_KBD_LCD_MENU3] = "KbdLcdMenu3", [KEY_KBD_LCD_MENU4] = "KbdLcdMenu4", [KEY_KBD_LCD_MENU5] = "KbdLcdMenu5",