Re: [PATCH] drm/komeda: use bitmap API to convert U32 to bitmap

[Yury Norov]

On Tue, Dec 29, 2020 at 10:09 AM Yury Norov <yury.norov@xxxxxxxxx> wrote:
>
> On Tue, Dec 29, 2020 at 5:50 AM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> >
> > On Tue, Dec 29, 2020 at 2:24 PM Carsten Haitzler
> > <Carsten.Haitzler@xxxxxxx> wrote:
> > >
> > > On 12/28/20 8:10 PM, Yury Norov wrote:
> > > > On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
> > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > >> On Mon, Dec 28, 2020 at 11:43:43AM -0800, Yury Norov wrote:
> > > >>> The commit be3e477effba636ad25 ("drm/komeda: Fix bit
> > > >>> check to import to value of proper type") fixes possible
> > > >>> out-of-bound issue related to find_first_bit() usage, but
> > > >>> does not address the endianness problem.
> > > >> Hmm... Can you elaborate?
> > > >>
> > > >> ...
> > > >>
> > > >>>                                    u32 comp_mask)
> > > >>> -     unsigned long comp_mask_local = (unsigned long)comp_mask;
> > > >> Here we convert u32 to unsigned long (LSB is kept LSB since it happens in
> > > >> native endianess).
> > > >>
> > > >>> -     id = find_first_bit(&comp_mask_local, 32);
> > > >> Here it takes an address to unsigned long and tries only lower 32 bits.
> > > >>
> > > >> Are you telling that find_first_bit() has an issue?
> > > > It seems you're right, there's no issue with endianness in existing code.
> > > > In fact, the line
> > >
> > > Indeed Andy covered this. Take LSB32 with the cast to "local on-stack
> > > long" and possible extend upper 32bits with 0's if needed (64bit longs).
> > >
> > > >>> -     unsigned long comp_mask_local = (unsigned long)comp_mask;
> > > > is an opencoded version of bitmap_from_arr32(dst, src, 32).
> > > >
> > > > Maybe it would be better to use the bitmap API here, but existing code is
> > > > correct. Sorry for the noise.
> > > While your code is seemingly also valid (I can check to be sure with
> > > KASAN if you want still), it does seem a little less "nice to read" with
> > > more lines of code for the same work. Is it worth making the code a
> > > little longer here as it's not actually fixing anything to do it this
> > > other way? DECLARE_BITMAP() is a bit of an obscure way to declare a
> > > single unsigned long (in this case) where the compiler does the right
> > > thing anyway with a simple assign+cast making it easier to read/follow IMHO.
> >
> > What we can do is to declare BITS_PER_U32.
> > Also we can pay attention on these definitions:
> > https://elixir.bootlin.com/linux/latest/source/kernel/bpf/btf.c#L168
> > https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/ebitmap.c#L27

On the other hand, fixed width types are designed especially to
contain a specific
number of bits. I don't understand why BITS_PER_U32 is any better than
just 32 ...

> > And define BITMAP_FROM_U32() macro.
> >
> > Then It would be written like
> >
> > DECLARE_BITMAP(comp_mask_local, BITS_PER_U32) = BITMAP_FROM_U32(comp_mask);
> >
> > But this is a bit verbose.
> >
> > Also, it can be something like DECLARE_BITMAP_U32(...) = BITMAP_FROM_U32(...);

People often do things like this:
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pcie-hisi-error.c#L199

Maybe it's worth adding a shorthand for it, like CREATE_BITMAP32(name, val) and
CREATE_BITMAP64(name, val)?