Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Yury Norov <ynorov@caviumnetworks.com>
To:     linux-kernel@vger.kernel.org
Cc:     Yury Norov <ynorov@caviumnetworks.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ben Hutchings <ben@decadent.org.uk>,
        David Decotigny <decot@googlers.com>,
        "David S . Miller" <davem@davemloft.net>,
        Matthew Wilcox <mawilcox@microsoft.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Subject: [PATCH RFC] lib: simplify bitmap_from_u32array API
Date:   Wed, 15 Nov 2017 20:40:56 +0300
Message-Id: <20171115174056.26537-1-ynorov@caviumnetworks.com>
MIME-Version: 1.0
Content-Type: text/plain
Received-SPF: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2017 17:41:11.2966 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: efccf076-bbee-45f2-9b5f-08d52c501054
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR0701MB3827
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

and make it looking like standard copy function:
bitmap_from_u32array(dst, src, size).

Currently bitmap_from_u32array() takes 4 arguments:
 - unsigned long *bitmap, which is destination;
 - unsigned int nbits, the length of destination bitmap, in bits;
 - const u32 *buf, the source; and
 - unsigned int nwords, the length of source buffer in ints.

In description to the function it is detailed like:
 * copy min(nbits, 32*nwords) bits from @buf to @bitmap, remaining
 * bits between nword and nbits in @bitmap (if any) are cleared.

Having two size arguments looks unneeded and potentially dangerous.

It is unneeded because normally user of copy-like function should
take care of the size of destination and make it big enough to fit
source data.

And it is dangerous because function may hide possible error if user
doesn't provide big enough bitmap, and data becomes silently dropped.

That's why all copy-like functions have 1 argument for size of copying
data, and I don't see any reason to make bitmap_from_u32array()
different.

One exception that comes in mind is strncpy() which also provides size
of destination in arguments, but it's strongly argued by the possibility
of taking broken strings in source, which is not the case of
bitmap_from_u32array().

There is no many real users of bitmap_from_u32array(), and they all
very clearly provide size of destination matched with the size of
source, so additional functionality is not used. Like this:
        bitmap_from_u32array(to->link_modes.supported,
                            __ETHTOOL_LINK_MODE_MASK_NBITS,
                             link_usettings.link_modes.supported,
			     __ETHTOOL_LINK_MODE_MASK_NU32);
Where:
#define __ETHTOOL_LINK_MODE_MASK_NU32 \
        DIV_ROUND_UP(__ETHTOOL_LINK_MODE_MASK_NBITS, 32)

This patch demonstrates how the code will look with simplified API.
If this simplification will be found useful, I'll send complete version
that also reworks bitmap_to_u32array() and fixes corresponding test in
lib/test_bitmap.c. I also expect that internal logic of functions will
be simplified.

Boot-tested on arm64.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Ben Hutchings <ben@decadent.org.uk>
CC: David Decotigny <decot@googlers.com>
CC: David S. Miller <davem@davemloft.net>
CC: Matthew Wilcox <mawilcox@microsoft.com>
CC: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/arm64/kernel/perf_event.c |  5 ++---
 include/linux/bitmap.h         |  7 +++----
 lib/bitmap.c                   | 19 +++++++------------
 net/core/ethtool.c             | 21 +++++++--------------
 4 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 9eaef51f83ff..70f4fd1952b6 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -931,9 +931,8 @@ static void __armv8pmu_probe_pmu(void *info)
 	pmceid[0] = read_sysreg(pmceid0_el0);
 	pmceid[1] = read_sysreg(pmceid1_el0);
 
-	bitmap_from_u32array(cpu_pmu->pmceid_bitmap,
-			     ARMV8_PMUV3_MAX_COMMON_EVENTS, pmceid,
-			     ARRAY_SIZE(pmceid));
+	bitmap_from_u32array(cpu_pmu->pmceid_bitmap, pmceid,
+			     ARMV8_PMUV3_MAX_COMMON_EVENTS);
 }
 
 static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 19748a5b0e77..b3c4bc04a466 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -60,7 +60,7 @@
  * bitmap_find_free_region(bitmap, bits, order)	Find and allocate bit region
  * bitmap_release_region(bitmap, pos, order)	Free specified bit region
  * bitmap_allocate_region(bitmap, pos, order)	Allocate specified bit region
- * bitmap_from_u32array(dst, nbits, buf, nwords) *dst = *buf (nwords 32b words)
+ * bitmap_from_u32array(dst, buf, nbits)	*dst = *buf (nwords 32b words)
  * bitmap_to_u32array(buf, nwords, src, nbits)	*buf = *dst (nwords 32b words)
  */
 
@@ -165,10 +165,9 @@ extern void bitmap_fold(unsigned long *dst, const unsigned long *orig,
 extern int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, int order);
 extern void bitmap_release_region(unsigned long *bitmap, unsigned int pos, int order);
 extern int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order);
