Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231006134529.2816540-1-glider@google.com> <20231006134529.2816540-2-glider@google.com>
 <ZSAeFfNOUZIrK3Yk@smile.fi.intel.com>
In-Reply-To: <ZSAeFfNOUZIrK3Yk@smile.fi.intel.com>
From:   Alexander Potapenko <glider@google.com>
Date:   Tue, 10 Oct 2023 10:16:26 +0200
Message-ID: <CAG_fn=UxSUEcj9wWexakd-F0-Dh+WjeKEaqwMKfD8AEOnkRTsQ@mail.gmail.com>
Subject: Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     catalin.marinas@arm.com, will@kernel.org, pcc@google.com,
        andreyknvl@gmail.com, aleksander.lobakin@intel.com,
        linux@rasmusvillemoes.dk, yury.norov@gmail.com,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        eugenis@google.com, syednwaris@gmail.com, william.gray@linaro.org,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Oct 6, 2023 at 4:48=E2=80=AFPM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Oct 06, 2023 at 03:45:25PM +0200, Alexander Potapenko wrote:
> > From: Syed Nayyar Waris <syednwaris@gmail.com>
> >
> > The two new functions allow reading/writing values of length up to
> > BITS_PER_LONG bits at arbitrary position in the bitmap.
> >
> > The code was taken from "bitops: Introduce the for_each_set_clump macro=
"
> > by Syed Nayyar Waris with a number of changes and simplifications:
> >  - instead of using roundup(), which adds an unnecessary dependency
> >    on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
> >  - indentation is reduced by not using else-clauses (suggested by
> >    checkpatch for bitmap_get_value());
> >  - bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read()
> >    and bitmap_write();
> >  - some redundant computations are omitted.
>
> ...
>
> > v6:
> >  - As suggested by Yury Norov, do not require bitmap_read(..., 0) to
> >    return 0.
>
> Hmm... See below.
>
> ...
>
> >   *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to=
 u32[] dst
> >   *  bitmap_to_arr64(buf, src, nbits)            Copy nbits from buf to=
 u64[] dst
>
> With the grouping as below I would add a blank line here. But was the int=
ention
> to group _arrXX() to these groups?

Note that there's no single blank line in this long list.
What if I swap bitmap_read with bitmap_set_value8(), would the
grouping become more logical?

I.e.
*  bitmap_get_value8(map, start)               Get 8bit value from map at s=
tart
*  bitmap_set_value8(map, value, start)        Set 8bit value to map at sta=
rt
*  bitmap_read(map, start, nbits)              Read an nbits-sized value fr=
om
*                                              map at start
*  bitmap_write(map, value, start, nbits)      Write an nbits-sized value t=
o
*                                              map at start

> > +     if (unlikely(!nbits))
> > +             return 0;
>
> Hmm... I didn't get was the comment to add or to remove these checks?

As Yury said, we should not require the return value to be 0, so I
added "nonzero" to the descriptions of the @nbits parameter.
The check stays in place, but the users relying on it is now a mistake.

>
> > +     if (space >=3D nbits)
> > +             return (map[index] >> offset) & GENMASK(nbits - 1, 0);
>
> And don't you want to replace this GENMASK() as well?

See my next reply to Yury, tl;dr this is a stale code version :(