[PATCH] Input: Use for_each_set_bit where appropriate

[PATCH] Input: Use for_each_set_bit where appropriate

[Anshul Garg]

Use for_each_set_bit to check for set bits in bitmap
as it is more efficient than checking individual bits.

Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
---
 drivers/input/ff-core.c |    5 ++---
 drivers/input/joydev.c  |   11 +++++------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 8f4a30fc..c642082 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -343,9 +343,8 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
 	__set_bit(EV_FF, dev->evbit);
 
 	/* Copy "true" bits into ff device bitmap */
-	for (i = 0; i <= FF_MAX; i++)
-		if (test_bit(i, dev->ffbit))
-			__set_bit(i, ff->ffbit);
+	for_each_set_bit(i, dev->ffbit, FF_CNT)
+		__set_bit(i, ff->ffbit);
 
 	/* we can emulate RUMBLE with periodic effects */
 	if (test_bit(FF_PERIODIC, ff->ffbit))
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index f362883..4686260 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -798,12 +798,11 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 	joydev->handle.handler = handler;
 	joydev->handle.private = joydev;
 
-	for (i = 0; i < ABS_CNT; i++)
-		if (test_bit(i, dev->absbit)) {
-			joydev->absmap[i] = joydev->nabs;
-			joydev->abspam[joydev->nabs] = i;
-			joydev->nabs++;
-		}
+	for_each_set_bit(i, dev->absbit, ABS_CNT) {
+		joydev->absmap[i] = joydev->nabs;
+		joydev->abspam[joydev->nabs] = i;
+		joydev->nabs++;
+	}
 
 	for (i = BTN_JOYSTICK - BTN_MISC; i < KEY_MAX - BTN_MISC + 1; i++)
 		if (test_bit(i + BTN_MISC, dev->keybit)) {
-- 
1.7.9.5


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Stephen Chandler Paul]

Hi! The currently upstream version of this patch actually breaks
uinput, and causes the kernel to panic when attempting to run it under
qemu using spice. Here's a backtrace from kdb:

Stack traceback for pid 656
0xffff8800babed480      656        1  1    2   R  0xffff8800babefa80 *spice-vdagentd
 ffff88013747bd58 0000000000000018 ffff88013747bd80 ffff8800b7977000
 0000000000000003 0000000000000001 0000000000000001 ffff8800b7977240
 ffff88013747bdc0 ffffffff8163f449 0000000000000286 0000000000000018
Call Trace:
 [<ffffffff8163f449>] ? input_event+0x59/0x80
 [<ffffffffa0509234>] ? uinput_write+0x154/0x460 [uinput]
 [<ffffffffa00e704d>] ? port_fops_read+0xfd/0x1f0 [virtio_console]
 [<ffffffff81261627>] ? __vfs_write+0x37/0x100
 [<ffffffff81261ff9>] ? vfs_write+0xa9/0x1a0
 [<ffffffff81283386>] ? __fget_light+0x66/0x90
 [<ffffffff81262cf8>] ? SyS_write+0x58/0xd0
 [<ffffffff81833c72>] ? entry_SYSCALL_64_fastpath+0x12/0x76

And the relevant messages from dmesg:

<1>[   15.064330] BUG: unable to handle kernel NULL pointer dereference at 0000000000000024
<1>[   15.064336] IP: [<ffffffff8163f142>] input_handle_event+0x232/0x4e0
<4>[   15.064343] PGD 0 
<4>[   15.064345] Oops: 0000 [#1] SMP

The steps for reproducing this are pretty simple: setup a Fedora 22 VM,
build the latest kernel and install it with make install, and try to
boot the machine and use it over spice with qemu. After moving the
cursor it'll run into a NULL dereference and panic.

I've tested reverting this commit, and that fixes the NULL dereference
completely. I'm willing to git send-email you the revert if wish.

Cheers,
	Lyude

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Dmitry Torokhov]

