On Fri, Nov 13, 2020 at 5:52 PM Syed Nayyar Waris [off-list ref] wrote:

On Wed, Nov 11, 2020 at 3:30 AM Syed Nayyar Waris [off-list ref] wrote:

quoted

On Tue, Nov 10, 2020 at 12:43:16PM -0500, William Breathitt Gray wrote:

quoted

On Tue, Nov 10, 2020 at 10:52:42PM +0530, Syed Nayyar Waris wrote:

quoted

On Tue, Nov 10, 2020 at 6:05 PM William Breathitt Gray
[off-list ref] wrote:

quoted

On Tue, Nov 10, 2020 at 11:02:43AM +0100, Michal Simek wrote:

quoted

On 09. 11. 20 18:31, William Breathitt Gray wrote:

quoted

On Mon, Nov 09, 2020 at 07:22:20PM +0200, Andy Shevchenko wrote:

quoted

On Mon, Nov 09, 2020 at 12:11:40PM -0500, William Breathitt Gray wrote:

quoted

On Mon, Nov 09, 2020 at 10:15:29PM +0530, Syed Nayyar Waris wrote:

quoted

On Mon, Nov 09, 2020 at 03:41:53PM +0100, Arnd Bergmann wrote:

...

quoted

quoted

 static inline void bitmap_set_value(unsigned long *map,
-                                    unsigned long value,
+                                    unsigned long value, const size_t length,
                                     unsigned long start, unsigned long nbits)
 {
         const size_t index = BIT_WORD(start);

@@ -15,6 +15,10 @@ static inline void bitmap_set_value(unsigned long *map,
         } else {
                 map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
                 map[index + 0] |= value << offset;
+
+               if (index + 1 >= length)
+                       __builtin_unreachable();
+
                 map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
                 map[index + 1] |= value >> space;
         }

Hi Syed,

Let's rename 'length' to 'nbits' as Arnd suggested, and rename 'nbits'
to value_width.

length here is in longs. I guess this is the point of entire patch.

Ah yes, this should become 'const unsigned long nbits' and represent the
length of the bitmap in bits and not longs.

Hi William, Andy and All,

Thank You for reviewing. I was looking into the review comments and I
have a question on the above.

Actually, in bitmap_set_value(), the intended comparison is to be made
between 'index + 1' and 'length' (which is now renamed as 'nbits').
That is, the comparison would look-like as follows:
if (index + 1 >= nbits)

The 'index' is getting populated with BIT_WORD(start).
The 'index' variable in above is the actual index of the bitmap array,
while in previous mail it is suggested to use 'nbits' which represent
the length of the bitmap in bits and not longs.

Isn't it comparing two different things? index of array (not the
bit-wise-length) on left hand side and nbits (bit-wise-length) on
right hand side?

Have I misunderstood something? If yes, request to clarify.

Or do I have to first divide 'nbits' by BITS_PER_LONG and then compare
it with 'index + 1'? Something like this?

Regards
Syed Nayyar Waris

The array elements of the bitmap memory region are abstracted away for
the covenience of the users of the bitmap_* functions; the driver
authors are able to treat their bitmaps as just a set of contiguous bits
and not worry about where the division between array elements happen.

So to match the interface of the other bitmap_* functions, you should
take in nbits and figure out the actual array length by dividing by
BITS_PER_LONG inside bitmap_set_value(). Then you can use your
conditional check (index + 1 >= length) like you have been doing so far.

William Breathitt Gray

Hi Arnd,

Sharing a new version of bitmap_set_value(). Let me know if it looks
good and whether it suppresses the compiler warning.

The below patch is created against the v12 version of bitmap_set_value().

-static inline void bitmap_set_value(unsigned long *map,
-                                    unsigned long value,
-                                    unsigned long start, unsigned long nbits)
+static inline void bitmap_set_value(unsigned long *map, unsigned long nbits,
+                                   unsigned long value, unsigned long value_width,
+                                   unsigned long start)
 {
-        const size_t index = BIT_WORD(start);
+        const unsigned long index = BIT_WORD(start);
+        const unsigned long length = BIT_WORD(nbits);
         const unsigned long offset = start % BITS_PER_LONG;
         const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG);
         const unsigned long space = ceiling - start;

-        value &= GENMASK(nbits - 1, 0);
+        value &= GENMASK(value_width - 1, 0);

-        if (space >= nbits) {
-                map[index] &= ~(GENMASK(nbits - 1, 0) << offset);
+        if (space >= value_width) {
+                map[index] &= ~(GENMASK(value_width - 1, 0) << offset);
                 map[index] |= value << offset;
         } else {
                 map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
                 map[index + 0] |= value << offset;
-                map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+
+               if (index + 1 >= length)
+                       __builtin_unreachable();
+
+                map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + value_width);
                 map[index + 1] |= value >> space;
         }
 }

Hi Arnd,

What do you think of the above solution ( new version of
bitmap_set_value() )? Does it look good?

Sorry for the late reply and thanks for continuing to look at solutions.

I don't really like the idea of having the __builtin_unreachable() in
there, since that would lead to even worse undefined behavior
(jumping to a random instruction) than the previous one (writing
to a random location) when invalid data gets passed.

Isn't passing the length of the bitmap sufficient to suppress the
warning (sorry I did not try myself)? If not, maybe this could
be a "BUG_ON(index + 1 >= length)" instead of the
__builtin_unreachable(). That way it would at least crash
in a well-defined way.

     Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel