bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

PATCH In review Andy Shevchenko 2 participants 9 messages Jun 1, 2018 – Jun 4, 2018

Subsystems (3)

HID Input drivers Bitmap API

lore

Share & tools

lore

Andy Shevchenko

author

2018-06-01 08:31 UTC #0 lore raw

PATCH 1/3 bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

A lot of code become ugly because of open coding allocations for bitmaps.

Introduce three helpers to allow users be more clear of intention and keep their code neat.

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/bitmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

include/linux/bitmap.h +16 -0

index 1ee46f492267..845822425393 100644
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/kernel.h>

@@ -104,6 +105,21 @@
  * contain all bit positions from 0 to 'bits' - 1.
  */
 
+static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+	return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+
+static inline unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+	return kcalloc(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+
+static inline void bitmap_free(const unsigned long *bitmap)
+{
+	kfree(bitmap);
+}
+
 /*
  * lib/bitmap.c provides these functions:
  */

Signature · 1 line

2.17.0

Andy Shevchenko

author

2018-06-01 08:31 UTC #1 lore raw

PATCH 2/3 Input: gpio-keys - Switch to bitmap_zalloc()

Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/input/keyboard/gpio_keys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/input/keyboard/gpio_keys.c +4 -4

index 052e37675086..492a971b95b5 100644
@@ -196,7 +196,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	ssize_t ret;
 	int i;
 
-	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	bits = bitmap_zalloc(n_events, GFP_KERNEL);
 	if (!bits)
 		return -ENOMEM;

@@ -216,7 +216,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	buf[ret++] = '\n';
 	buf[ret] = '\0';
 
-	kfree(bits);
+	bitmap_free(bits);
 
 	return ret;
 }

@@ -240,7 +240,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	ssize_t error;
 	int i;
 
-	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	bits = bitmap_zalloc(n_events, GFP_KERNEL);
 	if (!bits)
 		return -ENOMEM;

@@ -284,7 +284,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	mutex_unlock(&ddata->disable_lock);
 
 out:
-	kfree(bits);
+	bitmap_free(bits);
 	return error;
 }

Signature · 1 line

2.17.0

Dmitry Torokhov

maintainer Acked-by on v1 2/3

2018-06-01 18:34 UTC #2 lore raw

Re: [PATCH v1 2/3] Input: gpio-keys - Switch to bitmap_zalloc()

Quoted 4 lines · Andy Shevchenko

Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *.
Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Dmitry Torokhov <[email protected]>

Quoted 47 lines

---
 drivers/input/keyboard/gpio_keys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/input/keyboard/gpio_keys.c +4 -4

index 052e37675086..492a971b95b5 100644
@@ -196,7 +196,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	ssize_t ret;
 	int i;
 
-	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	bits = bitmap_zalloc(n_events, GFP_KERNEL);
 	if (!bits)
 		return -ENOMEM;

@@ -216,7 +216,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	buf[ret++] = '\n';
 	buf[ret] = '\0';
 
-	kfree(bits);
+	bitmap_free(bits);
 
 	return ret;
 }

@@ -240,7 +240,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	ssize_t error;
 	int i;
 
-	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	bits = bitmap_zalloc(n_events, GFP_KERNEL);
 	if (!bits)
 		return -ENOMEM;

@@ -284,7 +284,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	mutex_unlock(&ddata->disable_lock);
 
 out:
-	kfree(bits);
+	bitmap_free(bits);
 	return error;
 }

Signature · 1 line

2.17.0

Signature · 1 line

Dmitry

Andy Shevchenko

author

2018-06-01 08:31 UTC #3 lore raw

PATCH 3/3 Input: evdev - Switch to bitmap_zalloc()

In reply to Andy Shevchenko

Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/input/evdev.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

drivers/input/evdev.c +7 -9

index c81c79d01d93..3f87ef973bc7 100644
@@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 	evdev_detach_client(evdev, client);
 
 	for (i = 0; i < EV_CNT; ++i)
-		kfree(client->evmasks[i]);
+		bitmap_free(client->evmasks[i]);
 
 	kvfree(client);

@@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
 {
 	int ret;
 	unsigned long *mem;
-	size_t len;
 
-	len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
-	mem = kmalloc(len, GFP_KERNEL);
+	mem = bitmap_malloc(maxbit, GFP_KERNEL);
 	if (!mem)
 		return -ENOMEM;
 
 	spin_lock_irq(&dev->event_lock);
 	spin_lock(&client->buffer_lock);
 
-	memcpy(mem, bits, len);
+	bitmap_copy(mem, bits, maxbit);
 
 	spin_unlock(&dev->event_lock);

@@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 	if (ret < 0)
 		evdev_queue_syn_dropped(client);
 
-	kfree(mem);
+	bitmap_free(mem);
 
 	return ret;
 }

@@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
 	if (!cnt)
 		return 0;
 
