Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set_value

[Arnd Bergmann]

On Mon, Nov 9, 2020 at 1:34 PM Syed Nayyar Waris <syednwaris@xxxxxxxxx> wrote:
> On Sun, Nov 01, 2020 at 09:08:29PM +0100, Arnd Bergmann wrote:
> > > This avoids adding a costly conditional check inside bitmap_set_value()
> > > when almost all bitmap_set_value() calls will have static arguments with
> > > well-defined and obvious boundaries.
> > >
> > > Do you think this would be an acceptable solution to resolve your GCC
> > > warnings?
> >
> > Unfortunately, it does not seem to make a difference, as gcc still
> > knows that this compiles to the same result, and it produces the same
> > warning as before (see https://godbolt.org/z/rjx34r)
> >
> >          Arnd
>
> Hi Arnd,
>
> Sharing a different version of bitmap_set_valuei() function. See below.
>
> Let me know if the below solution looks good to you and if it resolves
> the above compiler warning.

Thanks for the follow-up!

> @@ -1,5 +1,5 @@
>  static inline void bitmap_set_value(unsigned long *map,
> -                                    unsigned long value,
> +                                    unsigned long value, const size_t length,
>                                      unsigned long start, unsigned long nbits)
>  {
>          const size_t index = BIT_WORD(start);
> @@ -7,6 +7,9 @@ static inline void bitmap_set_value(unsigned long *map,
>          const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG);
>          const unsigned long space = ceiling - start;
>
> +       if (index >= length)
> +               return;
> +
>          value &= GENMASK(nbits - 1, 0);
>
>          if (space >= nbits) {
> @@ -15,6 +18,10 @@ static inline void bitmap_set_value(unsigned long *map,
>          } else {
>                  map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
>                  map[index + 0] |= value << offset;
> +
> +               if (index + 1 >= length)
> +                       return;
> +
>                  map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
>                  map[index + 1] |= value >> space;
>          }

Yes, this does address the warning: https://godbolt.org/z/3nsGzq

Not sure what the best calling conventions would be though, as the function
now has five arguments, and the one called 'nbits' appears to be what
all other helpers in include/linux/bitmap.h use for the length of the bitmap,
while this one uses it for the length of the value.

I'd prefer passing the number of bits in the bitmap rather than the number
of 'unsigned long' words in it, and calling that 'nbits', while renaming
the current 'nbits' to something else, e.g.:

static inline void bitmap_set_value(unsigned long *map,
                                    unsigned long value, unsigned long start,
                                    unsigned long clump_size, unsigned
long nbits);

Though I'm still unsure about the argument order. Having 'nbits'
right next to 'map' would be the most logical to me as they logically
belong together, but most other linux/bitops.h helpers seem to have
'nbits' as the last argument.

        Arnd