[PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

[PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

[Andy Shevchenko]

On 32-bit platform the size of long is only 32 bits which makes wrong offset
in the array of 64 bit size.

Calculate offset based on BITS_PER_LONG.

Fixes: 30544ed5de43 ("lib/bitmap: introduce bitmap_replace() helper")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_bitmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 5cb35a734462..af522577a76e 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -275,22 +275,23 @@ static void __init test_copy(void)
 static void __init test_replace(void)
 {
 	unsigned int nbits = 64;
+	unsigned int step = DIV_ROUND_UP(nbits, BITS_PER_LONG);
 	DECLARE_BITMAP(bmap, 1024);
 
 	bitmap_zero(bmap, 1024);
-	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
+	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
 	expect_eq_bitmap(bmap, exp3_0_1, nbits);
 
 	bitmap_zero(bmap, 1024);
-	bitmap_replace(bmap, &exp2[1], &exp2[0], exp2_to_exp3_mask, nbits);
+	bitmap_replace(bmap, &exp2[1 * step], &exp2[0 * step], exp2_to_exp3_mask, nbits);
 	expect_eq_bitmap(bmap, exp3_1_0, nbits);
 
 	bitmap_fill(bmap, 1024);
-	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
+	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
 	expect_eq_bitmap(bmap, exp3_0_1, nbits);
 
 	bitmap_fill(bmap, 1024);
-	bitmap_replace(bmap, &exp2[1], &exp2[0], exp2_to_exp3_mask, nbits);
+	bitmap_replace(bmap, &exp2[1 * step], &exp2[0 * step], exp2_to_exp3_mask, nbits);
 	expect_eq_bitmap(bmap, exp3_1_0, nbits);
 }
 
-- 
2.24.1

[PATCH v1 2/2] lib/test_bitmap: Fix address space when test user buffer

[Andy Shevchenko]

Force address space to avoid the following warning:

lib/test_bitmap.c:461:53: warning: incorrect type in argument 1 (different address spaces)
lib/test_bitmap.c:461:53:    expected char const [noderef] <asn:1> *ubuf
lib/test_bitmap.c:461:53:    got char const *in

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_bitmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index af522577a76e..8d3154457ff8 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -458,7 +458,8 @@ static void __init __test_bitmap_parse(int is_user)
 
 			set_fs(KERNEL_DS);
 			time = ktime_get();
-			err = bitmap_parse_user(test.in, len, bmap, test.nbits);
+			err = bitmap_parse_user((__force const char __user *)test.in, len,
+						bmap, test.nbits);
 			time = ktime_get() - time;
 			set_fs(orig_fs);
 		} else {
-- 
2.24.1

Re: [PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

[Yury Norov]

On Wed, Jan 08, 2020 at 08:46:10PM +0200, Andy Shevchenko wrote:
> On 32-bit platform the size of long is only 32 bits which makes wrong offset
> in the array of 64 bit size.
> 
> Calculate offset based on BITS_PER_LONG.
> 
> Fixes: 30544ed5de43 ("lib/bitmap: introduce bitmap_replace() helper")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_bitmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 5cb35a734462..af522577a76e 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -275,22 +275,23 @@ static void __init test_copy(void)
>  static void __init test_replace(void)
>  {
>  	unsigned int nbits = 64;
> +	unsigned int step = DIV_ROUND_UP(nbits, BITS_PER_LONG);

Step is already defined in this file:
        #define step (sizeof(u64) / sizeof(unsigned long))
to avoid the same problem in other test cases. Introducing another variant of 
it looks messy.

>  	DECLARE_BITMAP(bmap, 1024);
>  
>  	bitmap_zero(bmap, 1024);
> -	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
> +	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
>  	expect_eq_bitmap(bmap, exp3_0_1, nbits);

If nbits is always 64, why don't you pass 64 directly?

Yury

>  	bitmap_zero(bmap, 1024);
> -	bitmap_replace(bmap, &exp2[1], &exp2[0], exp2_to_exp3_mask, nbits);
> +	bitmap_replace(bmap, &exp2[1 * step], &exp2[0 * step], exp2_to_exp3_mask, nbits);
>  	expect_eq_bitmap(bmap, exp3_1_0, nbits);
>  
>  	bitmap_fill(bmap, 1024);
> -	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
> +	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
>  	expect_eq_bitmap(bmap, exp3_0_1, nbits);
>  
>  	bitmap_fill(bmap, 1024);
> -	bitmap_replace(bmap, &exp2[1], &exp2[0], exp2_to_exp3_mask, nbits);
> +	bitmap_replace(bmap, &exp2[1 * step], &exp2[0 * step], exp2_to_exp3_mask, nbits);
>  	expect_eq_bitmap(bmap, exp3_1_0, nbits);
>  }
>  
> -- 
> 2.24.1

Re: [PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

[Andy Shevchenko]

On Wed, Jan 08, 2020 at 11:24:37AM -0800, Yury Norov wrote:
> On Wed, Jan 08, 2020 at 08:46:10PM +0200, Andy Shevchenko wrote:
> > On 32-bit platform the size of long is only 32 bits which makes wrong offset
> > in the array of 64 bit size.
> > 
> > Calculate offset based on BITS_PER_LONG.
> > 
> > Fixes: 30544ed5de43 ("lib/bitmap: introduce bitmap_replace() helper")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> >  	unsigned int nbits = 64;
> > +	unsigned int step = DIV_ROUND_UP(nbits, BITS_PER_LONG);
> 
> Step is already defined in this file:
>         #define step (sizeof(u64) / sizeof(unsigned long))

...and later undefined.

> to avoid the same problem in other test cases. Introducing another variant of 
> it looks messy.

I don't see any problem.

> 
> >  	DECLARE_BITMAP(bmap, 1024);
> >  
> >  	bitmap_zero(bmap, 1024);
> > -	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
> > +	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
> >  	expect_eq_bitmap(bmap, exp3_0_1, nbits);
> 
> If nbits is always 64, why don't you pass 64 directly?

We may use any setting. For now it's 64, but nothing prevents us to extend to,
let's say, 75.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

[Yury Norov]

On Wed, Jan 08, 2020 at 10:26:54PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 08, 2020 at 11:24:37AM -0800, Yury Norov wrote:
> > On Wed, Jan 08, 2020 at 08:46:10PM +0200, Andy Shevchenko wrote:
> > > On 32-bit platform the size of long is only 32 bits which makes wrong offset
> > > in the array of 64 bit size.
> > > 
> > > Calculate offset based on BITS_PER_LONG.
> > > 
> > > Fixes: 30544ed5de43 ("lib/bitmap: introduce bitmap_replace() helper")
> > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> > >  	unsigned int nbits = 64;
> > > +	unsigned int step = DIV_ROUND_UP(nbits, BITS_PER_LONG);
> > 
> > Step is already defined in this file:
> >         #define step (sizeof(u64) / sizeof(unsigned long))
> 
> ...and later undefined.
> 
> > to avoid the same problem in other test cases. Introducing another variant of 
> > it looks messy.
> 
> I don't see any problem.

The problem is that you reimplement the functionality instead of
reuse.
 
> > >  	DECLARE_BITMAP(bmap, 1024);
> > >  
> > >  	bitmap_zero(bmap, 1024);
> > > -	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
> > > +	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
> > >  	expect_eq_bitmap(bmap, exp3_0_1, nbits);
> > 
> > If nbits is always 64, why don't you pass 64 directly?
> 
> We may use any setting. For now it's 64, but nothing prevents us to extend to,
> let's say, 75.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

[Andy Shevchenko]

On Wed, Jan 08, 2020 at 12:43:07PM -0800, Yury Norov wrote:
> On Wed, Jan 08, 2020 at 10:26:54PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 08, 2020 at 11:24:37AM -0800, Yury Norov wrote:
> > > On Wed, Jan 08, 2020 at 08:46:10PM +0200, Andy Shevchenko wrote:
> > > > On 32-bit platform the size of long is only 32 bits which makes wrong offset
> > > > in the array of 64 bit size.
> > > > 
> > > > Calculate offset based on BITS_PER_LONG.
> > > > 
> > > > Fixes: 30544ed5de43 ("lib/bitmap: introduce bitmap_replace() helper")
> > > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > > >  	unsigned int nbits = 64;
> > > > +	unsigned int step = DIV_ROUND_UP(nbits, BITS_PER_LONG);
> > > 
> > > Step is already defined in this file:
> > >         #define step (sizeof(u64) / sizeof(unsigned long))
> > 
> > ...and later undefined.
> > 
> > > to avoid the same problem in other test cases. Introducing another variant of 
> > > it looks messy.
> > 
> > I don't see any problem.
> 
> The problem is that you reimplement the functionality instead of
> reuse.

I may not reuse by the reason I mentioned above. The definition of step works
only for 64 bits, we may modify test case for any amount of bits to be tested.

I'll rename the variable to something else to reduce confusion.

>  
> > > >  	DECLARE_BITMAP(bmap, 1024);
> > > >  
> > > >  	bitmap_zero(bmap, 1024);
> > > > -	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
> > > > +	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
> > > >  	expect_eq_bitmap(bmap, exp3_0_1, nbits);
> > > 
> > > If nbits is always 64, why don't you pass 64 directly?
> > 
> > We may use any setting. For now it's 64, but nothing prevents us to extend to,
> > let's say, 75.
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 

-- 
With Best Regards,
Andy Shevchenko