Re: [PATCH] HID: simplify code in fetch_item()
From: Nathan Chancellor <nathan@kernel.org>
Date: 2024-10-10 22:24:53
Also in:
lkml, llvm
Hi Dmitry, On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
quoted hunk ↗ jump to hunk
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 <dmitry.torokhov@gmail.com> --- 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 988d0acbdf04..00942d40fe08 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c@@ -754,35 +754,32 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *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; }
I am noticing some interesting behavior when building with clang, namely
some objtool warnings and a failed boot when LTO is enabled, which I
bisected to this change as commit 61595012f280 ("HID: simplify code in
fetch_item()"), such as:
$ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig vmlinux
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()
With LTO enabled, the warning becomes:
vmlinux.o: warning: objtool: hid_open_report+0x21b: can't find jump dest instruction at .text.hid_open_report+0x40f
A bare unreachable(), especially in the default case of a switch
statement, is generally considered harmful in my experience, as it can
introduce undefined behavior, which can mess up how a compiler might
optimize a function. Commit d652d5f1eeeb ("drm/edid: fix objtool warning
in drm_cvt_modes()") and commit 3764647b255a ("bcachefs: Remove
undefined behavior in bch2_dev_buckets_reserved()") have some good
commit messages talking about it.
Getting rid of the unreachable() in some way resolves the issue. I
tested using BUG() in lieu of unreachable() like the second change I
mentioned above, which resolves the issue cleanly, as the default case
clearly cannot happen. Another option I tested was some sort of printk
statement and returning NULL, which some maintainers prefer, even in
spite of impossible conditions. I am happy to send a patch with one of
those changes or open to other suggestions.
Cheers,
Nathan