-extern unsigned int bitmap_from_u32array(unsigned long *bitmap,
-					 unsigned int nbits,
+extern void *bitmap_from_u32array(unsigned long *bitmap,
 					 const u32 *buf,
-					 unsigned int nwords);
+					 unsigned int nbits);
 extern unsigned int bitmap_to_u32array(u32 *buf,
 				       unsigned int nwords,
 				       const unsigned long *bitmap,
diff --git a/lib/bitmap.c b/lib/bitmap.c
index c82c61b66e16..9d99f2f81281 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1108,22 +1108,17 @@ EXPORT_SYMBOL(bitmap_allocate_region);
  *	@bitmap: array of unsigned longs, the destination bitmap, non NULL
  *	@nbits: number of bits in @bitmap
  *	@buf: array of u32 (in host byte order), the source bitmap, non NULL
- *	@nwords: number of u32 words in @buf
- *
- * copy min(nbits, 32*nwords) bits from @buf to @bitmap, remaining
- * bits between nword and nbits in @bitmap (if any) are cleared. In
- * last word of @bitmap, the bits beyond nbits (if any) are kept
- * unchanged.
  *
- * Return the number of bits effectively copied.
+ * copy nbits bits from @buf to @bitmap. In last word of @bitmap, the bits
+ * beyond nbits (if any) are kept unchanged.
  */
-unsigned int
-bitmap_from_u32array(unsigned long *bitmap, unsigned int nbits,
-		     const u32 *buf, unsigned int nwords)
+void *bitmap_from_u32array(unsigned long *bitmap, const u32 *buf,
+						unsigned int nbits)
 {
 	unsigned int dst_idx, src_idx;
+	unsigned int nwords = BITS_TO_LONGS(nbits);
 
-	for (src_idx = dst_idx = 0; dst_idx < BITS_TO_LONGS(nbits); ++dst_idx) {
+	for (src_idx = dst_idx = 0; dst_idx < nwords; ++dst_idx) {
 		unsigned long part = 0;
 
 		if (src_idx < nwords)
@@ -1144,7 +1139,7 @@ bitmap_from_u32array(unsigned long *bitmap, unsigned int nbits,
 		}
 	}
 
-	return min_t(unsigned int, nbits, 32*nwords);
+	return bitmap;
 }
 EXPORT_SYMBOL(bitmap_from_u32array);
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 9a9a3d77e327..6ee127bb04e5 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -600,17 +600,14 @@ static int load_link_ksettings_from_user(struct ethtool_link_ksettings *to,
 
 	memcpy(&to->base, &link_usettings.base, sizeof(to->base));
 	bitmap_from_u32array(to->link_modes.supported,
-			     __ETHTOOL_LINK_MODE_MASK_NBITS,
 			     link_usettings.link_modes.supported,
-			     __ETHTOOL_LINK_MODE_MASK_NU32);
+			     __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_from_u32array(to->link_modes.advertising,
-			     __ETHTOOL_LINK_MODE_MASK_NBITS,
 			     link_usettings.link_modes.advertising,
-			     __ETHTOOL_LINK_MODE_MASK_NU32);
+			     __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_from_u32array(to->link_modes.lp_advertising,
-			     __ETHTOOL_LINK_MODE_MASK_NBITS,
 			     link_usettings.link_modes.lp_advertising,
-			     __ETHTOOL_LINK_MODE_MASK_NU32);
+			     __ETHTOOL_LINK_MODE_MASK_NBITS);
 
 	return 0;
 }
@@ -2343,10 +2340,8 @@ static int ethtool_get_per_queue_coalesce(struct net_device *dev,
 
 	useraddr += sizeof(*per_queue_opt);
 
-	bitmap_from_u32array(queue_mask,
-			     MAX_NUM_QUEUE,
-			     per_queue_opt->queue_mask,
-			     DIV_ROUND_UP(MAX_NUM_QUEUE, 32));
+	bitmap_from_u32array(queue_mask, per_queue_opt->queue_mask,
+			     MAX_NUM_QUEUE);
 
 	for_each_set_bit(bit, queue_mask, MAX_NUM_QUEUE) {
 		struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
@@ -2378,10 +2373,8 @@ static int ethtool_set_per_queue_coalesce(struct net_device *dev,
 
 	useraddr += sizeof(*per_queue_opt);
 
-	bitmap_from_u32array(queue_mask,
-			     MAX_NUM_QUEUE,
-			     per_queue_opt->queue_mask,
-			     DIV_ROUND_UP(MAX_NUM_QUEUE, 32));
+	bitmap_from_u32array(queue_mask, per_queue_opt->queue_mask,
+			     MAX_NUM_QUEUE);
 	n_queue = bitmap_weight(queue_mask, MAX_NUM_QUEUE);
 	tmp = backup = kmalloc_array(n_queue, sizeof(*backup), GFP_KERNEL);
 	if (!backup)
-- 
2.11.0


From 1584787231269125845@xxx Wed Nov 22 17:14:39 +0000 2017
X-GM-THRID: 1584785590923131463
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread