* [PATCH net-next v2 01/10] lib: packing: refuse operating on bit indices which exceed size of buffer
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:02 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 02/10] lib: packing: adjust definitions and implementation for arbitrary buffer lengths Jacob Keller
` (9 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean, Przemek Kitszel
From: Vladimir Oltean <vladimir.oltean@nxp.com>
While reworking the implementation, it became apparent that this check
does not exist.
There is no functional issue yet, because at call sites, "startbit" and
"endbit" are always hardcoded to correct values, and never come from the
user.
Even with the upcoming support of arbitrary buffer lengths, the
"startbit >= 8 * pbuflen" check will remain correct. This is because
we intend to always interpret the packed buffer in a way that avoids
discontinuities in the available bit indices.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
lib/packing.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/packing.c b/lib/packing.c
index 3f656167c17e..439125286d2b 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -86,8 +86,10 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
*/
int plogical_first_u8, plogical_last_u8, box;
- /* startbit is expected to be larger than endbit */
- if (startbit < endbit)
+ /* startbit is expected to be larger than endbit, and both are
+ * expected to be within the logically addressable range of the buffer.
+ */
+ if (unlikely(startbit < endbit || startbit >= 8 * pbuflen || endbit < 0))
/* Invalid function call */
return -EINVAL;
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 01/10] lib: packing: refuse operating on bit indices which exceed size of buffer
2024-10-02 21:51 ` [PATCH net-next v2 01/10] lib: packing: refuse operating on bit indices which exceed size of buffer Jacob Keller
@ 2024-10-03 15:02 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:02 UTC (permalink / raw)
To: Jacob Keller
Cc: Andrew Morton, Vladimir Oltean, David S. Miller, netdev,
Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:50PM -0700, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> While reworking the implementation, it became apparent that this check
> does not exist.
>
> There is no functional issue yet, because at call sites, "startbit" and
> "endbit" are always hardcoded to correct values, and never come from the
> user.
>
> Even with the upcoming support of arbitrary buffer lengths, the
> "startbit >= 8 * pbuflen" check will remain correct. This is because
> we intend to always interpret the packed buffer in a way that avoids
> discontinuities in the available bit indices.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Tested-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 02/10] lib: packing: adjust definitions and implementation for arbitrary buffer lengths
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
2024-10-02 21:51 ` [PATCH net-next v2 01/10] lib: packing: refuse operating on bit indices which exceed size of buffer Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:05 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 03/10] lib: packing: remove kernel-doc from header file Jacob Keller
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean, Przemek Kitszel
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Jacob Keller has a use case for packing() in the intel/ice networking
driver, but it cannot be used as-is.
Simply put, the API quirks for LSW32_IS_FIRST and LITTLE_ENDIAN are
naively implemented with the undocumented assumption that the buffer
length must be a multiple of 4. All calculations of group offsets and
offsets of bytes within groups assume that this is the case. But in the
ice case, this does not hold true. For example, packing into a buffer
of 22 bytes would yield wrong results, but pretending it was a 24 byte
buffer would work.
Rather than requiring such hacks, and leaving a big question mark when
it comes to discontinuities in the accessible bit fields of such buffer,
we should extend the packing API to support this use case.
It turns out that we can keep the design in terms of groups of 4 bytes,
but also make it work if the total length is not a multiple of 4.
Just like before, imagine the buffer as a big number, and its most
significant bytes (the ones that would make up to a multiple of 4) are
missing. Thus, with a big endian (no quirks) interpretation of the
buffer, those most significant bytes would be absent from the beginning
of the buffer, and with a LSW32_IS_FIRST interpretation, they would be
absent from the end of the buffer. The LITTLE_ENDIAN quirk, in the
packing() API world, only affects byte ordering within groups of 4.
Thus, it does not change which bytes are missing. Only the significance
of the remaining bytes within the (smaller) group.
No change intended for buffer sizes which are multiples of 4. Tested
with the sja1105 driver and with downstream unit tests.
Link: https://lore.kernel.org/netdev/a0338310-e66c-497c-bc1f-a597e50aa3ff@intel.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
lib/packing.c | 70 ++++++++++++++++++++++---------------
Documentation/core-api/packing.rst | 71 ++++++++++++++++++++++++++++++++++++++
2 files changed, 114 insertions(+), 27 deletions(-)
diff --git a/lib/packing.c b/lib/packing.c
index 439125286d2b..435236a914fe 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -9,27 +9,6 @@
#include <linux/types.h>
#include <linux/bitrev.h>
-static int get_le_offset(int offset)
-{
- int closest_multiple_of_4;
-
- closest_multiple_of_4 = (offset / 4) * 4;
- offset -= closest_multiple_of_4;
- return closest_multiple_of_4 + (3 - offset);
-}
-
-static int get_reverse_lsw32_offset(int offset, size_t len)
-{
- int closest_multiple_of_4;
- int word_index;
-
- word_index = offset / 4;
- closest_multiple_of_4 = word_index * 4;
- offset -= closest_multiple_of_4;
- word_index = (len / 4) - word_index - 1;
- return word_index * 4 + offset;
-}
-
static void adjust_for_msb_right_quirk(u64 *to_write, int *box_start_bit,
int *box_end_bit, u8 *box_mask)
{
@@ -47,6 +26,48 @@ static void adjust_for_msb_right_quirk(u64 *to_write, int *box_start_bit,
*box_end_bit = new_box_end_bit;
}
+/**
+ * calculate_box_addr - Determine physical location of byte in buffer
+ * @box: Index of byte within buffer seen as a logical big-endian big number
+ * @len: Size of buffer in bytes
+ * @quirks: mask of QUIRK_LSW32_IS_FIRST and QUIRK_LITTLE_ENDIAN
+ *
+ * Function interprets the buffer as a @len byte sized big number, and returns
+ * the physical offset of the @box logical octet within it. Internally, it
+ * treats the big number as groups of 4 bytes. If @len is not a multiple of 4,
+ * the last group may be shorter.
+ *
+ * @QUIRK_LSW32_IS_FIRST gives the ordering of groups of 4 octets relative to
+ * each other. If set, the most significant group of 4 octets is last in the
+ * buffer (and may be truncated if @len is not a multiple of 4).
+ *
+ * @QUIRK_LITTLE_ENDIAN gives the ordering of bytes within each group of 4.
+ * If set, the most significant byte is last in the group. If @len takes the
+ * form of 4k+3, the last group will only be able to represent 24 bits, and its
+ * most significant octet is byte 2.
+ *
+ * Return: the physical offset into the buffer corresponding to the logical box.
+ */
+static int calculate_box_addr(int box, size_t len, u8 quirks)
+{
+ size_t offset_of_group, offset_in_group, this_group = box / 4;
+ size_t group_size;
+
+ if (quirks & QUIRK_LSW32_IS_FIRST)
+ offset_of_group = this_group * 4;
+ else
+ offset_of_group = len - ((this_group + 1) * 4);
+
+ group_size = min(4, len - offset_of_group);
+
+ if (quirks & QUIRK_LITTLE_ENDIAN)
+ offset_in_group = box - this_group * 4;
+ else
+ offset_in_group = group_size - (box - this_group * 4) - 1;
+
+ return offset_of_group + offset_in_group;
+}
+
/**
* packing - Convert numbers (currently u64) between a packed and an unpacked
* format. Unpacked means laid out in memory in the CPU's native
@@ -157,12 +178,7 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
* effective addressing inside the pbuf (so it's not
* logical any longer).
*/
- box_addr = pbuflen - box - 1;
- if (quirks & QUIRK_LITTLE_ENDIAN)
- box_addr = get_le_offset(box_addr);
- if (quirks & QUIRK_LSW32_IS_FIRST)
- box_addr = get_reverse_lsw32_offset(box_addr,
- pbuflen);
+ box_addr = calculate_box_addr(box, pbuflen, quirks);
if (op == UNPACK) {
u64 pval;
diff --git a/Documentation/core-api/packing.rst b/Documentation/core-api/packing.rst
index 3ed13bc9a195..821691f23c54 100644
--- a/Documentation/core-api/packing.rst
+++ b/Documentation/core-api/packing.rst
@@ -151,6 +151,77 @@ the more significant 4-byte word.
We always think of our offsets as if there were no quirk, and we translate
them afterwards, before accessing the memory region.
+Note on buffer lengths not multiple of 4
+----------------------------------------
+
+To deal with memory layout quirks where groups of 4 bytes are laid out "little
+endian" relative to each other, but "big endian" within the group itself, the
+concept of groups of 4 bytes is intrinsic to the packing API (not to be
+confused with the memory access, which is performed byte by byte, though).
+
+With buffer lengths not multiple of 4, this means one group will be incomplete.
+Depending on the quirks, this may lead to discontinuities in the bit fields
+accessible through the buffer. The packing API assumes discontinuities were not
+the intention of the memory layout, so it avoids them by effectively logically
+shortening the most significant group of 4 octets to the number of octets
+actually available.
+
+Example with a 31 byte sized buffer given below. Physical buffer offsets are
+implicit, and increase from left to right within a group, and from top to
+bottom within a column.
+
+No quirks:
+
+::
+
+ 31 29 28 | Group 7 (most significant)
+ 27 26 25 24 | Group 6
+ 23 22 21 20 | Group 5
+ 19 18 17 16 | Group 4
+ 15 14 13 12 | Group 3
+ 11 10 9 8 | Group 2
+ 7 6 5 4 | Group 1
+ 3 2 1 0 | Group 0 (least significant)
+
+QUIRK_LSW32_IS_FIRST:
+
+::
+
+ 3 2 1 0 | Group 0 (least significant)
+ 7 6 5 4 | Group 1
+ 11 10 9 8 | Group 2
+ 15 14 13 12 | Group 3
+ 19 18 17 16 | Group 4
+ 23 22 21 20 | Group 5
+ 27 26 25 24 | Group 6
+ 30 29 28 | Group 7 (most significant)
+
+QUIRK_LITTLE_ENDIAN:
+
+::
+
+ 30 28 29 | Group 7 (most significant)
+ 24 25 26 27 | Group 6
+ 20 21 22 23 | Group 5
+ 16 17 18 19 | Group 4
+ 12 13 14 15 | Group 3
+ 8 9 10 11 | Group 2
+ 4 5 6 7 | Group 1
+ 0 1 2 3 | Group 0 (least significant)
+
+QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST:
+
+::
+
+ 0 1 2 3 | Group 0 (least significant)
+ 4 5 6 7 | Group 1
+ 8 9 10 11 | Group 2
+ 12 13 14 15 | Group 3
+ 16 17 18 19 | Group 4
+ 20 21 22 23 | Group 5
+ 24 25 26 27 | Group 6
+ 28 29 30 | Group 7 (most significant)
+
Intended use
------------
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 02/10] lib: packing: adjust definitions and implementation for arbitrary buffer lengths
2024-10-02 21:51 ` [PATCH net-next v2 02/10] lib: packing: adjust definitions and implementation for arbitrary buffer lengths Jacob Keller
@ 2024-10-03 15:05 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:05 UTC (permalink / raw)
To: Jacob Keller
Cc: Andrew Morton, Vladimir Oltean, David S. Miller, netdev,
Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:51PM -0700, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Jacob Keller has a use case for packing() in the intel/ice networking
> driver, but it cannot be used as-is.
>
> Simply put, the API quirks for LSW32_IS_FIRST and LITTLE_ENDIAN are
> naively implemented with the undocumented assumption that the buffer
> length must be a multiple of 4. All calculations of group offsets and
> offsets of bytes within groups assume that this is the case. But in the
> ice case, this does not hold true. For example, packing into a buffer
> of 22 bytes would yield wrong results, but pretending it was a 24 byte
> buffer would work.
>
> Rather than requiring such hacks, and leaving a big question mark when
> it comes to discontinuities in the accessible bit fields of such buffer,
> we should extend the packing API to support this use case.
>
> It turns out that we can keep the design in terms of groups of 4 bytes,
> but also make it work if the total length is not a multiple of 4.
> Just like before, imagine the buffer as a big number, and its most
> significant bytes (the ones that would make up to a multiple of 4) are
> missing. Thus, with a big endian (no quirks) interpretation of the
> buffer, those most significant bytes would be absent from the beginning
> of the buffer, and with a LSW32_IS_FIRST interpretation, they would be
> absent from the end of the buffer. The LITTLE_ENDIAN quirk, in the
> packing() API world, only affects byte ordering within groups of 4.
> Thus, it does not change which bytes are missing. Only the significance
> of the remaining bytes within the (smaller) group.
>
> No change intended for buffer sizes which are multiples of 4. Tested
> with the sja1105 driver and with downstream unit tests.
>
> Link: https://lore.kernel.org/netdev/a0338310-e66c-497c-bc1f-a597e50aa3ff@intel.com/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Tested-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 03/10] lib: packing: remove kernel-doc from header file
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
2024-10-02 21:51 ` [PATCH net-next v2 01/10] lib: packing: refuse operating on bit indices which exceed size of buffer Jacob Keller
2024-10-02 21:51 ` [PATCH net-next v2 02/10] lib: packing: adjust definitions and implementation for arbitrary buffer lengths Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:05 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 04/10] lib: packing: add pack() and unpack() wrappers over packing() Jacob Keller
` (7 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean, Przemek Kitszel
From: Vladimir Oltean <vladimir.oltean@nxp.com>
It is not necessary to have the kernel-doc duplicated both in the
header and in the implementation. It is better to have it near the
implementation of the function, since in C, a function can have N
declarations, but only one definition.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
include/linux/packing.h | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/include/linux/packing.h b/include/linux/packing.h
index 8d6571feb95d..69baefebcd02 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -17,32 +17,6 @@ enum packing_op {
UNPACK,
};
-/**
- * packing - Convert numbers (currently u64) between a packed and an unpacked
- * format. Unpacked means laid out in memory in the CPU's native
- * understanding of integers, while packed means anything else that
- * requires translation.
- *
- * @pbuf: Pointer to a buffer holding the packed value.
- * @uval: Pointer to an u64 holding the unpacked value.
- * @startbit: The index (in logical notation, compensated for quirks) where
- * the packed value starts within pbuf. Must be larger than, or
- * equal to, endbit.
- * @endbit: The index (in logical notation, compensated for quirks) where
- * the packed value ends within pbuf. Must be smaller than, or equal
- * to, startbit.
- * @op: If PACK, then uval will be treated as const pointer and copied (packed)
- * into pbuf, between startbit and endbit.
- * If UNPACK, then pbuf will be treated as const pointer and the logical
- * value between startbit and endbit will be copied (unpacked) to uval.
- * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
- * QUIRK_MSB_ON_THE_RIGHT.
- *
- * Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
- * correct usage, return code may be discarded.
- * If op is PACK, pbuf is modified.
- * If op is UNPACK, uval is modified.
- */
int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
enum packing_op op, u8 quirks);
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 03/10] lib: packing: remove kernel-doc from header file
2024-10-02 21:51 ` [PATCH net-next v2 03/10] lib: packing: remove kernel-doc from header file Jacob Keller
@ 2024-10-03 15:05 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:05 UTC (permalink / raw)
To: Jacob Keller
Cc: Andrew Morton, Vladimir Oltean, David S. Miller, netdev,
Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:52PM -0700, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> It is not necessary to have the kernel-doc duplicated both in the
> header and in the implementation. It is better to have it near the
> implementation of the function, since in C, a function can have N
> declarations, but only one definition.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 04/10] lib: packing: add pack() and unpack() wrappers over packing()
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (2 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 03/10] lib: packing: remove kernel-doc from header file Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:13 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 05/10] lib: packing: duplicate pack() and unpack() implementations Jacob Keller
` (6 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean, Przemek Kitszel
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Geert Uytterhoeven described packing() as "really bad API" because of
not being able to enforce const correctness. The same function is used
both when "pbuf" is input and "uval" is output, as in the other way
around.
Create 2 wrapper functions where const correctness can be ensured.
Do ugly type casts inside, to be able to reuse packing() as currently
implemented - which will _not_ modify the input argument.
Also, take the opportunity to change the type of startbit and endbit to
size_t - an unsigned type - in these new function prototypes. When int,
an extra check for negative values is necessary. Hopefully, when
packing() goes away completely, that check can be dropped.
My concern is that code which does rely on the conditional directionality
of packing() is harder to refactor without blowing up in size. So it may
take a while to completely eliminate packing(). But let's make alternatives
available for those who do not need that.
Link: https://lore.kernel.org/netdev/20210223112003.2223332-1-geert+renesas@glider.be/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
include/linux/packing.h | 6 ++++++
lib/packing.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
diff --git a/include/linux/packing.h b/include/linux/packing.h
index 69baefebcd02..ea25cb93cc70 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -20,4 +20,10 @@ enum packing_op {
int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
enum packing_op op, u8 quirks);
+int pack(void *pbuf, const u64 *uval, size_t startbit, size_t endbit,
+ size_t pbuflen, u8 quirks);
+
+int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
+ size_t pbuflen, u8 quirks);
+
#endif
diff --git a/lib/packing.c b/lib/packing.c
index 435236a914fe..2922db8a528c 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -90,6 +90,8 @@ static int calculate_box_addr(int box, size_t len, u8 quirks)
* @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
* QUIRK_MSB_ON_THE_RIGHT.
*
+ * Note: this is deprecated, prefer to use pack() or unpack() in new code.
+ *
* Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
* correct usage, return code may be discarded.
* If op is PACK, pbuf is modified.
@@ -216,4 +218,56 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
}
EXPORT_SYMBOL(packing);
+/**
+ * pack - Pack u64 number into bitfield of buffer.
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @uval: Pointer to an u64 holding the unpacked value.
+ * @startbit: The index (in logical notation, compensated for quirks) where
+ * the packed value starts within pbuf. Must be larger than, or
+ * equal to, endbit.
+ * @endbit: The index (in logical notation, compensated for quirks) where
+ * the packed value ends within pbuf. Must be smaller than, or equal
+ * to, startbit.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ * QUIRK_MSB_ON_THE_RIGHT.
+ *
+ * Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
+ * correct usage, return code may be discarded. The @pbuf memory will
+ * be modified on success.
+ */
+int pack(void *pbuf, const u64 *uval, size_t startbit, size_t endbit,
+ size_t pbuflen, u8 quirks)
+{
+ return packing(pbuf, (u64 *)uval, startbit, endbit, pbuflen, PACK, quirks);
+}
+EXPORT_SYMBOL(pack);
+
+/**
+ * unpack - Unpack u64 number from packed buffer.
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @uval: Pointer to an u64 holding the unpacked value.
+ * @startbit: The index (in logical notation, compensated for quirks) where
+ * the packed value starts within pbuf. Must be larger than, or
+ * equal to, endbit.
+ * @endbit: The index (in logical notation, compensated for quirks) where
+ * the packed value ends within pbuf. Must be smaller than, or equal
+ * to, startbit.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ * QUIRK_MSB_ON_THE_RIGHT.
+ *
+ * Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
+ * correct usage, return code may be discarded. The @uval will be
+ * modified on success.
+ */
+int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
+ size_t pbuflen, u8 quirks)
+{
+ return packing((void *)pbuf, uval, startbit, endbit, pbuflen, UNPACK, quirks);
+}
+EXPORT_SYMBOL(unpack);
+
MODULE_DESCRIPTION("Generic bitfield packing and unpacking");
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 04/10] lib: packing: add pack() and unpack() wrappers over packing()
2024-10-02 21:51 ` [PATCH net-next v2 04/10] lib: packing: add pack() and unpack() wrappers over packing() Jacob Keller
@ 2024-10-03 15:13 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:13 UTC (permalink / raw)
To: Jacob Keller
Cc: Andrew Morton, Vladimir Oltean, David S. Miller, netdev,
Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:53PM -0700, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Geert Uytterhoeven described packing() as "really bad API" because of
> not being able to enforce const correctness. The same function is used
> both when "pbuf" is input and "uval" is output, as in the other way
> around.
>
> Create 2 wrapper functions where const correctness can be ensured.
> Do ugly type casts inside, to be able to reuse packing() as currently
> implemented - which will _not_ modify the input argument.
>
> Also, take the opportunity to change the type of startbit and endbit to
> size_t - an unsigned type - in these new function prototypes. When int,
> an extra check for negative values is necessary. Hopefully, when
> packing() goes away completely, that check can be dropped.
>
> My concern is that code which does rely on the conditional directionality
> of packing() is harder to refactor without blowing up in size. So it may
> take a while to completely eliminate packing(). But let's make alternatives
> available for those who do not need that.
>
> Link: https://lore.kernel.org/netdev/20210223112003.2223332-1-geert+renesas@glider.be/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 05/10] lib: packing: duplicate pack() and unpack() implementations
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (3 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 04/10] lib: packing: add pack() and unpack() wrappers over packing() Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:11 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 06/10] lib: packing: add KUnit tests adapted from selftests Jacob Keller
` (5 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean, Przemek Kitszel
From: Vladimir Oltean <vladimir.oltean@nxp.com>
packing() is now used in some hot paths, and it would be good to get rid
of some ifs and buts that depend on "op", to speed things up a little bit.
With the main implementations now taking size_t endbit, we no longer
have to check for negative values. Update the local integer variables to
also be size_t to match.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
include/linux/packing.h | 4 +-
lib/packing.c | 243 ++++++++++++++++++++++++++++++------------------
2 files changed, 154 insertions(+), 93 deletions(-)
diff --git a/include/linux/packing.h b/include/linux/packing.h
index ea25cb93cc70..5d36dcd06f60 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -20,8 +20,8 @@ enum packing_op {
int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
enum packing_op op, u8 quirks);
-int pack(void *pbuf, const u64 *uval, size_t startbit, size_t endbit,
- size_t pbuflen, u8 quirks);
+int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
+ u8 quirks);
int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
size_t pbuflen, u8 quirks);
diff --git a/lib/packing.c b/lib/packing.c
index 2922db8a528c..500184530d07 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -9,11 +9,11 @@
#include <linux/types.h>
#include <linux/bitrev.h>
-static void adjust_for_msb_right_quirk(u64 *to_write, int *box_start_bit,
- int *box_end_bit, u8 *box_mask)
+static void adjust_for_msb_right_quirk(u64 *to_write, size_t *box_start_bit,
+ size_t *box_end_bit, u8 *box_mask)
{
- int box_bit_width = *box_start_bit - *box_end_bit + 1;
- int new_box_start_bit, new_box_end_bit;
+ size_t box_bit_width = *box_start_bit - *box_end_bit + 1;
+ size_t new_box_start_bit, new_box_end_bit;
*to_write >>= *box_end_bit;
*to_write = bitrev8(*to_write) >> (8 - box_bit_width);
@@ -48,7 +48,7 @@ static void adjust_for_msb_right_quirk(u64 *to_write, int *box_start_bit,
*
* Return: the physical offset into the buffer corresponding to the logical box.
*/
-static int calculate_box_addr(int box, size_t len, u8 quirks)
+static size_t calculate_box_addr(size_t box, size_t len, u8 quirks)
{
size_t offset_of_group, offset_in_group, this_group = box / 4;
size_t group_size;
@@ -69,13 +69,10 @@ static int calculate_box_addr(int box, size_t len, u8 quirks)
}
/**
- * packing - Convert numbers (currently u64) between a packed and an unpacked
- * format. Unpacked means laid out in memory in the CPU's native
- * understanding of integers, while packed means anything else that
- * requires translation.
+ * pack - Pack u64 number into bitfield of buffer.
*
* @pbuf: Pointer to a buffer holding the packed value.
- * @uval: Pointer to an u64 holding the unpacked value.
+ * @uval: CPU-readable unpacked value to pack.
* @startbit: The index (in logical notation, compensated for quirks) where
* the packed value starts within pbuf. Must be larger than, or
* equal to, endbit.
@@ -83,58 +80,45 @@ static int calculate_box_addr(int box, size_t len, u8 quirks)
* the packed value ends within pbuf. Must be smaller than, or equal
* to, startbit.
* @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
- * @op: If PACK, then uval will be treated as const pointer and copied (packed)
- * into pbuf, between startbit and endbit.
- * If UNPACK, then pbuf will be treated as const pointer and the logical
- * value between startbit and endbit will be copied (unpacked) to uval.
* @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
* QUIRK_MSB_ON_THE_RIGHT.
*
- * Note: this is deprecated, prefer to use pack() or unpack() in new code.
- *
* Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
- * correct usage, return code may be discarded.
- * If op is PACK, pbuf is modified.
- * If op is UNPACK, uval is modified.
+ * correct usage, return code may be discarded. The @pbuf memory will
+ * be modified on success.
*/
-int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
- enum packing_op op, u8 quirks)
+int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
+ u8 quirks)
{
- /* Number of bits for storing "uval"
- * also width of the field to access in the pbuf
- */
- u64 value_width;
/* Logical byte indices corresponding to the
* start and end of the field.
*/
int plogical_first_u8, plogical_last_u8, box;
+ /* width of the field to access in the pbuf */
+ u64 value_width;
/* startbit is expected to be larger than endbit, and both are
* expected to be within the logically addressable range of the buffer.
*/
- if (unlikely(startbit < endbit || startbit >= 8 * pbuflen || endbit < 0))
+ if (unlikely(startbit < endbit || startbit >= 8 * pbuflen))
/* Invalid function call */
return -EINVAL;
value_width = startbit - endbit + 1;
- if (value_width > 64)
+ if (unlikely(value_width > 64))
return -ERANGE;
/* Check if "uval" fits in "value_width" bits.
* If value_width is 64, the check will fail, but any
* 64-bit uval will surely fit.
*/
- if (op == PACK && value_width < 64 && (*uval >= (1ull << value_width)))
+ if (unlikely(value_width < 64 && uval >= (1ull << value_width)))
/* Cannot store "uval" inside "value_width" bits.
* Truncating "uval" is most certainly not desirable,
* so simply erroring out is appropriate.
*/
return -ERANGE;
- /* Initialize parameter */
- if (op == UNPACK)
- *uval = 0;
-
/* Iterate through an idealistic view of the pbuf as an u64 with
* no quirks, u8 by u8 (aligned at u8 boundaries), from high to low
* logical bit significance. "box" denotes the current logical u8.
@@ -144,11 +128,12 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
for (box = plogical_first_u8; box >= plogical_last_u8; box--) {
/* Bit indices into the currently accessed 8-bit box */
- int box_start_bit, box_end_bit, box_addr;
+ size_t box_start_bit, box_end_bit, box_addr;
u8 box_mask;
/* Corresponding bits from the unpacked u64 parameter */
- int proj_start_bit, proj_end_bit;
+ size_t proj_start_bit, proj_end_bit;
u64 proj_mask;
+ u64 pval;
/* This u8 may need to be accessed in its entirety
* (from bit 7 to bit 0), or not, depending on the
@@ -182,66 +167,21 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
*/
box_addr = calculate_box_addr(box, pbuflen, quirks);
- if (op == UNPACK) {
- u64 pval;
+ /* Write to pbuf, read from uval */
+ pval = uval & proj_mask;
+ pval >>= proj_end_bit;
+ if (quirks & QUIRK_MSB_ON_THE_RIGHT)
+ adjust_for_msb_right_quirk(&pval,
+ &box_start_bit,
+ &box_end_bit,
+ &box_mask);
- /* Read from pbuf, write to uval */
- pval = ((u8 *)pbuf)[box_addr] & box_mask;
- if (quirks & QUIRK_MSB_ON_THE_RIGHT)
- adjust_for_msb_right_quirk(&pval,
- &box_start_bit,
- &box_end_bit,
- &box_mask);
-
- pval >>= box_end_bit;
- pval <<= proj_end_bit;
- *uval &= ~proj_mask;
- *uval |= pval;
- } else {
- u64 pval;
-
- /* Write to pbuf, read from uval */
- pval = (*uval) & proj_mask;
- pval >>= proj_end_bit;
- if (quirks & QUIRK_MSB_ON_THE_RIGHT)
- adjust_for_msb_right_quirk(&pval,
- &box_start_bit,
- &box_end_bit,
- &box_mask);
-
- pval <<= box_end_bit;
- ((u8 *)pbuf)[box_addr] &= ~box_mask;
- ((u8 *)pbuf)[box_addr] |= pval;
- }
+ pval <<= box_end_bit;
+ ((u8 *)pbuf)[box_addr] &= ~box_mask;
+ ((u8 *)pbuf)[box_addr] |= pval;
}
return 0;
}
-EXPORT_SYMBOL(packing);
-
-/**
- * pack - Pack u64 number into bitfield of buffer.
- *
- * @pbuf: Pointer to a buffer holding the packed value.
- * @uval: Pointer to an u64 holding the unpacked value.
- * @startbit: The index (in logical notation, compensated for quirks) where
- * the packed value starts within pbuf. Must be larger than, or
- * equal to, endbit.
- * @endbit: The index (in logical notation, compensated for quirks) where
- * the packed value ends within pbuf. Must be smaller than, or equal
- * to, startbit.
- * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
- * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
- * QUIRK_MSB_ON_THE_RIGHT.
- *
- * Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
- * correct usage, return code may be discarded. The @pbuf memory will
- * be modified on success.
- */
-int pack(void *pbuf, const u64 *uval, size_t startbit, size_t endbit,
- size_t pbuflen, u8 quirks)
-{
- return packing(pbuf, (u64 *)uval, startbit, endbit, pbuflen, PACK, quirks);
-}
EXPORT_SYMBOL(pack);
/**
@@ -266,8 +206,129 @@ EXPORT_SYMBOL(pack);
int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
size_t pbuflen, u8 quirks)
{
- return packing((void *)pbuf, uval, startbit, endbit, pbuflen, UNPACK, quirks);
+ /* Logical byte indices corresponding to the
+ * start and end of the field.
+ */
+ int plogical_first_u8, plogical_last_u8, box;
+ /* width of the field to access in the pbuf */
+ u64 value_width;
+
+ /* startbit is expected to be larger than endbit, and both are
+ * expected to be within the logically addressable range of the buffer.
+ */
+ if (unlikely(startbit < endbit || startbit >= 8 * pbuflen))
+ /* Invalid function call */
+ return -EINVAL;
+
+ value_width = startbit - endbit + 1;
+ if (unlikely(value_width > 64))
+ return -ERANGE;
+
+ /* Initialize parameter */
+ *uval = 0;
+
+ /* Iterate through an idealistic view of the pbuf as an u64 with
+ * no quirks, u8 by u8 (aligned at u8 boundaries), from high to low
+ * logical bit significance. "box" denotes the current logical u8.
+ */
+ plogical_first_u8 = startbit / 8;
+ plogical_last_u8 = endbit / 8;
+
+ for (box = plogical_first_u8; box >= plogical_last_u8; box--) {
+ /* Bit indices into the currently accessed 8-bit box */
+ size_t box_start_bit, box_end_bit, box_addr;
+ u8 box_mask;
+ /* Corresponding bits from the unpacked u64 parameter */
+ size_t proj_start_bit, proj_end_bit;
+ u64 proj_mask;
+ u64 pval;
+
+ /* This u8 may need to be accessed in its entirety
+ * (from bit 7 to bit 0), or not, depending on the
+ * input arguments startbit and endbit.
+ */
+ if (box == plogical_first_u8)
+ box_start_bit = startbit % 8;
+ else
+ box_start_bit = 7;
+ if (box == plogical_last_u8)
+ box_end_bit = endbit % 8;
+ else
+ box_end_bit = 0;
+
+ /* We have determined the box bit start and end.
+ * Now we calculate where this (masked) u8 box would fit
+ * in the unpacked (CPU-readable) u64 - the u8 box's
+ * projection onto the unpacked u64. Though the
+ * box is u8, the projection is u64 because it may fall
+ * anywhere within the unpacked u64.
+ */
+ proj_start_bit = ((box * 8) + box_start_bit) - endbit;
+ proj_end_bit = ((box * 8) + box_end_bit) - endbit;
+ proj_mask = GENMASK_ULL(proj_start_bit, proj_end_bit);
+ box_mask = GENMASK_ULL(box_start_bit, box_end_bit);
+
+ /* Determine the offset of the u8 box inside the pbuf,
+ * adjusted for quirks. The adjusted box_addr will be used for
+ * effective addressing inside the pbuf (so it's not
+ * logical any longer).
+ */
+ box_addr = calculate_box_addr(box, pbuflen, quirks);
+
+ /* Read from pbuf, write to uval */
+ pval = ((u8 *)pbuf)[box_addr] & box_mask;
+ if (quirks & QUIRK_MSB_ON_THE_RIGHT)
+ adjust_for_msb_right_quirk(&pval,
+ &box_start_bit,
+ &box_end_bit,
+ &box_mask);
+
+ pval >>= box_end_bit;
+ pval <<= proj_end_bit;
+ *uval &= ~proj_mask;
+ *uval |= pval;
+ }
+ return 0;
}
EXPORT_SYMBOL(unpack);
+/**
+ * packing - Convert numbers (currently u64) between a packed and an unpacked
+ * format. Unpacked means laid out in memory in the CPU's native
+ * understanding of integers, while packed means anything else that
+ * requires translation.
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @uval: Pointer to an u64 holding the unpacked value.
+ * @startbit: The index (in logical notation, compensated for quirks) where
+ * the packed value starts within pbuf. Must be larger than, or
+ * equal to, endbit.
+ * @endbit: The index (in logical notation, compensated for quirks) where
+ * the packed value ends within pbuf. Must be smaller than, or equal
+ * to, startbit.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @op: If PACK, then uval will be treated as const pointer and copied (packed)
+ * into pbuf, between startbit and endbit.
+ * If UNPACK, then pbuf will be treated as const pointer and the logical
+ * value between startbit and endbit will be copied (unpacked) to uval.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ * QUIRK_MSB_ON_THE_RIGHT.
+ *
+ * Note: this is deprecated, prefer to use pack() or unpack() in new code.
+ *
+ * Return: 0 on success, EINVAL or ERANGE if called incorrectly. Assuming
+ * correct usage, return code may be discarded.
+ * If op is PACK, pbuf is modified.
+ * If op is UNPACK, uval is modified.
+ */
+int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
+ enum packing_op op, u8 quirks)
+{
+ if (op == PACK)
+ return pack(pbuf, *uval, startbit, endbit, pbuflen, quirks);
+
+ return unpack(pbuf, uval, startbit, endbit, pbuflen, quirks);
+}
+EXPORT_SYMBOL(packing);
+
MODULE_DESCRIPTION("Generic bitfield packing and unpacking");
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 05/10] lib: packing: duplicate pack() and unpack() implementations
2024-10-02 21:51 ` [PATCH net-next v2 05/10] lib: packing: duplicate pack() and unpack() implementations Jacob Keller
@ 2024-10-03 15:11 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:11 UTC (permalink / raw)
To: Jacob Keller
Cc: Andrew Morton, Vladimir Oltean, David S. Miller, netdev,
Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:54PM -0700, Jacob Keller wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> packing() is now used in some hot paths, and it would be good to get rid
> of some ifs and buts that depend on "op", to speed things up a little bit.
>
> With the main implementations now taking size_t endbit, we no longer
> have to check for negative values. Update the local integer variables to
> also be size_t to match.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 06/10] lib: packing: add KUnit tests adapted from selftests
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (4 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 05/10] lib: packing: duplicate pack() and unpack() implementations Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:18 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 07/10] lib: packing: add additional KUnit tests Jacob Keller
` (4 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean, Przemek Kitszel
Add 24 simple KUnit tests for the lib/packing.c pack() and unpack() APIs.
The first 16 tests exercise all combinations of quirks with a simple magic
number value on a 16-byte buffer. The remaining 8 tests cover
non-multiple-of-4 buffer sizes.
These tests were originally written by Vladimir as simple selftest
functions. I adapted them to KUnit, refactoring them into a table driven
approach. This will aid in adding additional tests in the future.
Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
lib/packing_test.c | 258 +++++++++++++++++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 1 +
lib/Kconfig | 12 +++
lib/Makefile | 1 +
4 files changed, 272 insertions(+)
diff --git a/lib/packing_test.c b/lib/packing_test.c
new file mode 100644
index 000000000000..4d07523dba29
--- /dev/null
+++ b/lib/packing_test.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024, Vladimir Oltean <olteanv@gmail.com>
+ * Copyright (c) 2024, Intel Corporation.
+ */
+#include <kunit/test.h>
+#include <linux/packing.h>
+
+struct packing_test_case {
+ const char *desc;
+ const u8 *pbuf;
+ size_t pbuf_size;
+ u64 uval;
+ size_t start_bit;
+ size_t end_bit;
+ u8 quirks;
+};
+
+#define NO_QUIRKS 0
+
+/**
+ * PBUF - Initialize .pbuf and .pbuf_size
+ * @array: elements of constant physical buffer
+ *
+ * Initializes the .pbuf and .pbuf_size fields of a struct packing_test_case
+ * with a constant array of the specified elements.
+ */
+#define PBUF(array...) \
+ .pbuf = (const u8[]){ array }, \
+ .pbuf_size = sizeof((const u8 []){ array })
+
+static const struct packing_test_case cases[] = {
+ /* These tests pack and unpack a magic 64-bit value
+ * (0xcafedeadbeefcafe) at a fixed logical offset (32) within an
+ * otherwise zero array of 128 bits (16 bytes). They test all possible
+ * bit layouts of the 128 bit buffer.
+ */
+ {
+ .desc = "no quirks, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xca, 0xfe, 0xde, 0xad,
+ 0xbe, 0xef, 0xca, 0xfe, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "lsw32 first, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xbe, 0xef, 0xca, 0xfe,
+ 0xca, 0xfe, 0xde, 0xad, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST,
+ },
+ {
+ .desc = "little endian words, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xad, 0xde, 0xfe, 0xca,
+ 0xfe, 0xca, 0xef, 0xbe, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xfe, 0xca, 0xef, 0xbe,
+ 0xad, 0xde, 0xfe, 0xca, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "msb right, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x53, 0x7f, 0x7b, 0xb5,
+ 0x7d, 0xf7, 0x53, 0x7f, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT,
+ },
+ {
+ .desc = "msb right + lsw32 first, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x7d, 0xf7, 0x53, 0x7f,
+ 0x53, 0x7f, 0x7b, 0xb5, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LSW32_IS_FIRST,
+ },
+ {
+ .desc = "msb right + little endian words, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xb5, 0x7b, 0x7f, 0x53,
+ 0x7f, 0x53, 0xf7, 0x7d, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "msb right + lsw32 first + little endian words, 16 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x7f, 0x53, 0xf7, 0x7d,
+ 0xb5, 0x7b, 0x7f, 0x53, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ /* These tests pack and unpack a magic 64-bit value
+ * (0xcafedeadbeefcafe) at a fixed logical offset (32) within an
+ * otherwise zero array of varying size from 18 bytes to 24 bytes.
+ */
+ {
+ .desc = "no quirks, 18 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xca, 0xfe,
+ 0xde, 0xad, 0xbe, 0xef, 0xca, 0xfe, 0x00, 0x00,
+ 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "no quirks, 19 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xca,
+ 0xfe, 0xde, 0xad, 0xbe, 0xef, 0xca, 0xfe, 0x00,
+ 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "no quirks, 20 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0xca, 0xfe, 0xde, 0xad, 0xbe, 0xef, 0xca, 0xfe,
+ 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "no quirks, 22 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0xca, 0xfe, 0xde, 0xad, 0xbe, 0xef,
+ 0xca, 0xfe, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "no quirks, 24 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0xca, 0xfe, 0xde, 0xad,
+ 0xbe, 0xef, 0xca, 0xfe, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 18 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xfe, 0xca, 0xef, 0xbe,
+ 0xad, 0xde, 0xfe, 0xca, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 19 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xfe, 0xca, 0xef, 0xbe,
+ 0xad, 0xde, 0xfe, 0xca, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 20 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xfe, 0xca, 0xef, 0xbe,
+ 0xad, 0xde, 0xfe, 0xca, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 22 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xfe, 0xca, 0xef, 0xbe,
+ 0xad, 0xde, 0xfe, 0xca, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 24 bytes",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xfe, 0xca, 0xef, 0xbe,
+ 0xad, 0xde, 0xfe, 0xca, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xcafedeadbeefcafe,
+ .start_bit = 95,
+ .end_bit = 32,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+};
+
+KUNIT_ARRAY_PARAM_DESC(packing, cases, desc);
+
+static void packing_test_pack(struct kunit *test)
+{
+ const struct packing_test_case *params = test->param_value;
+ u8 *pbuf;
+ int err;
+
+ pbuf = kunit_kzalloc(test, params->pbuf_size, GFP_KERNEL);
+
+ err = pack(pbuf, params->uval, params->start_bit, params->end_bit,
+ params->pbuf_size, params->quirks);
+
+ KUNIT_EXPECT_EQ_MSG(test, err, 0, "pack() returned %pe\n", ERR_PTR(err));
+ KUNIT_EXPECT_MEMEQ(test, pbuf, params->pbuf, params->pbuf_size);
+}
+
+static void packing_test_unpack(struct kunit *test)
+{
+ const struct packing_test_case *params = test->param_value;
+ u64 uval;
+ int err;
+
+ err = unpack(params->pbuf, &uval, params->start_bit, params->end_bit,
+ params->pbuf_size, params->quirks);
+ KUNIT_EXPECT_EQ_MSG(test, err, 0, "unpack() returned %pe\n", ERR_PTR(err));
+ KUNIT_EXPECT_EQ(test, uval, params->uval);
+}
+
+static struct kunit_case packing_test_cases[] = {
+ KUNIT_CASE_PARAM(packing_test_pack, packing_gen_params),
+ KUNIT_CASE_PARAM(packing_test_unpack, packing_gen_params),
+ {},
+};
+
+static struct kunit_suite packing_test_suite = {
+ .name = "packing",
+ .test_cases = packing_test_cases,
+};
+
+kunit_test_suite(packing_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("KUnit tests for packing library");
diff --git a/MAINTAINERS b/MAINTAINERS
index e71d066dc919..fc26816d8b7b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17468,6 +17468,7 @@ S: Supported
F: Documentation/core-api/packing.rst
F: include/linux/packing.h
F: lib/packing.c
+F: lib/packing_test.c
PADATA PARALLEL EXECUTION MECHANISM
M: Steffen Klassert <steffen.klassert@secunet.com>
diff --git a/lib/Kconfig b/lib/Kconfig
index b38849af6f13..50d85f38b569 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -40,6 +40,18 @@ config PACKING
When in doubt, say N.
+config PACKING_KUNIT_TEST
+ tristate "KUnit tests for packing library" if !KUNIT_ALL_TESTS
+ depends on PACKING && KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds KUnit tests for the packing library.
+
+ For more information on KUnit and unit tests in general,
+ please refer to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ When in doubt, say N.
+
config BITREVERSE
tristate
diff --git a/lib/Makefile b/lib/Makefile
index 773adf88af41..811ba12c8cd0 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -154,6 +154,7 @@ obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
obj-$(CONFIG_BITREVERSE) += bitrev.o
obj-$(CONFIG_LINEAR_RANGES) += linear_ranges.o
obj-$(CONFIG_PACKING) += packing.o
+obj-$(CONFIG_PACKING_KUNIT_TEST) += packing_test.o
obj-$(CONFIG_CRC_CCITT) += crc-ccitt.o
obj-$(CONFIG_CRC16) += crc16.o
obj-$(CONFIG_CRC_T10DIF)+= crc-t10dif.o
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 06/10] lib: packing: add KUnit tests adapted from selftests
2024-10-02 21:51 ` [PATCH net-next v2 06/10] lib: packing: add KUnit tests adapted from selftests Jacob Keller
@ 2024-10-03 15:18 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:18 UTC (permalink / raw)
To: Jacob Keller
Cc: Andrew Morton, Vladimir Oltean, David S. Miller, netdev,
Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:55PM -0700, Jacob Keller wrote:
> Add 24 simple KUnit tests for the lib/packing.c pack() and unpack() APIs.
>
> The first 16 tests exercise all combinations of quirks with a simple magic
> number value on a 16-byte buffer. The remaining 8 tests cover
> non-multiple-of-4 buffer sizes.
>
> These tests were originally written by Vladimir as simple selftest
> functions. I adapted them to KUnit, refactoring them into a table driven
> approach. This will aid in adding additional tests in the future.
>
> Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 07/10] lib: packing: add additional KUnit tests
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (5 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 06/10] lib: packing: add KUnit tests adapted from selftests Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:21 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 08/10] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior Jacob Keller
` (3 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Przemek Kitszel
While reviewing the initial KUnit tests for lib/packing, Przemek pointed
out that the test values have duplicate bytes in the input sequence.
In addition, I noticed that the unit tests pack and unpack on a byte
boundary, instead of crossing bytes. Thus, we lack good coverage of the
corner cases of the API.
Add additional unit tests to cover packing and unpacking byte buffers which
do not have duplicate bytes in the unpacked value, and which pack and
unpack to an unaligned offset.
A careful reviewer may note the lack tests for QUIRK_MSB_ON_THE_RIGHT. This
is because I found issues with that quirk during test implementation. This
quirk will be fixed and the tests will be included in a future change.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
lib/packing_test.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/lib/packing_test.c b/lib/packing_test.c
index 4d07523dba29..5d533e633e06 100644
--- a/lib/packing_test.c
+++ b/lib/packing_test.c
@@ -210,6 +210,88 @@ static const struct packing_test_case cases[] = {
.end_bit = 32,
.quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
},
+ /* These tests pack and unpack a magic 64-bit value
+ * (0x1122334455667788) at an odd starting bit (43) within an
+ * otherwise zero array of 128 bits (16 bytes). They test all possible
+ * bit layouts of the 128 bit buffer.
+ */
+ {
+ .desc = "no quirks, 16 bytes, non-aligned",
+ PBUF(0x00, 0x00, 0x00, 0x89, 0x11, 0x9a, 0x22, 0xab,
+ 0x33, 0xbc, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "lsw32 first, 16 bytes, non-aligned",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x33, 0xbc, 0x40, 0x00,
+ 0x11, 0x9a, 0x22, 0xab, 0x00, 0x00, 0x00, 0x89),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_LSW32_IS_FIRST,
+ },
+ {
+ .desc = "little endian words, 16 bytes, non-aligned",
+ PBUF(0x89, 0x00, 0x00, 0x00, 0xab, 0x22, 0x9a, 0x11,
+ 0x00, 0x40, 0xbc, 0x33, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 16 bytes, non-aligned",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0xbc, 0x33,
+ 0xab, 0x22, 0x9a, 0x11, 0x89, 0x00, 0x00, 0x00),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
+ /* These tests pack and unpack a u64 with all bits set
+ * (0xffffffffffffffff) at an odd starting bit (43) within an
+ * otherwise zero array of 128 bits (16 bytes). They test all possible
+ * bit layouts of the 128 bit buffer.
+ */
+ {
+ .desc = "no quirks, 16 bytes, non-aligned, 0xff",
+ PBUF(0x00, 0x00, 0x07, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = NO_QUIRKS,
+ },
+ {
+ .desc = "lsw32 first, 16 bytes, non-aligned, 0xff",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xf8, 0x00,
+ 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x07, 0xff),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_LSW32_IS_FIRST,
+ },
+ {
+ .desc = "little endian words, 16 bytes, non-aligned, 0xff",
+ PBUF(0xff, 0x07, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
+ 0x00, 0xf8, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "lsw32 first + little endian words, 16 bytes, non-aligned, 0xff",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0xf8, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0x07, 0x00, 0x00),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
};
KUNIT_ARRAY_PARAM_DESC(packing, cases, desc);
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 07/10] lib: packing: add additional KUnit tests
2024-10-02 21:51 ` [PATCH net-next v2 07/10] lib: packing: add additional KUnit tests Jacob Keller
@ 2024-10-03 15:21 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:21 UTC (permalink / raw)
To: Jacob Keller; +Cc: Andrew Morton, David S. Miller, netdev, Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:56PM -0700, Jacob Keller wrote:
> While reviewing the initial KUnit tests for lib/packing, Przemek pointed
> out that the test values have duplicate bytes in the input sequence.
>
> In addition, I noticed that the unit tests pack and unpack on a byte
> boundary, instead of crossing bytes. Thus, we lack good coverage of the
> corner cases of the API.
>
> Add additional unit tests to cover packing and unpacking byte buffers which
> do not have duplicate bytes in the unpacked value, and which pack and
> unpack to an unaligned offset.
>
> A careful reviewer may note the lack tests for QUIRK_MSB_ON_THE_RIGHT. This
> is because I found issues with that quirk during test implementation. This
> quirk will be fixed and the tests will be included in a future change.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 08/10] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (6 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 07/10] lib: packing: add additional KUnit tests Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:22 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 09/10] lib: packing: use BITS_PER_BYTE instead of 8 Jacob Keller
` (2 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Przemek Kitszel
The QUIRK_MSB_ON_THE_RIGHT quirk is intended to modify pack() and unpack()
so that the most significant bit of each byte in the packed layout is on
the right.
The way the quirk is currently implemented is broken whenever the packing
code packs or unpacks any value that is not exactly a full byte.
The broken behavior can occur when packing any values smaller than one
byte, when packing any value that is not exactly a whole number of bytes,
or when the packing is not aligned to a byte boundary.
This quirk is documented in the following way:
1. Normally (no quirks), we would do it like this:
::
63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
7 6 5 4
31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
3 2 1 0
<snip>
2. If QUIRK_MSB_ON_THE_RIGHT is set, we do it like this:
::
56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
7 6 5 4
24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
3 2 1 0
That is, QUIRK_MSB_ON_THE_RIGHT does not affect byte positioning, but
inverts bit offsets inside a byte.
Essentially, the mapping for physical bit offsets should be reserved for a
given byte within the payload. This reversal should be fixed to the bytes
in the packing layout.
The logic to implement this quirk is handled within the
adjust_for_msb_right_quirk() function. This function does not work properly
when dealing with the bytes that contain only a partial amount of data.
In particular, consider trying to pack or unpack the range 53-44. We should
always be mapping the bits from the logical ordering to their physical
ordering in the same way, regardless of what sequence of bits we are
unpacking.
This, we should grab the following logical bits:
Logical: 55 54 53 52 51 50 49 48 47 45 44 43 42 41 40 39
^ ^ ^ ^ ^ ^ ^ ^ ^
And pack them into the physical bits:
Physical: 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47
Logical: 48 49 50 51 52 53 44 45 46 47
^ ^ ^ ^ ^ ^ ^ ^ ^ ^
The current logic in adjust_for_msb_right_quirk is broken. I believe it is
intending to map according to the following:
Physical: 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47
Logical: 48 49 50 51 52 53 44 45 46 47
^ ^ ^ ^ ^ ^ ^ ^ ^ ^
That is, it tries to keep the bits at the start and end of a packing
together. This is wrong, as it makes the packing change what bit is being
mapped to what based on which bits you're currently packing or unpacking.
Worse, the actual calculations within adjust_for_msb_right_quirk don't make
sense.
Consider the case when packing the last byte of an unaligned packing. It
might have a start bit of 7 and an end bit of 5. This would have a width of
3 bits. The new_start_bit will be calculated as the width - the box_end_bit
- 1. This will underflow and produce a negative value, which will
ultimate result in generating a new box_mask of all 0s.
For any other values, the result of the calculations of the
new_box_end_bit, new_box_start_bit, and the new box_mask will result in the
exact same values for the box_end_bit, box_start_bit, and box_mask. This
makes the calculations completely irrelevant.
If box_end_bit is 0, and box_start_bit is 7, then the entire function of
adjust_for_msb_right_quirk will boil down to just:
*to_write = bitrev8(*to_write)
The other adjustments are attempting (incorrectly) to keep the bits in the
same place but just reversed. This is not the right behavior even if
implemented correctly, as it leaves the mapping dependent on the bit values
being packed or unpacked.
Remove adjust_for_msb_right_quirk() and just use bitrev8 to reverse the
byte order when interacting with the packed data.
In particular, for packing, we need to reverse both the box_mask and the
physical value being packed. This is done after shifting the value by
box_end_bit so that the reversed mapping is always aligned to the physical
buffer byte boundary. The box_mask is reversed as we're about to use it to
clear any stale bits in the physical buffer at this block.
For unpacking, we need to reverse the contents of the physical buffer
*before* masking with the box_mask. This is critical, as the box_mask is a
logical mask of the bit layout before handling the QUIRK_MSB_ON_THE_RIGHT.
Add several new tests which cover this behavior. These tests will fail
without the fix and pass afterwards. Note that no current drivers make use
of QUIRK_MSB_ON_THE_RIGHT. I suspect this is why there have been no reports
of this inconsistency before.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
lib/packing.c | 39 +++++++++--------------------
lib/packing_test.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 28 deletions(-)
diff --git a/lib/packing.c b/lib/packing.c
index 500184530d07..9efe57d347c7 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -9,23 +9,6 @@
#include <linux/types.h>
#include <linux/bitrev.h>
-static void adjust_for_msb_right_quirk(u64 *to_write, size_t *box_start_bit,
- size_t *box_end_bit, u8 *box_mask)
-{
- size_t box_bit_width = *box_start_bit - *box_end_bit + 1;
- size_t new_box_start_bit, new_box_end_bit;
-
- *to_write >>= *box_end_bit;
- *to_write = bitrev8(*to_write) >> (8 - box_bit_width);
- *to_write <<= *box_end_bit;
-
- new_box_end_bit = box_bit_width - *box_start_bit - 1;
- new_box_start_bit = box_bit_width - *box_end_bit - 1;
- *box_mask = GENMASK_ULL(new_box_start_bit, new_box_end_bit);
- *box_start_bit = new_box_start_bit;
- *box_end_bit = new_box_end_bit;
-}
-
/**
* calculate_box_addr - Determine physical location of byte in buffer
* @box: Index of byte within buffer seen as a logical big-endian big number
@@ -170,13 +153,13 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
/* Write to pbuf, read from uval */
pval = uval & proj_mask;
pval >>= proj_end_bit;
- if (quirks & QUIRK_MSB_ON_THE_RIGHT)
- adjust_for_msb_right_quirk(&pval,
- &box_start_bit,
- &box_end_bit,
- &box_mask);
-
pval <<= box_end_bit;
+
+ if (quirks & QUIRK_MSB_ON_THE_RIGHT) {
+ pval = bitrev8(pval);
+ box_mask = bitrev8(box_mask);
+ }
+
((u8 *)pbuf)[box_addr] &= ~box_mask;
((u8 *)pbuf)[box_addr] |= pval;
}
@@ -276,12 +259,12 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
box_addr = calculate_box_addr(box, pbuflen, quirks);
/* Read from pbuf, write to uval */
- pval = ((u8 *)pbuf)[box_addr] & box_mask;
+ pval = ((u8 *)pbuf)[box_addr];
+
if (quirks & QUIRK_MSB_ON_THE_RIGHT)
- adjust_for_msb_right_quirk(&pval,
- &box_start_bit,
- &box_end_bit,
- &box_mask);
+ pval = bitrev8(pval);
+
+ pval &= box_mask;
pval >>= box_end_bit;
pval <<= proj_end_bit;
diff --git a/lib/packing_test.c b/lib/packing_test.c
index 5d533e633e06..015ad1180d23 100644
--- a/lib/packing_test.c
+++ b/lib/packing_test.c
@@ -251,6 +251,42 @@ static const struct packing_test_case cases[] = {
.end_bit = 43,
.quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
},
+ {
+ .desc = "msb right, 16 bytes, non-aligned",
+ PBUF(0x00, 0x00, 0x00, 0x91, 0x88, 0x59, 0x44, 0xd5,
+ 0xcc, 0x3d, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT,
+ },
+ {
+ .desc = "msb right + lsw32 first, 16 bytes, non-aligned",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xcc, 0x3d, 0x02, 0x00,
+ 0x88, 0x59, 0x44, 0xd5, 0x00, 0x00, 0x00, 0x91),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LSW32_IS_FIRST,
+ },
+ {
+ .desc = "msb right + little endian words, 16 bytes, non-aligned",
+ PBUF(0x91, 0x00, 0x00, 0x00, 0xd5, 0x44, 0x59, 0x88,
+ 0x00, 0x02, 0x3d, 0xcc, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "msb right + lsw32 first + little endian words, 16 bytes, non-aligned",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x3d, 0xcc,
+ 0xd5, 0x44, 0x59, 0x88, 0x91, 0x00, 0x00, 0x00),
+ .uval = 0x1122334455667788,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
/* These tests pack and unpack a u64 with all bits set
* (0xffffffffffffffff) at an odd starting bit (43) within an
* otherwise zero array of 128 bits (16 bytes). They test all possible
@@ -292,6 +328,42 @@ static const struct packing_test_case cases[] = {
.end_bit = 43,
.quirks = QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
},
+ {
+ .desc = "msb right, 16 bytes, non-aligned, 0xff",
+ PBUF(0x00, 0x00, 0xe0, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0x1f, 0x00, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT,
+ },
+ {
+ .desc = "msb right + lsw32 first, 16 bytes, non-aligned, 0xff",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x1f, 0x00,
+ 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0xe0, 0xff),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LSW32_IS_FIRST,
+ },
+ {
+ .desc = "msb right + little endian words, 16 bytes, non-aligned, 0xff",
+ PBUF(0xff, 0xe0, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
+ 0x00, 0x1f, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LITTLE_ENDIAN,
+ },
+ {
+ .desc = "msb right + lsw32 first + little endian words, 16 bytes, non-aligned, 0xff",
+ PBUF(0x00, 0x00, 0x00, 0x00, 0x00, 0x1f, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xe0, 0x00, 0x00),
+ .uval = 0xffffffffffffffff,
+ .start_bit = 106,
+ .end_bit = 43,
+ .quirks = QUIRK_MSB_ON_THE_RIGHT | QUIRK_LSW32_IS_FIRST | QUIRK_LITTLE_ENDIAN,
+ },
};
KUNIT_ARRAY_PARAM_DESC(packing, cases, desc);
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 08/10] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior
2024-10-02 21:51 ` [PATCH net-next v2 08/10] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior Jacob Keller
@ 2024-10-03 15:22 ` Vladimir Oltean
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Oltean @ 2024-10-03 15:22 UTC (permalink / raw)
To: Jacob Keller; +Cc: Andrew Morton, David S. Miller, netdev, Przemek Kitszel
On Wed, Oct 02, 2024 at 02:51:57PM -0700, Jacob Keller wrote:
> The QUIRK_MSB_ON_THE_RIGHT quirk is intended to modify pack() and unpack()
> so that the most significant bit of each byte in the packed layout is on
> the right.
>
> The way the quirk is currently implemented is broken whenever the packing
> code packs or unpacks any value that is not exactly a full byte.
>
> The broken behavior can occur when packing any values smaller than one
> byte, when packing any value that is not exactly a whole number of bytes,
> or when the packing is not aligned to a byte boundary.
>
> This quirk is documented in the following way:
>
> 1. Normally (no quirks), we would do it like this:
>
> ::
>
> 63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
> 7 6 5 4
> 31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
> 3 2 1 0
>
> <snip>
>
> 2. If QUIRK_MSB_ON_THE_RIGHT is set, we do it like this:
>
> ::
>
> 56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
> 7 6 5 4
> 24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
> 3 2 1 0
>
> That is, QUIRK_MSB_ON_THE_RIGHT does not affect byte positioning, but
> inverts bit offsets inside a byte.
>
> Essentially, the mapping for physical bit offsets should be reserved for a
> given byte within the payload. This reversal should be fixed to the bytes
> in the packing layout.
>
> The logic to implement this quirk is handled within the
> adjust_for_msb_right_quirk() function. This function does not work properly
> when dealing with the bytes that contain only a partial amount of data.
>
> In particular, consider trying to pack or unpack the range 53-44. We should
> always be mapping the bits from the logical ordering to their physical
> ordering in the same way, regardless of what sequence of bits we are
> unpacking.
>
> This, we should grab the following logical bits:
>
> Logical: 55 54 53 52 51 50 49 48 47 45 44 43 42 41 40 39
> ^ ^ ^ ^ ^ ^ ^ ^ ^
>
> And pack them into the physical bits:
>
> Physical: 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47
> Logical: 48 49 50 51 52 53 44 45 46 47
> ^ ^ ^ ^ ^ ^ ^ ^ ^ ^
>
> The current logic in adjust_for_msb_right_quirk is broken. I believe it is
> intending to map according to the following:
>
> Physical: 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47
> Logical: 48 49 50 51 52 53 44 45 46 47
> ^ ^ ^ ^ ^ ^ ^ ^ ^ ^
>
> That is, it tries to keep the bits at the start and end of a packing
> together. This is wrong, as it makes the packing change what bit is being
> mapped to what based on which bits you're currently packing or unpacking.
>
> Worse, the actual calculations within adjust_for_msb_right_quirk don't make
> sense.
>
> Consider the case when packing the last byte of an unaligned packing. It
> might have a start bit of 7 and an end bit of 5. This would have a width of
> 3 bits. The new_start_bit will be calculated as the width - the box_end_bit
> - 1. This will underflow and produce a negative value, which will
> ultimate result in generating a new box_mask of all 0s.
>
> For any other values, the result of the calculations of the
> new_box_end_bit, new_box_start_bit, and the new box_mask will result in the
> exact same values for the box_end_bit, box_start_bit, and box_mask. This
> makes the calculations completely irrelevant.
>
> If box_end_bit is 0, and box_start_bit is 7, then the entire function of
> adjust_for_msb_right_quirk will boil down to just:
>
> *to_write = bitrev8(*to_write)
>
> The other adjustments are attempting (incorrectly) to keep the bits in the
> same place but just reversed. This is not the right behavior even if
> implemented correctly, as it leaves the mapping dependent on the bit values
> being packed or unpacked.
>
> Remove adjust_for_msb_right_quirk() and just use bitrev8 to reverse the
> byte order when interacting with the packed data.
>
> In particular, for packing, we need to reverse both the box_mask and the
> physical value being packed. This is done after shifting the value by
> box_end_bit so that the reversed mapping is always aligned to the physical
> buffer byte boundary. The box_mask is reversed as we're about to use it to
> clear any stale bits in the physical buffer at this block.
>
> For unpacking, we need to reverse the contents of the physical buffer
> *before* masking with the box_mask. This is critical, as the box_mask is a
> logical mask of the bit layout before handling the QUIRK_MSB_ON_THE_RIGHT.
>
> Add several new tests which cover this behavior. These tests will fail
> without the fix and pass afterwards. Note that no current drivers make use
> of QUIRK_MSB_ON_THE_RIGHT. I suspect this is why there have been no reports
> of this inconsistency before.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v2 09/10] lib: packing: use BITS_PER_BYTE instead of 8
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (7 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 08/10] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 15:23 ` Vladimir Oltean
2024-10-02 21:51 ` [PATCH net-next v2 10/10] lib: packing: use GENMASK() for box_mask Jacob Keller
2024-10-03 23:10 ` [PATCH net-next v2 00/10] packing: various improvements and KUnit tests patchwork-bot+netdevbpf
10 siblings, 1 reply; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
This helps clarify what the 8 is for.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
lib/packing.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/lib/packing.c b/lib/packing.c
index 9efe57d347c7..ac1f36c56a77 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -83,7 +83,7 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
/* startbit is expected to be larger than endbit, and both are
* expected to be within the logically addressable range of the buffer.
*/
- if (unlikely(startbit < endbit || startbit >= 8 * pbuflen))
+ if (unlikely(startbit < endbit || startbit >= BITS_PER_BYTE * pbuflen))
/* Invalid function call */
return -EINVAL;
@@ -106,8 +106,8 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
* no quirks, u8 by u8 (aligned at u8 boundaries), from high to low
* logical bit significance. "box" denotes the current logical u8.
*/
- plogical_first_u8 = startbit / 8;
- plogical_last_u8 = endbit / 8;
+ plogical_first_u8 = startbit / BITS_PER_BYTE;
+ plogical_last_u8 = endbit / BITS_PER_BYTE;
for (box = plogical_first_u8; box >= plogical_last_u8; box--) {
/* Bit indices into the currently accessed 8-bit box */
@@ -123,11 +123,11 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
* input arguments startbit and endbit.
*/
if (box == plogical_first_u8)
- box_start_bit = startbit % 8;
+ box_start_bit = startbit % BITS_PER_BYTE;
else
box_start_bit = 7;
if (box == plogical_last_u8)
- box_end_bit = endbit % 8;
+ box_end_bit = endbit % BITS_PER_BYTE;
else
box_end_bit = 0;
@@ -138,8 +138,8 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
* box is u8, the projection is u64 because it may fall
* anywhere within the unpacked u64.
*/
- proj_start_bit = ((box * 8) + box_start_bit) - endbit;
- proj_end_bit = ((box * 8) + box_end_bit) - endbit;
+ proj_start_bit = ((box * BITS_PER_BYTE) + box_start_bit) - endbit;
+ proj_end_bit = ((box * BITS_PER_BYTE) + box_end_bit) - endbit;
proj_mask = GENMASK_ULL(proj_start_bit, proj_end_bit);
box_mask = GENMASK_ULL(box_start_bit, box_end_bit);
@@ -199,7 +199,7 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
/* startbit is expected to be larger than endbit, and both are
* expected to be within the logically addressable range of the buffer.
*/
- if (unlikely(startbit < endbit || startbit >= 8 * pbuflen))
+ if (unlikely(startbit < endbit || startbit >= BITS_PER_BYTE * pbuflen))
/* Invalid function call */
return -EINVAL;
@@ -214,8 +214,8 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
* no quirks, u8 by u8 (aligned at u8 boundaries), from high to low
* logical bit significance. "box" denotes the current logical u8.
*/
- plogical_first_u8 = startbit / 8;
- plogical_last_u8 = endbit / 8;
+ plogical_first_u8 = startbit / BITS_PER_BYTE;
+ plogical_last_u8 = endbit / BITS_PER_BYTE;
for (box = plogical_first_u8; box >= plogical_last_u8; box--) {
/* Bit indices into the currently accessed 8-bit box */
@@ -231,11 +231,11 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
* input arguments startbit and endbit.
*/
if (box == plogical_first_u8)
- box_start_bit = startbit % 8;
+ box_start_bit = startbit % BITS_PER_BYTE;
else
box_start_bit = 7;
if (box == plogical_last_u8)
- box_end_bit = endbit % 8;
+ box_end_bit = endbit % BITS_PER_BYTE;
else
box_end_bit = 0;
@@ -246,8 +246,8 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
* box is u8, the projection is u64 because it may fall
* anywhere within the unpacked u64.
*/
- proj_start_bit = ((box * 8) + box_start_bit) - endbit;
- proj_end_bit = ((box * 8) + box_end_bit) - endbit;
+ proj_start_bit = ((box * BITS_PER_BYTE) + box_start_bit) - endbit;
+ proj_end_bit = ((box * BITS_PER_BYTE) + box_end_bit) - endbit;
proj_mask = GENMASK_ULL(proj_start_bit, proj_end_bit);
box_mask = GENMASK_ULL(box_start_bit, box_end_bit);
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v2 10/10] lib: packing: use GENMASK() for box_mask
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (8 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 09/10] lib: packing: use BITS_PER_BYTE instead of 8 Jacob Keller
@ 2024-10-02 21:51 ` Jacob Keller
2024-10-03 23:10 ` [PATCH net-next v2 00/10] packing: various improvements and KUnit tests patchwork-bot+netdevbpf
10 siblings, 0 replies; 21+ messages in thread
From: Jacob Keller @ 2024-10-02 21:51 UTC (permalink / raw)
To: Andrew Morton, Vladimir Oltean, David S. Miller
Cc: netdev, Jacob Keller, Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
This is an u8, so using GENMASK_ULL() for unsigned long long is
unnecessary.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
lib/packing.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/packing.c b/lib/packing.c
index ac1f36c56a77..793942745e34 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -141,7 +141,7 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
proj_start_bit = ((box * BITS_PER_BYTE) + box_start_bit) - endbit;
proj_end_bit = ((box * BITS_PER_BYTE) + box_end_bit) - endbit;
proj_mask = GENMASK_ULL(proj_start_bit, proj_end_bit);
- box_mask = GENMASK_ULL(box_start_bit, box_end_bit);
+ box_mask = GENMASK(box_start_bit, box_end_bit);
/* Determine the offset of the u8 box inside the pbuf,
* adjusted for quirks. The adjusted box_addr will be used for
@@ -249,7 +249,7 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
proj_start_bit = ((box * BITS_PER_BYTE) + box_start_bit) - endbit;
proj_end_bit = ((box * BITS_PER_BYTE) + box_end_bit) - endbit;
proj_mask = GENMASK_ULL(proj_start_bit, proj_end_bit);
- box_mask = GENMASK_ULL(box_start_bit, box_end_bit);
+ box_mask = GENMASK(box_start_bit, box_end_bit);
/* Determine the offset of the u8 box inside the pbuf,
* adjusted for quirks. The adjusted box_addr will be used for
--
2.46.2.828.g9e56e24342b6
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v2 00/10] packing: various improvements and KUnit tests
2024-10-02 21:51 [PATCH net-next v2 00/10] packing: various improvements and KUnit tests Jacob Keller
` (9 preceding siblings ...)
2024-10-02 21:51 ` [PATCH net-next v2 10/10] lib: packing: use GENMASK() for box_mask Jacob Keller
@ 2024-10-03 23:10 ` patchwork-bot+netdevbpf
10 siblings, 0 replies; 21+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-10-03 23:10 UTC (permalink / raw)
To: Jacob Keller
Cc: akpm, olteanv, davem, netdev, vladimir.oltean, przemyslaw.kitszel
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 02 Oct 2024 14:51:49 -0700 you wrote:
> This series contains a handful of improvements and fixes for the packing
> library, including the addition of KUnit tests.
>
> There are two major changes which might be considered bug fixes:
>
> 1) The library is updated to handle arbitrary buffer lengths, fixing
> undefined behavior when operating on buffers which are not a multiple of
> 4 bytes.
>
> [...]
Here is the summary with links:
- [net-next,v2,01/10] lib: packing: refuse operating on bit indices which exceed size of buffer
https://git.kernel.org/netdev/net-next/c/8b3e26677bc6
- [net-next,v2,02/10] lib: packing: adjust definitions and implementation for arbitrary buffer lengths
(no matching commit)
- [net-next,v2,03/10] lib: packing: remove kernel-doc from header file
https://git.kernel.org/netdev/net-next/c/816ad8f1e498
- [net-next,v2,04/10] lib: packing: add pack() and unpack() wrappers over packing()
https://git.kernel.org/netdev/net-next/c/7263f64e16d9
- [net-next,v2,05/10] lib: packing: duplicate pack() and unpack() implementations
https://git.kernel.org/netdev/net-next/c/28aec9ca29f0
- [net-next,v2,06/10] lib: packing: add KUnit tests adapted from selftests
(no matching commit)
- [net-next,v2,07/10] lib: packing: add additional KUnit tests
https://git.kernel.org/netdev/net-next/c/fcd6dd91d0e8
- [net-next,v2,08/10] lib: packing: fix QUIRK_MSB_ON_THE_RIGHT behavior
https://git.kernel.org/netdev/net-next/c/e7fdf5dddce5
- [net-next,v2,09/10] lib: packing: use BITS_PER_BYTE instead of 8
https://git.kernel.org/netdev/net-next/c/fb02c7c8a577
- [net-next,v2,10/10] lib: packing: use GENMASK() for box_mask
https://git.kernel.org/netdev/net-next/c/46e784e94b82
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 21+ messages in thread