-	mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
+	mask = bitmap_zalloc(cnt, GFP_KERNEL);
 	if (!mask)
 		return -ENOMEM;
 
 	error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
 	if (error < 0) {
-		kfree(mask);
+		bitmap_free(mask);
 		return error;
 	}

@@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
 	client->evmasks[type] = mask;
 	spin_unlock_irqrestore(&client->buffer_lock, flags);
 
-	kfree(oldmask);
+	bitmap_free(oldmask);
 
 	return 0;
 }

Signature · 1 line

2.17.0

Dmitry Torokhov

maintainer Acked-by on v1 3/3

2018-06-01 18:34 UTC #4 lore raw

Re: [PATCH v1 3/3] Input: evdev - Switch to bitmap_zalloc()

Quoted 4 lines · Andy Shevchenko

Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *.
Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Dmitry Torokhov <[email protected]>

Quoted 74 lines

---
 drivers/input/evdev.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

drivers/input/evdev.c +7 -9

index c81c79d01d93..3f87ef973bc7 100644
@@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 	evdev_detach_client(evdev, client);
 
 	for (i = 0; i < EV_CNT; ++i)
-		kfree(client->evmasks[i]);
+		bitmap_free(client->evmasks[i]);
 
 	kvfree(client);

@@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
 {
 	int ret;
 	unsigned long *mem;
-	size_t len;
 
-	len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
-	mem = kmalloc(len, GFP_KERNEL);
+	mem = bitmap_malloc(maxbit, GFP_KERNEL);
 	if (!mem)
 		return -ENOMEM;
 
 	spin_lock_irq(&dev->event_lock);
 	spin_lock(&client->buffer_lock);
 
-	memcpy(mem, bits, len);
+	bitmap_copy(mem, bits, maxbit);
 
 	spin_unlock(&dev->event_lock);

@@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 	if (ret < 0)
 		evdev_queue_syn_dropped(client);
 
-	kfree(mem);
+	bitmap_free(mem);
 
 	return ret;
 }

@@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
 	if (!cnt)
 		return 0;
 
-	mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
+	mask = bitmap_zalloc(cnt, GFP_KERNEL);
 	if (!mask)
 		return -ENOMEM;
 
 	error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
 	if (error < 0) {
-		kfree(mask);
+		bitmap_free(mask);
 		return error;
 	}

@@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
 	client->evmasks[type] = mask;
 	spin_unlock_irqrestore(&client->buffer_lock, flags);
 
-	kfree(oldmask);
+	bitmap_free(oldmask);
 
 	return 0;
 }

Signature · 1 line

2.17.0

Signature · 1 line

Dmitry

Dmitry Torokhov

maintainer

2018-06-01 18:33 UTC #5 lore raw

In reply to Andy Shevchenko

Hi Andy,

Quoted 6 lines · Andy Shevchenko

A lot of code become ugly because of open coding allocations for bitmaps.
Introduce three helpers to allow users be more clear of intention and keep their code neat.
Signed-off-by Andy Shevchenko <[email protected]>

This looks nice and I like how it simplifies drivers. How do we merge this?

Quoted 18 lines

---
 include/linux/bitmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

include/linux/bitmap.h +1 -0

index 1ee46f492267..845822425393 100644
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/kernel.h>

@@ -104,6 +105,21 @@
  * contain all bit positions from 0 to 'bits' - 1.

include/linux/bitmap.h line 115

Quoted 10 lines

include/linux/bitmap.h +8 -0

  */
 
+static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+	return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+
+static inline unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+	return kcalloc(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);

	retrun bitmap_alloc(nbits, flags | __GFP_ZERO);

Quoted 13 lines

include/linux/bitmap.h +7 -0

+}
+
+static inline void bitmap_free(const unsigned long *bitmap)
+{
+	kfree(bitmap);
+}
+
 /*
  * lib/bitmap.c provides these functions:
  */

Signature · 1 line

2.17.0

Thanks.

Signature · 1 line

Dmitry

I though about this, but decide not to rely on linux/gfp.h. If you think explicit __GFP_ZERO is better, I can replace in v2, or if you have a chance to do that when applying it would be appreciated.

Quoted 17 lines

Quoted 14 lines

include/linux/bitmap.h +7 -0

+}
+
+static inline void bitmap_free(const unsigned long *bitmap)
+{
+	kfree(bitmap);
+}
+
 /*
  * lib/bitmap.c provides these functions:
  */

Signature · 1 line

2.17.0

Thanks.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Dmitry Torokhov

maintainer

2018-06-04 21:57 UTC #7 lore raw

Quoted 12 lines · Andy Shevchenko

Quoted 11 lines · Dmitry Torokhov
Hi Andy,
Quoted 7 lines · Andy Shevchenko
A lot of code become ugly because of open coding allocations for bitmaps.
Introduce three helpers to allow users be more clear of intention and keep their code neat.
Signed-off-by Andy Shevchenko <[email protected]>

