linux-kernel - Re: [PATCH v2 5/8] mtd: maps: gpio-addr-flash: Replace array with an integer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 1 Oct 2018 14:10:03 +0200
From:   Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To:     Boris Brezillon <boris.brezillon@...tlin.com>
Cc:     David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Zhouyang Jia <jiazhouyang09@...il.com>,
        linux-mtd@...ts.infradead.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 5/8] mtd: maps: gpio-addr-flash: Replace array with an integer

Hi Boris

On Thu, Sep 27, 2018 at 1:42 PM Boris Brezillon
<boris.brezillon@...tlin.com> wrote:
>
> On Wed,  5 Sep 2018 16:36:40 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@...il.com> wrote:
>
> > By replacing the array with an integer we can avoid completely
> > the bit comparison loop if the value has not changed (by far
> > the most common case).
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
> > ---
> >  drivers/mtd/maps/gpio-addr-flash.c | 31 +++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> > index 22e100f07112..8f5e3dce9be3 100644
> > --- a/drivers/mtd/maps/gpio-addr-flash.c
> > +++ b/drivers/mtd/maps/gpio-addr-flash.c
> > @@ -43,7 +43,7 @@ struct async_state {
> >       struct map_info map;
> >       size_t gpio_count;
> >       unsigned *gpio_addrs;
> > -     int *gpio_values;
> > +     unsigned int gpio_values;
> >       unsigned int win_order;
> >  };
> >  #define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
> > @@ -55,22 +55,25 @@ struct async_state {
> >   *
> >   * Rather than call the GPIO framework every time, cache the last-programmed
> >   * value.  This speeds up sequential accesses (which are by far the most common
> > - * type).  We rely on the GPIO framework to treat non-zero value as high so
> > - * that we don't have to normalize the bits.
> > + * type).
> >   */
> >  static void gf_set_gpios(struct async_state *state, unsigned long ofs)
> >  {
> > -     size_t i = 0;
> > -     int value;
> > +     int i;
> >
> >       ofs >>= state->win_order;
> > -     do {
> > -             value = ofs & (1 << i);
> > -             if (state->gpio_values[i] != value) {
> > -                     gpio_set_value(state->gpio_addrs[i], value);
> > -                     state->gpio_values[i] = value;
> > -             }
> > -     } while (++i < state->gpio_count);
> > +
> > +     if (ofs == state->gpio_values)
> > +             return;
> > +
> > +     for (i = 0; i < state->gpio_count; i++) {
> > +             if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))
>
> Parens around the xx & BIT(i) operations are unneeded.

If I remove it:


ricardo@...pili:~/curro/kernel-upstream$ make drivers/mtd/maps/gpio-addr-flash.o
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CC      drivers/mtd/maps/gpio-addr-flash.o
drivers/mtd/maps/gpio-addr-flash.c: In function ‘gf_set_gpios’:
drivers/mtd/maps/gpio-addr-flash.c:70:20: warning: suggest parentheses
around comparison in operand of ‘&’ [-Wparentheses]
   if (ofs & BIT(i) == (state->gpio_values & BIT(i)))


>
> > +                     continue;
> > +
> > +             gpio_set_value(state->gpio_addrs[i], !!(ofs & BIT(i)));
> > +     }
> > +
> > +     state->gpio_values = ofs;
> >  }
> >
> >  /**
> > @@ -215,7 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
> >       if (!memory || !gpios || !gpios->end)
> >               return -EINVAL;
> >
> > -     arr_size = sizeof(int) * gpios->end;
> > +     arr_size = sizeof(state->gpio_addrs[0]) * gpios->end;
> >       state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
> >       if (!state)
> >               return -ENOMEM;
> > @@ -226,9 +229,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
> >        */
> >       state->gpio_count     = gpios->end;
> >       state->gpio_addrs     = (void *)(unsigned long)gpios->start;
> > -     state->gpio_values    = (void *)(state + 1);
> >       state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
> > -     memset(state->gpio_values, 0xff, arr_size);
> >
> >       state->map.name       = DRIVER_NAME;
> >       state->map.read       = gf_read;
>


-- 
Ricardo Ribalda