1. Patchew
  2. linux
  3. View series

[PATCH] HID: simplify code in fetch_item()

Dmitry Torokhov posted 1 patch 1 month, 3 weeks ago 
drivers/hid/hid-core.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
[PATCH] HID: simplify code in fetch_item()
Posted by Dmitry Torokhov 1 month, 3 weeks ago 
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;
 }
 
 static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
-- 
2.46.1.824.gd892dcdcdd-goog


-- 
Dmitry
Re: [PATCH] HID: simplify code in fetch_item()
Posted by Nathan Chancellor 1 month, 2 weeks ago 
Hi Dmitry,

On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> 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
Re: [PATCH] HID: simplify code in fetch_item()
Posted by Dmitry Torokhov 1 month, 1 week ago 
Hi Nathan,

On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> Hi Dmitry,
> 
> On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> > 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.

Oh well, if our toolchain does not like "unreachable()" then we can
simply remove it - the switch does cover all possible values and the
"return" statement should be valid even if compiler somehow decides that
"switch" statement can be skipped.

If you can send a patch that would be great.

I'm adding Paul and a few others to CC who apparently seeing the same
issue.

Thanks.

-- 
Dmitry
Re: [PATCH] HID: simplify code in fetch_item()
Posted by Nathan Chancellor 1 month, 1 week ago 
On Tue, Oct 15, 2024 at 11:28:26AM -0700, Dmitry Torokhov wrote:
> Oh well, if our toolchain does not like "unreachable()" then we can
> simply remove it - the switch does cover all possible values and the
> "return" statement should be valid even if compiler somehow decides that
> "switch" statement can be skipped.
> 
> If you can send a patch that would be great.

Done, thanks a lot for the input!

https://lore.kernel.org/20241015-hid-fix-fetch_item-unreachable-v1-1-b131cd10dbd1@kernel.org/

Cheers,
Nathan
Re: [PATCH] HID: simplify code in fetch_item()
Posted by Segher Boessenkool 1 month, 1 week ago 
On Tue, Oct 15, 2024 at 12:26:04PM -0700, Nathan Chancellor wrote:
> On Tue, Oct 15, 2024 at 11:28:26AM -0700, Dmitry Torokhov wrote:
> > Oh well, if our toolchain does not like "unreachable()" then we can
> > simply remove it - the switch does cover all possible values and the
> > "return" statement should be valid even if compiler somehow decides that
> > "switch" statement can be skipped.
> > 
> > If you can send a patch that would be great.
> 
> Done, thanks a lot for the input!
> 
> https://lore.kernel.org/20241015-hid-fix-fetch_item-unreachable-v1-1-b131cd10dbd1@kernel.org/

There also is -funreachable-traps, which the kernel might want to use
(with that option every builtin_unreachablei() is compiled to a trap
instruction, instead of that the compiler just thinks "Aha!  This can
never happen!", and optimise based on that).


Segher
Re: [PATCH] HID: simplify code in fetch_item()
Posted by Paul E. McKenney 1 month, 1 week ago 
On Tue, Oct 15, 2024 at 11:28:26AM -0700, Dmitry Torokhov wrote:
> Hi Nathan,
> 
> On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> > Hi Dmitry,
> > 
> > On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> > > 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.
> 
> Oh well, if our toolchain does not like "unreachable()" then we can
> simply remove it - the switch does cover all possible values and the
> "return" statement should be valid even if compiler somehow decides that
> "switch" statement can be skipped.
> 
> If you can send a patch that would be great.
> 
> I'm adding Paul and a few others to CC who apparently seeing the same
> issue.

Commenting out the unreachable() fixes things for me, as does
replacing the unreachable() with BUG().  So, for either solution:

Tested-by: Paul E. McKenney <paulmck@kernel.org>

							Thanx, Paul
Re: [PATCH] HID: simplify code in fetch_item()
Posted by Benjamin Tissoires 1 month, 3 weeks ago 
On Tue, 01 Oct 2024 08:42:36 -0700, Dmitry Torokhov wrote:
> 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.
> 
> 

Applied to hid/hid.git (for-6.13/core), thanks!

[1/1] HID: simplify code in fetch_item()
      https://git.kernel.org/hid/hid/c/61595012f280

Cheers,
-- 
Benjamin Tissoires <bentiss@kernel.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.