[RESEND] sh: sq: Use the bitmap API when applicable

  Using the bitmap API is less verbose than hand writing them.
It also improves the semantic.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This is a resend of [1].

Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y

[1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
---
 arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
  

Hi Christophe!

On Sun, 2023-04-16 at 21:05 +0200, Christophe JAILLET wrote:
> Using the bitmap API is less verbose than hand writing them.
> It also improves the semantic.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This is a resend of [1].
> 
> Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y
> 
> [1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
> ---
>  arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
> index 27f2e3da5aa2..d289e99dc118 100644
> --- a/arch/sh/kernel/cpu/sh4/sq.c
> +++ b/arch/sh/kernel/cpu/sh4/sq.c
> @@ -372,7 +372,6 @@ static struct subsys_interface sq_interface = {
>  static int __init sq_api_init(void)
>  {
>  	unsigned int nr_pages = 0x04000000 >> PAGE_SHIFT;
> -	unsigned int size = (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG;
>  	int ret = -ENOMEM;
>  
>  	printk(KERN_NOTICE "sq: Registering store queue API.\n");
> @@ -382,7 +381,7 @@ static int __init sq_api_init(void)
>  	if (unlikely(!sq_cache))
>  		return ret;
>  
> -	sq_bitmap = kzalloc(size, GFP_KERNEL);
> +	sq_bitmap = bitmap_zalloc(nr_pages, GFP_KERNEL);
>  	if (unlikely(!sq_bitmap))
>  		goto out;
>  
> @@ -393,7 +392,7 @@ static int __init sq_api_init(void)
>  	return 0;
>  
>  out:
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  
>  	return ret;
> @@ -402,7 +401,7 @@ static int __init sq_api_init(void)
>  static void __exit sq_api_exit(void)
>  {
>  	subsys_interface_unregister(&sq_interface);
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  }
>  

Thanks for resending this patch. I will try to review it next week
and also boot-test it on my SH7785LCR evaluation board.

I am not familiar with the bitmap API at the moment, so I might take
a little longer to review this patch.

Adrian
  

On Sun, Apr 16, 2023 at 9:10 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Using the bitmap API is less verbose than hand writing them.
> It also improves the semantic.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
  

On 4/16/23 14:05, Christophe JAILLET wrote:
> Using the bitmap API is less verbose than hand writing them.
> It also improves the semantic.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Tested-by: Rob Landley <rob@landley.net>

It booted and ran for me.

Rob
  

Hi Christophe!

Thanks for your patch. The changes look good to me. However, I have
one question, see below.

On Sun, 2023-04-16 at 21:05 +0200, Christophe JAILLET wrote:
> Using the bitmap API is less verbose than hand writing them.
> It also improves the semantic.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This is a resend of [1].
> 
> Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y
> 
> [1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
> ---
>  arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
> index 27f2e3da5aa2..d289e99dc118 100644
> --- a/arch/sh/kernel/cpu/sh4/sq.c
> +++ b/arch/sh/kernel/cpu/sh4/sq.c
> @@ -372,7 +372,6 @@ static struct subsys_interface sq_interface = {
>  static int __init sq_api_init(void)
>  {
>  	unsigned int nr_pages = 0x04000000 >> PAGE_SHIFT;
> -	unsigned int size = (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG;
>  	int ret = -ENOMEM;
>  
>  	printk(KERN_NOTICE "sq: Registering store queue API.\n");
> @@ -382,7 +381,7 @@ static int __init sq_api_init(void)
>  	if (unlikely(!sq_cache))
>  		return ret;
>  
> -	sq_bitmap = kzalloc(size, GFP_KERNEL);
> +	sq_bitmap = bitmap_zalloc(nr_pages, GFP_KERNEL);
>  	if (unlikely(!sq_bitmap))
>  		goto out;
> 

I have look through other patches where k{z,c,m}alloc() were replaced with
bitmap_zalloc() and I noticed that in the other cases such as [1], kcalloc()
was used instead of kzalloc() in our cases with the element size set to
sizeof(long) while kzalloc() is using an element size equal to a byte.

Wouldn't that mean that the current code in sq is allocating a buffer that is
too small by a factor of 1/sizeof(long) or am I missing something?

@Geert: Do you have any idea?

> @@ -393,7 +392,7 @@ static int __init sq_api_init(void)
>  	return 0;
>  
>  out:
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  
>  	return ret;
> @@ -402,7 +401,7 @@ static int __init sq_api_init(void)
>  static void __exit sq_api_exit(void)
>  {
>  	subsys_interface_unregister(&sq_interface);
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  }
>  

Adrian

> [1] https://lkml.org/lkml/2021/11/28/155
  

Hi Adrian,

On Tue, Apr 18, 2023 at 8:36 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> Thanks for your patch. The changes look good to me. However, I have
> one question, see below.
>
> On Sun, 2023-04-16 at 21:05 +0200, Christophe JAILLET wrote:
> > Using the bitmap API is less verbose than hand writing them.
> > It also improves the semantic.
> >
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

> > --- a/arch/sh/kernel/cpu/sh4/sq.c
> > +++ b/arch/sh/kernel/cpu/sh4/sq.c
> > @@ -372,7 +372,6 @@ static struct subsys_interface sq_interface = {
> >  static int __init sq_api_init(void)
> >  {
> >       unsigned int nr_pages = 0x04000000 >> PAGE_SHIFT;
> > -     unsigned int size = (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG;
> >       int ret = -ENOMEM;
> >
> >       printk(KERN_NOTICE "sq: Registering store queue API.\n");
> > @@ -382,7 +381,7 @@ static int __init sq_api_init(void)
> >       if (unlikely(!sq_cache))
> >               return ret;
> >
> > -     sq_bitmap = kzalloc(size, GFP_KERNEL);
> > +     sq_bitmap = bitmap_zalloc(nr_pages, GFP_KERNEL);
> >       if (unlikely(!sq_bitmap))
> >               goto out;
> >
>
> I have look through other patches where k{z,c,m}alloc() were replaced with
> bitmap_zalloc() and I noticed that in the other cases such as [1], kcalloc()
> was used instead of kzalloc() in our cases with the element size set to
> sizeof(long) while kzalloc() is using an element size equal to a byte.
>
> Wouldn't that mean that the current code in sq is allocating a buffer that is
> too small by a factor of 1/sizeof(long) or am I missing something?
>
> @Geert: Do you have any idea?

Nice catch!

Looking more deeply at the code, the intention is to allocate a bitmap
with nr_pages bits, so the code fater Christophe's patch is correct.
However, the old code is indeed wrong:

    (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG

The aim is to calculate the size in bytes, rounded up to an integral
number of longs, but it lacks a final multiplication by BITS_PER_BYTE,
so it's off by a factor of 4.

Fixes: d7c30c682a278abe ("sh: Store Queue API rework.")

As we didn't have bitmap_zalloc() until commit c42b65e363ce97a8
("bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()")
in v4.19, it would be good to fix the bug first in a separate patch,
not using

BTW, interesting how this got missed when fixing the other out-of-range
bug in commit 9f650cf2b811cfb6 ("sh: Fix store queue bitmap end.",
s/marc.theaimsgroup.com/marc.info/ when following the link).

Gr{oetje,eeting}s,

                        Geert
  

Hi Geert!

On Tue, 2023-04-18 at 09:14 +0200, Geert Uytterhoeven wrote:
> Hi Adrian,
> 
> On Tue, Apr 18, 2023 at 8:36 AM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
> > Thanks for your patch. The changes look good to me. However, I have
> > one question, see below.
> > 
> > On Sun, 2023-04-16 at 21:05 +0200, Christophe JAILLET wrote:
> > > Using the bitmap API is less verbose than hand writing them.
> > > It also improves the semantic.
> > > 
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> > > --- a/arch/sh/kernel/cpu/sh4/sq.c
> > > +++ b/arch/sh/kernel/cpu/sh4/sq.c
> > > @@ -372,7 +372,6 @@ static struct subsys_interface sq_interface = {
> > >  static int __init sq_api_init(void)
> > >  {
> > >       unsigned int nr_pages = 0x04000000 >> PAGE_SHIFT;
> > > -     unsigned int size = (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG;
> > >       int ret = -ENOMEM;
> > > 
> > >       printk(KERN_NOTICE "sq: Registering store queue API.\n");
> > > @@ -382,7 +381,7 @@ static int __init sq_api_init(void)
> > >       if (unlikely(!sq_cache))
> > >               return ret;
> > > 
> > > -     sq_bitmap = kzalloc(size, GFP_KERNEL);
> > > +     sq_bitmap = bitmap_zalloc(nr_pages, GFP_KERNEL);
> > >       if (unlikely(!sq_bitmap))
> > >               goto out;
> > > 
> > 
> > I have look through other patches where k{z,c,m}alloc() were replaced with
> > bitmap_zalloc() and I noticed that in the other cases such as [1], kcalloc()
> > was used instead of kzalloc() in our cases with the element size set to
> > sizeof(long) while kzalloc() is using an element size equal to a byte.
> > 
> > Wouldn't that mean that the current code in sq is allocating a buffer that is
> > too small by a factor of 1/sizeof(long) or am I missing something?
> > 
> > @Geert: Do you have any idea?
> 
> Nice catch!
> 
> Looking more deeply at the code, the intention is to allocate a bitmap
> with nr_pages bits, so the code fater Christophe's patch is correct.
> However, the old code is indeed wrong:
> 
>     (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG
> 
> The aim is to calculate the size in bytes, rounded up to an integral
> number of longs, but it lacks a final multiplication by BITS_PER_BYTE,
> so it's off by a factor of 4.

Yeah, that's what I understood from reading the code which is why I was
wondering why the factor was missing.

> Fixes: d7c30c682a278abe ("sh: Store Queue API rework.")
> 
> As we didn't have bitmap_zalloc() until commit c42b65e363ce97a8
> ("bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()")
> in v4.19, it would be good to fix the bug first in a separate patch,
> not using

I agree. Do you want to send a patch I can review?

> BTW, interesting how this got missed when fixing the other out-of-range
> bug in commit 9f650cf2b811cfb6 ("sh: Fix store queue bitmap end.",
> s/marc.theaimsgroup.com/marc.info/ when following the link).

Yeah.

PS: Sorry for the slightly messy grammar in my previous mail, didn't have
    a coffee yet that early in the morning :-).

Adrian
  

On Tue, Apr 18, 2023 at 08:36:40AM +0200, John Paul Adrian Glaubitz wrote:
> Hi Christophe!
> 
> Thanks for your patch. The changes look good to me. However, I have
> one question, see below.
> 
> On Sun, 2023-04-16 at 21:05 +0200, Christophe JAILLET wrote:
> > Using the bitmap API is less verbose than hand writing them.
> > It also improves the semantic.
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > This is a resend of [1].
> > 
> > Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y
> > 
> > [1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
> > ---
> >  arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
> > index 27f2e3da5aa2..d289e99dc118 100644
> > --- a/arch/sh/kernel/cpu/sh4/sq.c
> > +++ b/arch/sh/kernel/cpu/sh4/sq.c
> > @@ -372,7 +372,6 @@ static struct subsys_interface sq_interface = {
> >  static int __init sq_api_init(void)
> >  {
> >  	unsigned int nr_pages = 0x04000000 >> PAGE_SHIFT;
> > -	unsigned int size = (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG;
> >  	int ret = -ENOMEM;
> >  
> >  	printk(KERN_NOTICE "sq: Registering store queue API.\n");
> > @@ -382,7 +381,7 @@ static int __init sq_api_init(void)
> >  	if (unlikely(!sq_cache))
> >  		return ret;
> >  
> > -	sq_bitmap = kzalloc(size, GFP_KERNEL);
> > +	sq_bitmap = bitmap_zalloc(nr_pages, GFP_KERNEL);
> >  	if (unlikely(!sq_bitmap))
> >  		goto out;
> > 
> 
> I have look through other patches where k{z,c,m}alloc() were replaced with
> bitmap_zalloc() and I noticed that in the other cases such as [1], kcalloc()
> was used instead of kzalloc() in our cases with the element size set to
> sizeof(long) while kzalloc() is using an element size equal to a byte.
> 
> Wouldn't that mean that the current code in sq is allocating a buffer that is
> too small by a factor of 1/sizeof(long) or am I missing something?

Yes.  You are correct.  The original code is buggy.

size is the number of longs we need so kzalloc() is allocating a too
small buffer.  It should be kzalloc(size * sizeof(long), ... etc as you
say.

I have some unpublished Smatch stuff which tries to track "variable x
is in terms of bit units or byte units etc."  I will try to make a
static checker rule for this.

regards,
dan carpenter
  

On Tue, Apr 18, 2023 at 10:30:01AM +0300, Dan Carpenter wrote:
> I have some unpublished Smatch stuff which tries to track "variable x
> is in terms of bit units or byte units etc."  I will try to make a
> static checker rule for this.

Attached.  It prints a warning like this:

drivers/net/ethernet/broadcom/cnic.c:667 cnic_init_id_tbl() warn: allocating units of longs instead of bytes 'test_var'

I'll test it out tonight.

regards,
dan carpenter
  

Hi Dan!

On Tue, 2023-04-18 at 12:39 +0300, Dan Carpenter wrote:
> On Tue, Apr 18, 2023 at 10:30:01AM +0300, Dan Carpenter wrote:
> > I have some unpublished Smatch stuff which tries to track "variable x
> > is in terms of bit units or byte units etc."  I will try to make a
> > static checker rule for this.
> 
> Attached.  It prints a warning like this:
> 
> drivers/net/ethernet/broadcom/cnic.c:667 cnic_init_id_tbl() warn: allocating units of longs instead of bytes 'test_var'
> 
> I'll test it out tonight.

Nice job, thanks for creating this very handy script!

Adrian
  

Le 18/04/2023 à 09:14, Geert Uytterhoeven a écrit :
> 
> Nice catch!
> 
> Looking more deeply at the code, the intention is to allocate a bitmap
> with nr_pages bits, so the code fater Christophe's patch is correct.
> However, the old code is indeed wrong:
> 
>      (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG
> 
> The aim is to calculate the size in bytes, rounded up to an integral
> number of longs, but it lacks a final multiplication by BITS_PER_BYTE,
> so it's off by a factor of 4.
> 
> Fixes: d7c30c682a278abe ("sh: Store Queue API rework.")
> 
> As we didn't have bitmap_zalloc() until commit c42b65e363ce97a8
> ("bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()")
> in v4.19, it would be good to fix the bug first in a separate patch,
> not using
> 
> BTW, interesting how this got missed when fixing the other out-of-range
> bug in commit 9f650cf2b811cfb6 ("sh: Fix store queue bitmap end.",
> s/marc.theaimsgroup.com/marc.info/ when following the link).

So, this means that this got unnoticed for 16 years?
Waouh!

I would never have thought that a "trivial" clean-up that I took time to 
repost could trigger such a thing!

Again.
Waouh!


Maybe, 0x04000000 is way to big?
Anyone knows where this value comes from?

Could there have been some memory corruption in real world application?

CJ

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
  

On Tue, Apr 18, 2023 at 12:39:43PM +0300, Dan Carpenter wrote:
> On Tue, Apr 18, 2023 at 10:30:01AM +0300, Dan Carpenter wrote:
> > I have some unpublished Smatch stuff which tries to track "variable x
> > is in terms of bit units or byte units etc."  I will try to make a
> > static checker rule for this.
> 
> Attached.  It prints a warning like this:
> 
> drivers/net/ethernet/broadcom/cnic.c:667 cnic_init_id_tbl() warn: allocating units of longs instead of bytes 'test_var'
> 
> I'll test it out tonight.

It didn't find any bugs.  (Which is hardly surprising because most would
be caught in testing).  No false postives either.  I imagine that people
will introduce bugs like this in the future and now we'll see it when
that happens.

regards,
dan carpenter
  

Hi Christophe!

On Tue, 2023-04-18 at 20:05 +0200, Christophe JAILLET wrote:
> Le 18/04/2023 à 09:14, Geert Uytterhoeven a écrit :
> > 
> > Nice catch!
> > 
> > Looking more deeply at the code, the intention is to allocate a bitmap
> > with nr_pages bits, so the code fater Christophe's patch is correct.
> > However, the old code is indeed wrong:
> > 
> >      (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG
> > 
> > The aim is to calculate the size in bytes, rounded up to an integral
> > number of longs, but it lacks a final multiplication by BITS_PER_BYTE,
> > so it's off by a factor of 4.
> > 
> > Fixes: d7c30c682a278abe ("sh: Store Queue API rework.")
> > 
> > As we didn't have bitmap_zalloc() until commit c42b65e363ce97a8
> > ("bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()")
> > in v4.19, it would be good to fix the bug first in a separate patch,
> > not using
> > 
> > BTW, interesting how this got missed when fixing the other out-of-range
> > bug in commit 9f650cf2b811cfb6 ("sh: Fix store queue bitmap end.",
> > s/marc.theaimsgroup.com/marc.info/ when following the link).
> 
> So, this means that this got unnoticed for 16 years?
> Waouh!
> 
> I would never have thought that a "trivial" clean-up that I took time to 
> repost could trigger such a thing!

I have fixed the original bug in my for-next branch [1] now. Would you mind
rebasing your patch on top of that branch and resend it?

The reason why we're doing this is because we want to be able to backport the
fix to older kernel versions such as 4.14 which don't have the bitmap API yet.

Thanks,
Adrian

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/glaubitz/sh-linux.git/log/?h=for-next