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

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

[Yury Norov]

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.

We can use bitmap_from_arr32() here.

Since I have no hardware, the patch is compile-testes only.
Carsten, could you please test it and consider including into
your tree?

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..27968215e41d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -136,11 +136,12 @@ struct komeda_component *
 komeda_pipeline_get_first_component(struct komeda_pipeline *pipe,
 				    u32 comp_mask)
 {
+	DECLARE_BITMAP(comp_mask_local, 32);
 	struct komeda_component *c = NULL;
-	unsigned long comp_mask_local = (unsigned long)comp_mask;
 	int id;
 
-	id = find_first_bit(&comp_mask_local, 32);
+	bitmap_from_arr32(comp_mask_local, &comp_mask, 32);
+	id = find_first_bit(comp_mask_local, 32);
 	if (id < 32)
 		c = komeda_pipeline_get_component(pipe, id);
 
-- 
2.25.1

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Yury Norov]

On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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

> > -     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.

Yury

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

[Carsten Haitzler]

On 12/28/20 8:10 PM, Yury Norov wrote:
> On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[Andy Shevchenko]

On Tue, Dec 29, 2020 at 2:24 PM Carsten Haitzler
<Carsten.Haitzler@arm.com> wrote:
>
> On 12/28/20 8:10 PM, Yury Norov wrote:
> > On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> 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

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(...);

> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Hmm... you probably have to get rid of this footer.

-- 
With Best Regards,
Andy Shevchenko

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

[Yury Norov]

On Tue, Dec 29, 2020 at 5:50 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Dec 29, 2020 at 2:24 PM Carsten Haitzler
> <Carsten.Haitzler@arm.com> wrote:
> >
> > On 12/28/20 8:10 PM, Yury Norov wrote:
> > > On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> 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


> 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(...);
>
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
> Hmm... you probably have to get rid of this footer.
>
> --
> With Best Regards,
> Andy Shevchenko

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@gmail.com> wrote:
>
> On Tue, Dec 29, 2020 at 5:50 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Tue, Dec 29, 2020 at 2:24 PM Carsten Haitzler
> > <Carsten.Haitzler@arm.com> wrote:
> > >
> > > On 12/28/20 8:10 PM, Yury Norov wrote:
> > > > On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> 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)?