Re: [PATCH] Input: Use for_each_set_bit where appropriate

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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