On Thu, Sep 17, 2015 at 04:02:47PM -0400, Stephen Chandler Paul wrote:
> Hi! The currently upstream version of this patch actually breaks
> uinput, and causes the kernel to panic when attempting to run it under
> qemu using spice. Here's a backtrace from kdb:
> 
> Stack traceback for pid 656
> 0xffff8800babed480      656        1  1    2   R  0xffff8800babefa80 *spice-vdagentd
>  ffff88013747bd58 0000000000000018 ffff88013747bd80 ffff8800b7977000
>  0000000000000003 0000000000000001 0000000000000001 ffff8800b7977240
>  ffff88013747bdc0 ffffffff8163f449 0000000000000286 0000000000000018
> Call Trace:
>  [<ffffffff8163f449>] ? input_event+0x59/0x80
>  [<ffffffffa0509234>] ? uinput_write+0x154/0x460 [uinput]
>  [<ffffffffa00e704d>] ? port_fops_read+0xfd/0x1f0 [virtio_console]
>  [<ffffffff81261627>] ? __vfs_write+0x37/0x100
>  [<ffffffff81261ff9>] ? vfs_write+0xa9/0x1a0
>  [<ffffffff81283386>] ? __fget_light+0x66/0x90
>  [<ffffffff81262cf8>] ? SyS_write+0x58/0xd0
>  [<ffffffff81833c72>] ? entry_SYSCALL_64_fastpath+0x12/0x76
> 
> And the relevant messages from dmesg:
> 
> <1>[   15.064330] BUG: unable to handle kernel NULL pointer dereference at 0000000000000024
> <1>[   15.064336] IP: [<ffffffff8163f142>] input_handle_event+0x232/0x4e0
> <4>[   15.064343] PGD 0 
> <4>[   15.064345] Oops: 0000 [#1] SMP
> 
> The steps for reproducing this are pretty simple: setup a Fedora 22 VM,
> build the latest kernel and install it with make install, and try to
> boot the machine and use it over spice with qemu. After moving the
> cursor it'll run into a NULL dereference and panic.
> 
> I've tested reverting this commit, and that fixes the NULL dereference
> completely. I'm willing to git send-email you the revert if wish.

*sigh* Sorry about that, the 2nd chunk of the change was completely
bogus.

Does the patch below fixes this for you?

Thanks.

-- 
Dmitry

Input: uinput - fix crash when using ABS events

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Commit b6d30968d86c45a7bb599eaca13ff048d3fa576c (Input: uinput - switch to
using for_each_set_bit()) switched driver to use for_each_set_bit().
However during initial write of the uinput structure that contains min/max
data for all possible axes none of them are reflected in dev->absbit yet
and so we were skipping over all of them and were not allocating absinfo
memory which caused crash later when driver tried to sens EV_ABS events:

<1>[   15.064330] BUG: unable to handle kernel NULL pointer dereference at 0000000000000024
<1>[   15.064336] IP: [<ffffffff8163f142>] input_handle_event+0x232/0x4e0
<4>[   15.064343] PGD 0
<4>[   15.064345] Oops: 0000 [#1] SMP

Fixes: b6d30968d86c45a7bb599eaca13ff048d3fa576c
Cc: stable@vger.kernel.org
Reported-by: Stephen Chandler Paul <cpaul@redhat.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/uinput.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 345df9b..5adbced 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -414,7 +414,7 @@ static int uinput_setup_device(struct uinput_device *udev,
 	dev->id.product	= user_dev->id.product;
 	dev->id.version	= user_dev->id.version;
 
-	for_each_set_bit(i, dev->absbit, ABS_CNT) {
+	for (i = 0; i < ABS_CNT; i++) {
 		input_abs_set_max(dev, i, user_dev->absmax[i]);
 		input_abs_set_min(dev, i, user_dev->absmin[i]);
 		input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Stephen Chandler Paul]

On Sat, 2015-09-19 at 11:26 -0700, Dmitry Torokhov wrote:
> On Thu, Sep 17, 2015 at 04:02:47PM -0400, Stephen Chandler Paul
> wrote:
> > Hi! The currently upstream version of this patch actually breaks
> > uinput, and causes the kernel to panic when attempting to run it
> > under
> > qemu using spice. Here's a backtrace from kdb:
> > 
> > Stack traceback for pid 656
> > 0xffff8800babed480      656        1  1    2   R 
> >  0xffff8800babefa80 *spice-vdagentd
> >  ffff88013747bd58 0000000000000018 ffff88013747bd80
> > ffff8800b7977000
> >  0000000000000003 0000000000000001 0000000000000001
> > ffff8800b7977240
> >  ffff88013747bdc0 ffffffff8163f449 0000000000000286
> > 0000000000000018
> > Call Trace:
> >  [<ffffffff8163f449>] ? input_event+0x59/0x80
> >  [<ffffffffa0509234>] ? uinput_write+0x154/0x460 [uinput]
> >  [<ffffffffa00e704d>] ? port_fops_read+0xfd/0x1f0 [virtio_console]
> >  [<ffffffff81261627>] ? __vfs_write+0x37/0x100
> >  [<ffffffff81261ff9>] ? vfs_write+0xa9/0x1a0
> >  [<ffffffff81283386>] ? __fget_light+0x66/0x90
> >  [<ffffffff81262cf8>] ? SyS_write+0x58/0xd0
> >  [<ffffffff81833c72>] ? entry_SYSCALL_64_fastpath+0x12/0x76
> > 
> > And the relevant messages from dmesg:
> > 
> > <1>[   15.064330] BUG: unable to handle kernel NULL pointer
> > dereference at 0000000000000024
> > <1>[   15.064336] IP: [<ffffffff8163f142>]
> > input_handle_event+0x232/0x4e0
> > <4>[   15.064343] PGD 0 
> > <4>[   15.064345] Oops: 0000 [#1] SMP
> > 
> > The steps for reproducing this are pretty simple: setup a Fedora 22
> > VM,
> > build the latest kernel and install it with make install, and try
> > to
> > boot the machine and use it over spice with qemu. After moving the
> > cursor it'll run into a NULL dereference and panic.
> > 
> > I've tested reverting this commit, and that fixes the NULL
> > dereference
> > completely. I'm willing to git send-email you the revert if wish.
> 
> *sigh* Sorry about that, the 2nd chunk of the change was completely
> bogus.
> 
> Does the patch below fixes this for you?
Yep! Applied it to my local tree, and the virtual machine appears to
have no issues now.

Cheers,
	Lyude

> 
> Thanks.
> 

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Dmitry Torokhov]

On Mon, Sep 21, 2015 at 10:30:51AM -0400, Stephen Chandler Paul wrote:
> On Sat, 2015-09-19 at 11:26 -0700, Dmitry Torokhov wrote:
> > On Thu, Sep 17, 2015 at 04:02:47PM -0400, Stephen Chandler Paul
> > wrote:
> > > Hi! The currently upstream version of this patch actually breaks
> > > uinput, and causes the kernel to panic when attempting to run it
> > > under
> > > qemu using spice. Here's a backtrace from kdb:
> > > 
> > > Stack traceback for pid 656
> > > 0xffff8800babed480      656        1  1    2   R 
> > >  0xffff8800babefa80 *spice-vdagentd
> > >  ffff88013747bd58 0000000000000018 ffff88013747bd80
> > > ffff8800b7977000
> > >  0000000000000003 0000000000000001 0000000000000001
> > > ffff8800b7977240
> > >  ffff88013747bdc0 ffffffff8163f449 0000000000000286
> > > 0000000000000018
> > > Call Trace:
> > >  [<ffffffff8163f449>] ? input_event+0x59/0x80
> > >  [<ffffffffa0509234>] ? uinput_write+0x154/0x460 [uinput]
> > >  [<ffffffffa00e704d>] ? port_fops_read+0xfd/0x1f0 [virtio_console]
> > >  [<ffffffff81261627>] ? __vfs_write+0x37/0x100
> > >  [<ffffffff81261ff9>] ? vfs_write+0xa9/0x1a0
> > >  [<ffffffff81283386>] ? __fget_light+0x66/0x90
> > >  [<ffffffff81262cf8>] ? SyS_write+0x58/0xd0
> > >  [<ffffffff81833c72>] ? entry_SYSCALL_64_fastpath+0x12/0x76
> > > 
> > > And the relevant messages from dmesg:
> > > 
> > > <1>[   15.064330] BUG: unable to handle kernel NULL pointer
> > > dereference at 0000000000000024
> > > <1>[   15.064336] IP: [<ffffffff8163f142>]
> > > input_handle_event+0x232/0x4e0
> > > <4>[   15.064343] PGD 0 
> > > <4>[   15.064345] Oops: 0000 [#1] SMP
> > > 
> > > The steps for reproducing this are pretty simple: setup a Fedora 22
> > > VM,
> > > build the latest kernel and install it with make install, and try
> > > to
> > > boot the machine and use it over spice with qemu. After moving the
> > > cursor it'll run into a NULL dereference and panic.
> > > 
> > > I've tested reverting this commit, and that fixes the NULL
> > > dereference
> > > completely. I'm willing to git send-email you the revert if wish.
> > 
> > *sigh* Sorry about that, the 2nd chunk of the change was completely
> > bogus.
> > 
> > Does the patch below fixes this for you?
> Yep! Applied it to my local tree, and the virtual machine appears to
> have no issues now.

Thanks Lyude!

-- 
Dmitry

[PATCH] Input: Use for_each_set_bit where appropriate

[Anshul Garg]

Use for_each_set_bit to check for set bits in bitmap
as it is more efficient and compact.
Also use bitwise and instead of expensive %
operation while fetching next event.

Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
---
 drivers/input/misc/uinput.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 421e29e..a3c15ad 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -319,11 +319,8 @@ static int uinput_validate_absbits(struct input_dev *dev)
 	/*
 	 * Check if absmin/absmax/absfuzz/absflat are sane.
 	 */
-
-	for (cnt = 0; cnt < ABS_CNT; cnt++) {
+	for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
 		int min, max;
-		if (!test_bit(cnt, dev->absbit))
-			continue;
 
 		min = input_abs_get_min(dev, cnt);
 		max = input_abs_get_max(dev, cnt);
@@ -415,8 +412,8 @@ static int uinput_setup_device(struct uinput_device *udev,
 	dev->id.vendor	= user_dev->id.vendor;
 	dev->id.product	= user_dev->id.product;
 	dev->id.version	= user_dev->id.version;
-
-	for (i = 0; i < ABS_CNT; i++) {
+
+	for_each_set_bit(i, dev->absbit, ABS_CNT) {
 		input_abs_set_max(dev, i, user_dev->absmax[i]);
 		input_abs_set_min(dev, i, user_dev->absmin[i]);
 		input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
@@ -493,7 +490,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev,
 	have_event = udev->head != udev->tail;
 	if (have_event) {
 		*event = udev->buff[udev->tail];
-		udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
+		udev->tail &= UINPUT_BUFFER_SIZE - 1;
 	}
 
 	spin_unlock_irq(&udev->dev->event_lock);
-- 
1.7.9.5


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Dmitry Torokhov]

On Thu, Jul 09, 2015 at 06:41:01AM -0700, Anshul Garg wrote:
> Use for_each_set_bit to check for set bits in bitmap
> as it is more efficient and compact.
> Also use bitwise and instead of expensive %
> operation while fetching next event.

It is not expensive if we are using a constant that is carefully
selected (and it is).

> 
> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
> ---
>  drivers/input/misc/uinput.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 421e29e..a3c15ad 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -319,11 +319,8 @@ static int uinput_validate_absbits(struct input_dev *dev)
>  	/*
>  	 * Check if absmin/absmax/absfuzz/absflat are sane.
>  	 */
> -
> -	for (cnt = 0; cnt < ABS_CNT; cnt++) {
> +	for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
>  		int min, max;
> -		if (!test_bit(cnt, dev->absbit))
> -			continue;
>  
>  		min = input_abs_get_min(dev, cnt);
>  		max = input_abs_get_max(dev, cnt);
> @@ -415,8 +412,8 @@ static int uinput_setup_device(struct uinput_device *udev,
>  	dev->id.vendor	= user_dev->id.vendor;
>  	dev->id.product	= user_dev->id.product;
>  	dev->id.version	= user_dev->id.version;
> -
> -	for (i = 0; i < ABS_CNT; i++) {
> +
> +	for_each_set_bit(i, dev->absbit, ABS_CNT) {
>  		input_abs_set_max(dev, i, user_dev->absmax[i]);
>  		input_abs_set_min(dev, i, user_dev->absmin[i]);
>  		input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
> @@ -493,7 +490,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev,
>  	have_event = udev->head != udev->tail;
>  	if (have_event) {
>  		*event = udev->buff[udev->tail];
> -		udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
> +		udev->tail &= UINPUT_BUFFER_SIZE - 1;

What is this exactly? And how did you test it?

This chunk dropped form the patch.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Anshul Garg]

Hello Mr. Dmitry ,



On Thu, Jul 9, 2015 at 10:56 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Jul 09, 2015 at 06:41:01AM -0700, Anshul Garg wrote:
>> Use for_each_set_bit to check for set bits in bitmap
>> as it is more efficient and compact.
>> Also use bitwise and instead of expensive %
>> operation while fetching next event.
>
> It is not expensive if we are using a constant that is carefully
> selected (and it is).
>
Ok i understood now.
>>
>> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
>> ---
>>  drivers/input/misc/uinput.c |   11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index 421e29e..a3c15ad 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -319,11 +319,8 @@ static int uinput_validate_absbits(struct input_dev *dev)
>>       /*
>>        * Check if absmin/absmax/absfuzz/absflat are sane.
>>        */
>> -
>> -     for (cnt = 0; cnt < ABS_CNT; cnt++) {
>> +     for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
>>               int min, max;
>> -             if (!test_bit(cnt, dev->absbit))
>> -                     continue;
>>
>>               min = input_abs_get_min(dev, cnt);
>>               max = input_abs_get_max(dev, cnt);
>> @@ -415,8 +412,8 @@ static int uinput_setup_device(struct uinput_device *udev,
>>       dev->id.vendor  = user_dev->id.vendor;
>>       dev->id.product = user_dev->id.product;
>>       dev->id.version = user_dev->id.version;
>> -
>> -     for (i = 0; i < ABS_CNT; i++) {
>> +
>> +     for_each_set_bit(i, dev->absbit, ABS_CNT) {
>>               input_abs_set_max(dev, i, user_dev->absmax[i]);
>>               input_abs_set_min(dev, i, user_dev->absmin[i]);
>>               input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
>> @@ -493,7 +490,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev,
>>       have_event = udev->head != udev->tail;
>>       if (have_event) {
>>               *event = udev->buff[udev->tail];
>> -             udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
>> +             udev->tail &= UINPUT_BUFFER_SIZE - 1;
>
> What is this exactly? And how did you test it?
>
> This chunk dropped form the patch.
>
Rest changes are using  for_each_set_bit instead of for loop which
checks every bit in absbit bitmap.

In uinput_validate_absbits api
for (cnt = 0; cnt < ABS_CNT; cnt++) {  -> for_each_set_bit(cnt,
dev->absbit, ABS_CNT) {
ans removing test_bit code inside loop.

In uinput_setup_device api

for (i = 0; i < ABS_CNT; i++) { - >for_each_set_bit(i, dev->absbit, ABS_CNT) {

Thanks
Anshul Garg


> Thanks.
>
> --
> Dmitry

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Dmitry Torokhov]

On Thu, Jul 09, 2015 at 11:05:57PM +0530, Anshul Garg wrote:
> Hello Mr. Dmitry ,
> 
> 
> 
> On Thu, Jul 9, 2015 at 10:56 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Jul 09, 2015 at 06:41:01AM -0700, Anshul Garg wrote:
> >> Use for_each_set_bit to check for set bits in bitmap
> >> as it is more efficient and compact.
> >> Also use bitwise and instead of expensive %
> >> operation while fetching next event.
> >
> > It is not expensive if we are using a constant that is carefully
> > selected (and it is).
> >
> Ok i understood now.
> >>
> >> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
> >> ---
> >>  drivers/input/misc/uinput.c |   11 ++++-------
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> >> index 421e29e..a3c15ad 100644
> >> --- a/drivers/input/misc/uinput.c
> >> +++ b/drivers/input/misc/uinput.c
> >> @@ -319,11 +319,8 @@ static int uinput_validate_absbits(struct input_dev *dev)
> >>       /*
> >>        * Check if absmin/absmax/absfuzz/absflat are sane.
> >>        */
> >> -
> >> -     for (cnt = 0; cnt < ABS_CNT; cnt++) {
> >> +     for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
> >>               int min, max;
> >> -             if (!test_bit(cnt, dev->absbit))
> >> -                     continue;
> >>
> >>               min = input_abs_get_min(dev, cnt);
> >>               max = input_abs_get_max(dev, cnt);
> >> @@ -415,8 +412,8 @@ static int uinput_setup_device(struct uinput_device *udev,
> >>       dev->id.vendor  = user_dev->id.vendor;
> >>       dev->id.product = user_dev->id.product;
> >>       dev->id.version = user_dev->id.version;
> >> -
> >> -     for (i = 0; i < ABS_CNT; i++) {
> >> +
> >> +     for_each_set_bit(i, dev->absbit, ABS_CNT) {
> >>               input_abs_set_max(dev, i, user_dev->absmax[i]);
> >>               input_abs_set_min(dev, i, user_dev->absmin[i]);
> >>               input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
> >> @@ -493,7 +490,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev,
> >>       have_event = udev->head != udev->tail;
> >>       if (have_event) {
> >>               *event = udev->buff[udev->tail];
> >> -             udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
> >> +             udev->tail &= UINPUT_BUFFER_SIZE - 1;
> >
> > What is this exactly? And how did you test it?
> >
> > This chunk dropped form the patch.
> >
> Rest changes are using  for_each_set_bit instead of for loop which
> checks every bit in absbit bitmap.

I understand what the rest of the changes are doing. My comment was
regarding the very last chunk and I wondered how exactly you tested you
changes because it is obviously broken as it no longer advances the
tail.

The proper way (if you wanted to switch to bitwise and) would be to do:

	udev->tail = (udev->tail + 1) & (UINPUT_BUFFER_SIZE - 1);

Thanks.

-- 
Dmitry

Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Anshul Garg]

Hello Mr. Dmitry ,

udev->tail change i have not tested.
i have suggested this change for sole optimization purpose.

Yes this "udev->tail = (udev->tail + 1) & (UINPUT_BUFFER_SIZE - 1);"
should be code.


On Thu, Jul 9, 2015 at 11:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Jul 09, 2015 at 11:05:57PM +0530, Anshul Garg wrote:
>> Hello Mr. Dmitry ,
>>
>>
>>
>> On Thu, Jul 9, 2015 at 10:56 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Thu, Jul 09, 2015 at 06:41:01AM -0700, Anshul Garg wrote:
>> >> Use for_each_set_bit to check for set bits in bitmap
>> >> as it is more efficient and compact.
>> >> Also use bitwise and instead of expensive %
>> >> operation while fetching next event.
>> >
>> > It is not expensive if we are using a constant that is carefully
>> > selected (and it is).
>> >
>> Ok i understood now.
>> >>
>> >> Signed-off-by: Anshul Garg <aksgarg1989@gmail.com>
>> >> ---
>> >>  drivers/input/misc/uinput.c |   11 ++++-------
>> >>  1 file changed, 4 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> >> index 421e29e..a3c15ad 100644
>> >> --- a/drivers/input/misc/uinput.c
>> >> +++ b/drivers/input/misc/uinput.c
>> >> @@ -319,11 +319,8 @@ static int uinput_validate_absbits(struct input_dev *dev)
>> >>       /*
>> >>        * Check if absmin/absmax/absfuzz/absflat are sane.
>> >>        */
>> >> -
>> >> -     for (cnt = 0; cnt < ABS_CNT; cnt++) {
>> >> +     for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
>> >>               int min, max;
>> >> -             if (!test_bit(cnt, dev->absbit))
>> >> -                     continue;
>> >>
>> >>               min = input_abs_get_min(dev, cnt);
>> >>               max = input_abs_get_max(dev, cnt);
>> >> @@ -415,8 +412,8 @@ static int uinput_setup_device(struct uinput_device *udev,
>> >>       dev->id.vendor  = user_dev->id.vendor;
>> >>       dev->id.product = user_dev->id.product;
>> >>       dev->id.version = user_dev->id.version;
>> >> -
>> >> -     for (i = 0; i < ABS_CNT; i++) {
>> >> +
>> >> +     for_each_set_bit(i, dev->absbit, ABS_CNT) {
>> >>               input_abs_set_max(dev, i, user_dev->absmax[i]);
>> >>               input_abs_set_min(dev, i, user_dev->absmin[i]);
>> >>               input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
>> >> @@ -493,7 +490,7 @@ static bool uinput_fetch_next_event(struct uinput_device *udev,
>> >>       have_event = udev->head != udev->tail;
>> >>       if (have_event) {
>> >>               *event = udev->buff[udev->tail];
>> >> -             udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
>> >> +             udev->tail &= UINPUT_BUFFER_SIZE - 1;
>> >
>> > What is this exactly? And how did you test it?
>> >
>> > This chunk dropped form the patch.
>> >
>> Rest changes are using  for_each_set_bit instead of for loop which
>> checks every bit in absbit bitmap.
>
> I understand what the rest of the changes are doing. My comment was
> regarding the very last chunk and I wondered how exactly you tested you
> changes because it is obviously broken as it no longer advances the
> tail.
>
> The proper way (if you wanted to switch to bitwise and) would be to do:
>
>         udev->tail = (udev->tail + 1) & (UINPUT_BUFFER_SIZE - 1);
>
> Thanks.
>
> --
> Dmitry