Quoted 8 lines

Quoted 1 line
This looks nice and I like how it simplifies drivers.
Thanks!
Quoted 2 lines
How do we merge
this?
I suppose through 'input' tree if there is no objections.

OK, let's wait for objections for a few days.

Quoted 36 lines

Quoted 32 lines

---
 include/linux/bitmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

include/linux/bitmap.h +9 -0

index 1ee46f492267..845822425393 100644
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/kernel.h>

@@ -104,6 +105,21 @@
  * contain all bit positions from 0 to 'bits' - 1.
  */
 
+static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t
 flags)
+{
+	return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned
 long), flags);
+}
+
+static inline unsigned long *bitmap_zalloc(unsigned int nbits,
 gfp_t flags)
+{
+	return kcalloc(BITS_TO_LONGS(nbits), sizeof(unsigned long),

Quoted 1 line

flags);

	retrun bitmap_alloc(nbits, flags | __GFP_ZERO);

Quoted 4 lines

Quoted 1 line
?
I though about this, but decide not to rely on linux/gfp.h. If you think explicit __GFP_ZERO is better, I can replace in v2, or if

I like it ;) and we already do it for kcalloc done via kmalloc_array with __GFP_ZERO or kzalloc (kmalloc with __GFP_ZERO).

Quoted 1 line

you have a chance to do that when applying it would be appreciated.

Yeah, I can do that, no problem.

Thanks.

Signature · 1 line

Dmitry

Dmitry Torokhov

maintainer

2018-06-04 22:14 UTC #8 lore raw

Quoted 65 lines · Dmitry Torokhov

Quoted 12 lines · Andy Shevchenko
Quoted 11 lines · Dmitry Torokhov
Hi Andy,
Quoted 7 lines · Andy Shevchenko
A lot of code become ugly because of open coding allocations for bitmaps.
Introduce three helpers to allow users be more clear of intention and keep their code neat.
Signed-off-by Andy Shevchenko <[email protected]>
Quoted 8 lines
Quoted 1 line
This looks nice and I like how it simplifies drivers.
Thanks!
Quoted 2 lines
How do we merge
this?
I suppose through 'input' tree if there is no objections.
OK, let's wait for objections for a few days.
Quoted 36 lines
Quoted 36 lines
Quoted 32 lines
---
 include/linux/bitmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
include/linux/bitmap.h +9 -0
index 1ee46f492267..845822425393 100644
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
 
@@ -104,6 +105,21 @@
  * contain all bit positions from 0 to 'bits' - 1.
  */
 
+static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t
 flags)
+{
+	return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned
 long), flags);
+}
+
+static inline unsigned long *bitmap_zalloc(unsigned int nbits,
 gfp_t flags)
+{
+	return kcalloc(BITS_TO_LONGS(nbits), sizeof(unsigned long),
Quoted 1 line
flags);
	retrun bitmap_alloc(nbits, flags | __GFP_ZERO);
Quoted 4 lines
Quoted 1 line
?
I though about this, but decide not to rely on linux/gfp.h. If you think explicit __GFP_ZERO is better, I can replace in v2, or if

Quoted 6 lines

I like it ;) and we already do it for kcalloc done via kmalloc_array with __GFP_ZERO or kzalloc (kmalloc with __GFP_ZERO).
Quoted 1 line
you have a chance to do that when applying it would be appreciated.
Yeah, I can do that, no problem.

Ugh, there is circular dependency:

CC arch/x86/kernel/asm-offsets.s
In file included from ./include/linux/cpumask.h:12:0,

                 from ./arch/x86/include/asm/cpumask.h:5,
                 from ./arch/x86/include/asm/msr.h:11,
                 from ./arch/x86/include/asm/processor.h:21,
                 from ./arch/x86/include/asm/cpufeature.h:5,
                 from ./arch/x86/include/asm/thread_info.h:53,
                 from ./include/linux/thread_info.h:38,
                 from ./arch/x86/include/asm/preempt.h:7,
                 from ./include/linux/preempt.h:81,
                 from ./include/linux/spinlock.h:51,
                 from ./include/linux/mmzone.h:8,
                 from ./include/linux/gfp.h:6,
                 from ./include/linux/slab.h:15,
                 from ./include/linux/crypto.h:24,
                 from arch/x86/kernel/asm-offsets.c:9:
./include/linux/bitmap.h: In function ‘bitmap_alloc’:
./include/linux/bitmap.h:110:9: error: implicit declaration of function ‘kmalloc_array’ [-Werror=implicit-function-declaration]
  return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
         ^~~~~~~~~~~~~
./include/linux/bitmap.h:110:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
  return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So we have slab.h ... -> cpumask.h -> bitmap.h -> slab.h -> BOOM

We will probably have to move implementation into lib/bitmap.h. I think that should be OK. Can you make the change?

Thanks.

Signature · 1 line

Dmitry