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@xxxxxxxxxxxx>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > 
> > > >  	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