Hello Simon,
Thank for your review.
On Wed, 4 Oct 2023 13:07:14 +0200 Simon Horman horms@kernel.org wrote:
On Tue, Oct 03, 2023 at 10:56:52AM +0200, Köry Maincent wrote:
From: Kory Maincent kory.maincent@bootlin.com
@@ -448,8 +450,11 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, } no_mask = tb[ETHTOOL_A_BITSET_NOMASK];
- if (no_mask)
ethnl_bitmap32_clear(bitmap, 0, nbits, mod);
- if (no_mask) {
tmp = kcalloc(nbits, sizeof(u32), GFP_KERNEL);memcpy(tmp, bitmap, nbits);Hi Köry,
I'm no expert on etnhl bitmaps. But the above doesn't seem correct to me. Given that sizeof(u32) == 4:
- The allocation is for nbits * 4 bytes
- The copy is for its for nbits bytes
- I believe that bitmap contains space for the value followed by a mask. So it seems to me the size of bitmap, in words, is DIV_ROUND_UP(nbits, 32) * 2 And in bytes: DIV_ROUND_UP(nbits, 32) * 16 But perhaps only half is needed if only the value part of tmp is used.
If I'm on the right track here I'd suggest helpers might be in order.
You are right I should use the same alloc as ethnl_update_bitset with tmp instead of bitmap32:
u32 small_bitmap32[ETHNL_SMALL_BITMAP_WORDS]; u32 *bitmap32 = small_bitmap32; if (nbits > ETHNL_SMALL_BITMAP_BITS) { unsigned int dst_words = DIV_ROUND_UP(nbits, 32);
bitmap32 = kmalloc_array(dst_words, sizeof(u32), GFP_KERNEL); if (!bitmap32) return -ENOMEM; }
But I am still wondering if it needs to be double as you said for the size of the value followed by the mask. Not sure about it, as ethnl_update_bitset does not do it.
Regards,