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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx/ > > --- > > 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
