[PATCH net-next v3 0/9] lib: packing: introduce and use (un)pack_fields

[PATCH net-next v3 0/9] lib: packing: introduce and use (un)pack_fields

[Jacob Keller]

This series improves the packing library with a new API for packing or
unpacking a large number of fields at once with minimal code footprint. The
API is then used to replace bespoke packing logic in the ice driver,
preparing it to handle unpacking in the future. Finally, the ice driver has
a few other cleanups related to the packing logic.

The pack_fields and unpack_fields functions have the following improvements
over the existing pack() and unpack() API:

 1. Packing or unpacking a large number of fields takes significantly less
    code. This significantly reduces the .text size for an increase in the
    .data size which is much smaller.

 2. The unpacked data can be stored in sizes smaller than u64 variables.
    This reduces the storage requirement both for runtime data structures,
    and for the rodata defining the fields. This scales with the number of
    fields used.

 3. Most of the error checking is done at compile time, rather than
    runtime, via additional checks added to modpost. This saves wasted
    computation time *and* catches errors in the field definitions early,
    rather than only after the offending code is executed.

The actual packing and unpacking code still uses the u64 size
variables. However, these are converted to the appropriate field sizes when
storing or reading the data from the buffer.

The compile time checks do add some complexity to modpost. An attempt was
made originally to implement the checks with C macros. Unfortunately, the C
pre-processor cannot generate loops. Without loops, drivers are responsible
for calling the correct CHECK macro. In addition, this required
several thousand lines of macro to provide variants for the sizes expected
to be used. While this was possible to be generated, it still meant a lot
of mess spreading to the drivers, Makefile, and build process.

To enable modpost checks, the packed field arrays are placed into a special
section of the binary, using DECLARE_PACKED_FIELDS_(S|M) macros. This makes
it relatively easy for drivers to enable. We can also easily implement
checkpatch or cocinelle scripts to detect raw usage of the packed fields
structures if this becomes a problem.

The modpost logic is implemented by scanning the symbols to find entries
in the relevant ".rodata.packed_field_s" or ".rodata.packed_field_m"
sections. This must be extended if we ever add support for a
"packed_field_l" or similar, though this seems unlikely.

In order to enforce buffer size, modpost must have access to the size of
the target buffer. This is achieved by having DECLARE_PACKED_FIELDS* also
generate a const variable with the buffer size. I tried a couple of other
methods of obtaining the size, but this was the simplest one. I think it
may be possible to have the unused symbols discarded, but I did not attempt
this.

I'm in favor of this modpost implementation vs the macros attempted in
previous iterations.

Pros:

 * Significantly less code to implement the checks, even if we ignore the
   generated portions in <generated/packing-checks.h>

 * The modpost checks are able to work in arbitrarily sized arrays, without
   needing to add any modification in the future. The macro implementation
   could require adding additional macros if a driver ever needed >50
   fields.

 * Checks are relatively self contained to modpost. We don't need to edit
   the top level build Kbuild, we don't need to add 50 extra config
   options, and we don't need to leave behind a 20K+ LOC generated header
   file.

 * The fields are required to be fully ordered, by detecting whether the
   first two fields indicate ascending or descending order. This enables
   driver authors to order the fields in which ever ordering is most
   natural for their data. The macro checks did a full overlap check but
   did not enforce order.

 * Drivers simply DECLARE_PACKED_FIELDS_(S|M) and get modpost warnings,
   without requiring a manual call to CHECK_PACKED_FIELDS*. This should
   make it easier to review, and less likely that checks are skipped.

Cons:

 * modpost doesn't seem to have a way to track the symbols back to line
   numbers, so we can't report that info to the user.

 * modpost errors are reported slightly later in the compile process.

 * To maintain the full set of checks, we need to export the size of the
   target buffer to modpost, which I implemented with an additional
   variable that is otherwise entirely unused.

 * We have to place the packed field arrays in special sections to enable
   modpost. This is handled by DECLARE macros, so we have to ensure all
   users use these macros. I did not attempt to add a check for that in
   this series, but it could be done in principle with cocinelle or
   checkpatch.

 * A fair amount of boilerplate code is required to extract the relevant
   data from the symbol definitions, including special casing to handle the
   potential endian issues when using packed_field_m.

 * These type of checks do feel somewhat ancillary to the original purpose
   of modpost.

The fact that the mess of checking the fields is fairly self contained and
avoids the mess of the CHECK_* and config options is a big win, and I think
the modpost option is significantly better for this.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes in v3:
- Replace macro-based C pre-processor checks with checks implemented in
  modpost.
- Move structure definitions into  <linux/packing_types.h> to enable reuse
  within modpost.
- Add DECLARE_PACKED_FIELDS_S and DECLARE_PACKED_FIELDS_M to enable
  automatically generating the buffer size constants and the section
  attributes.
- Add additional unit tests for the pack_fields and unpack_fields APIs.
- Update documentation with an explanation of the new API as well as some
  example code.
- Link to v2: https://lore.kernel.org/r/20241025-packing-pack-fields-and-ice-implementation-v2-0-734776c88e40@intel.com

Changes in v2:
- Add my missing sign-off to the first patch
- Update the descriptions for a few patches
- Only generate CHECK_PACKED_FIELDS_N when another module selects it
- Add a new patch introducing wrapper structures for the packed Tx and Rx
  queue context, suggested by Vladimir.
- Drop the now unnecessary macros in ice, thanks to the new types
- Link to v1: https://lore.kernel.org/r/20241011-packing-pack-fields-and-ice-implementation-v1-0-d9b1f7500740@intel.com

---
Jacob Keller (6):
      ice: remove int_q_state from ice_tlan_ctx
      ice: use structures to keep track of queue context size
      ice: use <linux/packing.h> for Tx and Rx queue context data
      ice: reduce size of queue context fields
      ice: move prefetch enable to ice_setup_rx_ctx
      ice: cleanup Rx queue context programming functions

Vladimir Oltean (3):
      lib: packing: create __pack() and __unpack() variants without error checking
      lib: packing: demote truncation error in pack() to a warning in __pack()
      lib: packing: add pack_fields() and unpack_fields()

 drivers/net/ethernet/intel/ice/ice_adminq_cmd.h |  11 +-
 drivers/net/ethernet/intel/ice/ice_common.h     |   5 +-
 drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h  |  49 +---
 include/linux/packing.h                         |  29 +++
 include/linux/packing_types.h                   |  54 +++++
 scripts/mod/modpost.h                           |  19 ++
 drivers/net/dsa/sja1105/sja1105_static_config.c |   8 +-
 drivers/net/ethernet/intel/ice/ice_base.c       |   6 +-
 drivers/net/ethernet/intel/ice/ice_common.c     | 297 +++++-------------------
 lib/packing.c                                   | 285 +++++++++++++++++------
 lib/packing_test.c                              |  61 +++++
 scripts/mod/modpost.c                           |  18 +-
 scripts/mod/packed_fields.c                     | 294 +++++++++++++++++++++++
 Documentation/core-api/packing.rst              |  55 +++++
 MAINTAINERS                                     |   2 +
 drivers/net/ethernet/intel/Kconfig              |   1 +
 scripts/mod/Makefile                            |   4 +-
 17 files changed, 828 insertions(+), 370 deletions(-)
---
base-commit: a84e8c05f58305dfa808bc5465c5175c29d7c9b6
change-id: 20241004-packing-pack-fields-and-ice-implementation-b17c7ce8e373

Best regards,
-- 
Jacob Keller <jacob.e.keller@intel.com>

[PATCH net-next v3 1/9] lib: packing: create __pack() and __unpack() variants without error checking

[Jacob Keller]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

A future variant of the API, which works on arrays of packed_field
structures, will make most of these checks redundant. The idea will be
that we want to perform sanity checks at compile time, not once
for every function call.

Introduce new variants of pack() and unpack(), which elide the sanity
checks, assuming that the input was pre-sanitized.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 lib/packing.c | 142 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 78 insertions(+), 64 deletions(-)

diff --git a/lib/packing.c b/lib/packing.c
index 793942745e34..c29b079fdd78 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -51,64 +51,20 @@ static size_t calculate_box_addr(size_t box, size_t len, u8 quirks)
 	return offset_of_group + offset_in_group;
 }
 
-/**
- * pack - Pack u64 number into bitfield of buffer.
- *
- * @pbuf: Pointer to a buffer holding the packed 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.
- * @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, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
-	 u8 quirks)
+static void __pack(void *pbuf, u64 uval, size_t startbit, size_t endbit,
+		   size_t pbuflen, u8 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 >= BITS_PER_BYTE * pbuflen))
-		/* Invalid function call */
-		return -EINVAL;
-
-	value_width = startbit - endbit + 1;
-	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 (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;
+	int plogical_first_u8 = startbit / BITS_PER_BYTE;
+	int plogical_last_u8 = endbit / BITS_PER_BYTE;
+	int box;
 
 	/* 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 / 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 */
 		size_t box_start_bit, box_end_bit, box_addr;
@@ -163,15 +119,13 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
 		((u8 *)pbuf)[box_addr] &= ~box_mask;
 		((u8 *)pbuf)[box_addr] |= pval;
 	}
-	return 0;
 }
-EXPORT_SYMBOL(pack);
 
 /**
- * unpack - Unpack u64 number from packed buffer.
+ * 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.
@@ -183,16 +137,12 @@ EXPORT_SYMBOL(pack);
  *	    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.
+ *	   correct usage, return code may be discarded. The @pbuf memory will
+ *	   be modified on success.
  */
-int unpack(const void *pbuf, 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)
 {
-	/* 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;
 
@@ -207,6 +157,33 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
 	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 (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;
+
+	__pack(pbuf, uval, startbit, endbit, pbuflen, quirks);
+
+	return 0;
+}
+EXPORT_SYMBOL(pack);
+
+static void __unpack(const void *pbuf, u64 *uval, size_t startbit,
+		     size_t endbit, size_t pbuflen, u8 quirks)
+{
+	/* Logical byte indices corresponding to the
+	 * start and end of the field.
+	 */
+	int plogical_first_u8 = startbit / BITS_PER_BYTE;
+	int plogical_last_u8 = endbit / BITS_PER_BYTE;
+	int box;
+
 	/* Initialize parameter */
 	*uval = 0;
 
@@ -214,9 +191,6 @@ 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 / 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 */
 		size_t box_start_bit, box_end_bit, box_addr;
@@ -271,6 +245,46 @@ int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
 		*uval &= ~proj_mask;
 		*uval |= pval;
 	}
+}
+
+/**
+ * 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)
+{
+	/* 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 (startbit < endbit || startbit >= BITS_PER_BYTE * pbuflen)
+		/* Invalid function call */
+		return -EINVAL;
+
+	value_width = startbit - endbit + 1;
+	if (value_width > 64)
+		return -ERANGE;
+
+	__unpack(pbuf, uval, startbit, endbit, pbuflen, quirks);
+
 	return 0;
 }
 EXPORT_SYMBOL(unpack);

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 2/9] lib: packing: demote truncation error in pack() to a warning in __pack()

[Jacob Keller]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Most of the sanity checks in pack() and unpack() can be covered at
compile time. There is only one exception, and that is truncation of the
uval during a pack() operation.

We'd like the error-less __pack() to catch that condition as well. But
at the same time, it is currently the responsibility of consumer drivers
(currently just sja1105) to print anything at all when this error
occurs, and then discard the return code.

We can just print a loud warning in the library code and continue with
the truncated __pack() operation. In practice, having the warning is
very important, see commit 24deec6b9e4a ("net: dsa: sja1105: disallow
C45 transactions on the BASE-TX MDIO bus") where the bug was caught
exactly by noticing this print.

Add the first print to the packing library, and at the same time remove
the print for the same condition from the sja1105 driver, to avoid
double printing.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_static_config.c |  8 ++------
 lib/packing.c                                   | 26 ++++++++++---------------
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_static_config.c b/drivers/net/dsa/sja1105/sja1105_static_config.c
index baba204ad62f..3d790f8c6f4d 100644
--- a/drivers/net/dsa/sja1105/sja1105_static_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_static_config.c
@@ -26,12 +26,8 @@ void sja1105_pack(void *buf, const u64 *val, int start, int end, size_t len)
 		pr_err("Start bit (%d) expected to be larger than end (%d)\n",
 		       start, end);
 	} else if (rc == -ERANGE) {
-		if ((start - end + 1) > 64)
-			pr_err("Field %d-%d too large for 64 bits!\n",
-			       start, end);
-		else
-			pr_err("Cannot store %llx inside bits %d-%d (would truncate)\n",
-			       *val, start, end);
+		pr_err("Field %d-%d too large for 64 bits!\n",
+		       start, end);
 	}
 	dump_stack();
 }
diff --git a/lib/packing.c b/lib/packing.c
index c29b079fdd78..2bf81951dfc8 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -59,8 +59,17 @@ static void __pack(void *pbuf, u64 uval, size_t startbit, size_t endbit,
 	 */
 	int plogical_first_u8 = startbit / BITS_PER_BYTE;
 	int plogical_last_u8 = endbit / BITS_PER_BYTE;
+	int value_width = startbit - endbit + 1;
 	int box;
 
+	/* Check if "uval" fits in "value_width" bits.
+	 * The test only works for value_width < 64, but in the latter case,
+	 * any 64-bit uval will surely fit.
+	 */
+	WARN(value_width < 64 && uval >= (1ull << value_width),
+	     "Cannot store 0x%llx inside bits %zu-%zu - will truncate\n",
+	     uval, startbit, endbit);
+
 	/* 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.
@@ -143,9 +152,6 @@ static void __pack(void *pbuf, u64 uval, size_t startbit, size_t endbit,
 int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
 	 u8 quirks)
 {
-	/* 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.
 	 */
@@ -153,19 +159,7 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
 		/* Invalid function call */
 		return -EINVAL;
 
-	value_width = startbit - endbit + 1;
-	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 (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.
-		 */
+	if (unlikely(startbit - endbit >= 64))
 		return -ERANGE;
 
 	__pack(pbuf, uval, startbit, endbit, pbuflen, quirks);

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[Jacob Keller]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This is new API which caters to the following requirements:

- Pack or unpack a large number of fields to/from a buffer with a small
  code footprint. The current alternative is to open-code a large number
  of calls to pack() and unpack(), or to use packing() to reduce that
  number to half. But packing() is not const-correct.

- Use unpacked numbers stored in variables smaller than u64. This
  reduces the rodata footprint of the stored field arrays.

- Perform error checking at compile time, rather than runtime, and return
  void from the API functions. Because the C preprocessor can't generat
  variable length code (loops), we can't easily use macros to implement the
  compile time checks.

  Instead, checks are implemented in modpost. This allows writing C code to
  validate the expected data. The symbols for the packed field arrays are
  placed into special sections of the binary, via the __section()
  attribute. The maximum size of the buffer is also placed as a constant to
  allow extracting it from the module in modpost, ensuring we can validate
  the packed fields fit into the relevant packed buffer.

  While the use of modpost is somewhat ugly, it is entirely contained and
  drivers simply declare their packed field arrays with
  DECLARE_PACKED_FIELDS_S or DECLARE_PACKED_FIELDS_M as appropriate. This
  reduces the burden on driver authors and keeps the checks contained.
  Additionally, use of modpost enables dynamically enforcing all the fields
  be in ascending or descending order.

- Reduced rodata footprint for the storage of the packed field arrays.
  To that end, we have struct packed_field_s (small) and packed_field_m
  (medium). More can be added as needed (unlikely for now). On these
  types, the same generic pack_fields() and unpack_fields() API can be
  used, thanks to the new C11 _Generic() selection feature, which can
  call pack_fields_s() or pack_fields_m(), depending on the type of the
  "fields" array - a simplistic form of polymorphism. It is evaluated at
  compile time which function will actually be called.

Over time, packing() is expected to be completely replaced either with
pack() or with pack_fields().

Co-developed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/packing.h            |  29 ++++
 include/linux/packing_types.h      |  54 +++++++
 scripts/mod/modpost.h              |  19 +++
 lib/packing.c                      | 149 ++++++++++++++++++-
 lib/packing_test.c                 |  61 ++++++++
 scripts/mod/modpost.c              |  18 +--
 scripts/mod/packed_fields.c        | 294 +++++++++++++++++++++++++++++++++++++
 Documentation/core-api/packing.rst |  55 +++++++
 MAINTAINERS                        |   2 +
 scripts/mod/Makefile               |   4 +-
 10 files changed, 667 insertions(+), 18 deletions(-)

diff --git a/include/linux/packing.h b/include/linux/packing.h
index 5d36dcd06f60..12218a82d956 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -7,6 +7,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
+#include <linux/packing_types.h>
 
 #define QUIRK_MSB_ON_THE_RIGHT	BIT(0)
 #define QUIRK_LITTLE_ENDIAN	BIT(1)
@@ -26,4 +27,32 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
 int unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
 	   size_t pbuflen, u8 quirks);
 
+void pack_fields_s(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_s *fields, size_t num_fields,
+		   u8 quirks);
+
+void pack_fields_m(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_m *fields, size_t num_fields,
+		   u8 quirks);
+
+void unpack_fields_s(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_s *fields, size_t num_fields,
+		     u8 quirks);
+
+void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_m *fields, size_t num_fields,
+		     u8 quirks);
+
+#define pack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
+	_Generic((fields), \
+		 const struct packed_field_s * : pack_fields_s, \
+		 const struct packed_field_m * : pack_fields_m \
+		)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
+
+#define unpack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
+	_Generic((fields), \
+		 const struct packed_field_s * : unpack_fields_s, \
+		 const struct packed_field_m * : unpack_fields_m \
+		)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
+
 #endif
diff --git a/include/linux/packing_types.h b/include/linux/packing_types.h
new file mode 100644
index 000000000000..49ae4329a494
--- /dev/null
+++ b/include/linux/packing_types.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2024, Intel Corporation
+ * Copyright (c) 2024, Vladimir Oltean <olteanv@gmail.com>
+ */
+#ifndef _LINUX_PACKING_TYPES_H
+#define _LINUX_PACKING_TYPES_H
+
+#include <linux/types.h>
+
+/* If you add another packed field type, please update
+ * scripts/mod/packed_fields.c to enable compile time sanity checks.
+ */
+
+#define GEN_PACKED_FIELD_MEMBERS(__type) \
+	__type startbit; \
+	__type endbit; \
+	__type offset; \
+	__type size;
+
+/* Small packed field. Use with bit offsets < 256, buffers < 32B and
+ * unpacked structures < 256B.
+ */
+struct packed_field_s {
+	GEN_PACKED_FIELD_MEMBERS(u8);
+};
+
+#define __pf_section_s __section(".rodata.packed_fields_s")
+
+#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
+	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
+	const struct packed_field_s name[] __pf_section_s
+
+/* Medium packed field. Use with bit offsets < 65536, buffers < 8KB and
+ * unpacked structures < 64KB.
+ */
+struct packed_field_m {
+	GEN_PACKED_FIELD_MEMBERS(u16);
+};
+
+#define __pf_section_m __section(".rodata.packed_fields_m")
+
+#define DECLARE_PACKED_FIELDS_M(name, buffer_sz) \
+	const size_t __ ## name ## _buffer_sz __pf_section_m = buffer_sz; \
+	const struct packed_field_m name[] __pf_section_m
+
+#define PACKED_FIELD(start, end, struct_name, struct_field) \
+{ \
+	(start), \
+	(end), \
+	offsetof(struct_name, struct_field), \
+	sizeof_field(struct_name, struct_field), \
+}
+
+#endif
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index ada3a36cc4bc..013bf4be2642 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -160,6 +160,19 @@ static inline bool is_valid_name(struct elf_info *elf, Elf_Sym *sym)
 	return !is_mapping_symbol(name);
 }
 
+/* This is based on the hash algorithm from gdbm, via tdb */
+static inline unsigned int tdb_hash(const char *name)
+{
+	unsigned value;	/* Used to compute the hash value.  */
+	unsigned   i;	/* Used to cycle through random values. */
+
+	/* Set the initial value from the key size. */
+	for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
+		value = (value + (((unsigned char *)name)[i] << (i*5 % 24)));
+
+	return (1103515243 * value + 12345);
+}
+
 /* symsearch.c */
 void symsearch_init(struct elf_info *elf);
 void symsearch_finish(struct elf_info *elf);
@@ -175,12 +188,18 @@ void add_moddevtable(struct buffer *buf, struct module *mod);
 /* sumversion.c */
 void get_src_version(const char *modname, char sum[], unsigned sumlen);
 
+/* packed_fields.c */
+void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
+				Elf_Sym *sym, const char *symname);
+void check_packed_field_symbols(void);
+
 /* from modpost.c */
 extern bool target_is_big_endian;
 extern bool host_is_big_endian;
 char *read_text_file(const char *filename);
 char *get_line(char **stringp);
 void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym);
+const char *sec_name(const struct elf_info *info, unsigned int secindex);
 
 void __attribute__((format(printf, 2, 3)))
 modpost_log(bool is_error, const char *fmt, ...);
diff --git a/lib/packing.c b/lib/packing.c
index 2bf81951dfc8..45164f73fe5b 100644
--- a/lib/packing.c
+++ b/lib/packing.c
@@ -5,10 +5,37 @@
 #include <linux/packing.h>
 #include <linux/module.h>
 #include <linux/bitops.h>
+#include <linux/bits.h>
 #include <linux/errno.h>
 #include <linux/types.h>
 #include <linux/bitrev.h>
 
+#define __pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks)	\
+	({									\
+		for (size_t i = 0; i < (num_fields); i++) {			\
+			typeof(&(fields)[0]) field = &(fields)[i];		\
+			u64 uval;						\
+										\
+			uval = ustruct_field_to_u64(ustruct, field->offset, field->size); \
+										\
+			__pack(pbuf, uval, field->startbit, field->endbit,	\
+			       pbuflen, quirks);				\
+		}								\
+	})
+
+#define __unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks)	\
+	({									\
+		for (size_t i = 0; i < (num_fields); i++) {			\
+			typeof(&(fields)[0]) field = &fields[i];		\
+			u64 uval;						\
+										\
+			__unpack(pbuf, &uval, field->startbit, field->endbit,	\
+				 pbuflen, quirks);				\
+										\
+			u64_to_ustruct_field(ustruct, field->offset, field->size, uval); \
+		}								\
+	})
+
 /**
  * calculate_box_addr - Determine physical location of byte in buffer
  * @box: Index of byte within buffer seen as a logical big-endian big number
@@ -168,8 +195,8 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
 }
 EXPORT_SYMBOL(pack);
 
-static void __unpack(const void *pbuf, u64 *uval, size_t startbit,
-		     size_t endbit, size_t pbuflen, u8 quirks)
+static void __unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
+		     size_t pbuflen, u8 quirks)
 {
 	/* Logical byte indices corresponding to the
 	 * start and end of the field.
@@ -322,4 +349,122 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
 }
 EXPORT_SYMBOL(packing);
 
+static u64 ustruct_field_to_u64(const void *ustruct, size_t field_offset,
+				size_t field_size)
+{
+	switch (field_size) {
+	case 1:
+		return *((u8 *)(ustruct + field_offset));
+	case 2:
+		return *((u16 *)(ustruct + field_offset));
+	case 4:
+		return *((u32 *)(ustruct + field_offset));
+	default:
+		return *((u64 *)(ustruct + field_offset));
+	}
+}
+
+static void u64_to_ustruct_field(void *ustruct, size_t field_offset,
+				 size_t field_size, u64 uval)
+{
+	switch (field_size) {
+	case 1:
+		*((u8 *)(ustruct + field_offset)) = uval;
+		break;
+	case 2:
+		*((u16 *)(ustruct + field_offset)) = uval;
+		break;
+	case 4:
+		*((u32 *)(ustruct + field_offset)) = uval;
+		break;
+	default:
+		*((u64 *)(ustruct + field_offset)) = uval;
+		break;
+	}
+}
+
+/**
+ * pack_fields_s - Pack array of small fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of small packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void pack_fields_s(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_s *fields, size_t num_fields,
+		   u8 quirks)
+{
+	__pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(pack_fields_s);
+
+/**
+ * pack_fields_m - Pack array of medium fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of medium packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void pack_fields_m(void *pbuf, size_t pbuflen, const void *ustruct,
+		   const struct packed_field_m *fields, size_t num_fields,
+		   u8 quirks)
+{
+	__pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(pack_fields_m);
+
+/**
+ * unpack_fields_s - Unpack array of small fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of small packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void unpack_fields_s(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_s *fields, size_t num_fields,
+		     u8 quirks)
+{
+	__unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(unpack_fields_s);
+
+/**
+ * unpack_fields_m - Unpack array of medium fields
+ *
+ * @pbuf: Pointer to a buffer holding the packed value.
+ * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
+ * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
+ *	     It is expected (but not checked) that this has the same data type
+ *	     as all struct packed_field_s definitions.
+ * @fields: Array of medium packed fields definition. They must not overlap.
+ * @num_fields: Length of @fields array.
+ * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
+ *	    QUIRK_MSB_ON_THE_RIGHT.
+ */
+void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
+		     const struct packed_field_m *fields, size_t num_fields,
+		     u8 quirks)
+{
+	__unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
+}
+EXPORT_SYMBOL(unpack_fields_m);
+
 MODULE_DESCRIPTION("Generic bitfield packing and unpacking");
diff --git a/lib/packing_test.c b/lib/packing_test.c
index b38ea43c03fd..ff5be660de01 100644
--- a/lib/packing_test.c
+++ b/lib/packing_test.c
@@ -396,9 +396,70 @@ static void packing_test_unpack(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, uval, params->uval);
 }
 
+#define PACKED_BUF_SIZE 8
+
+typedef struct __packed { u8 buf[PACKED_BUF_SIZE]; } packed_buf_t;
+
+struct test_data {
+	u8 field1;
+	u16 field2;
+	u32 field3;
+	u16 field4;
+	u8 field5;
+	u16 field6;
+};
+
+DECLARE_PACKED_FIELDS_S(test_fields, sizeof(packed_buf_t)) = {
+	PACKED_FIELD(63, 61, struct test_data, field1),
+	PACKED_FIELD(60, 52, struct test_data, field2),
+	PACKED_FIELD(51, 28, struct test_data, field3),
+	PACKED_FIELD(27, 14, struct test_data, field4),
+	PACKED_FIELD(13, 9, struct test_data, field5),
+	PACKED_FIELD(8, 0, struct test_data, field6),
+};
+
+static void packing_test_pack_fields(struct kunit *test)
+{
+	const struct test_data data = {
+		.field1 = 0x2,
+		.field2 = 0x100,
+		.field3 = 0xF00050,
+		.field4 = 0x7D3,
+		.field5 = 0x9,
+		.field6 = 0x10B,
+	};
+	packed_buf_t buf = {};
+	packed_buf_t expect = {
+		.buf = { 0x50, 0x0F, 0x00, 0x05, 0x01, 0xF4, 0xD3, 0x0B },
+	};
+
+	pack_fields(&buf, sizeof(buf), &data, test_fields, 0);
+
+	KUNIT_EXPECT_MEMEQ(test, &expect, &buf, sizeof(buf));
+}
+
+static void packing_test_unpack_fields(struct kunit *test)
+{
+	const packed_buf_t buf = {
+		.buf = { 0x17, 0x28, 0x10, 0x19, 0x3D, 0xA9, 0x07, 0x9C },
+	};
+	struct test_data data = {};
+
+	unpack_fields(&buf, sizeof(buf), &data, test_fields, 0);
+
+	KUNIT_EXPECT_EQ(test, 0, data.field1);
+	KUNIT_EXPECT_EQ(test, 0x172, data.field2);
+	KUNIT_EXPECT_EQ(test, 0x810193, data.field3);
+	KUNIT_EXPECT_EQ(test, 0x36A4, data.field4);
+	KUNIT_EXPECT_EQ(test, 0x3, data.field5);
+	KUNIT_EXPECT_EQ(test, 0x19C, data.field6);
+}
+
 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),
+	KUNIT_CASE(packing_test_pack_fields),
+	KUNIT_CASE(packing_test_unpack_fields),
 	{},
 };
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 107393a8c48a..3f707bba18d7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -209,19 +209,6 @@ struct symbol {
 
 static HASHTABLE_DEFINE(symbol_hashtable, 1U << 10);
 
-/* This is based on the hash algorithm from gdbm, via tdb */
-static inline unsigned int tdb_hash(const char *name)
-{
-	unsigned value;	/* Used to compute the hash value.  */
-	unsigned   i;	/* Used to cycle through random values. */
-
-	/* Set the initial value from the key size. */
-	for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
-		value = (value + (((unsigned char *)name)[i] << (i*5 % 24)));
-
-	return (1103515243 * value + 12345);
-}
-
 /**
  * Allocate a new symbols for use in the hash of exported symbols or
  * the list of unresolved symbols per module
@@ -327,7 +314,7 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
 				      sechdr->sh_name);
 }
 
-static const char *sec_name(const struct elf_info *info, unsigned int secindex)
+const char *sec_name(const struct elf_info *info, unsigned int secindex)
 {
 	/*
 	 * If sym->st_shndx is a special section index, there is no
@@ -1602,6 +1589,7 @@ static void read_symbols(const char *modname)
 
 		handle_symbol(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
+		handle_packed_field_symbol(mod, &info, sym, symname);
 	}
 
 	check_sec_ref(mod, &info);
@@ -2222,6 +2210,8 @@ int main(int argc, char **argv)
 	if (missing_namespace_deps)
 		write_namespace_deps_files(missing_namespace_deps);
 
+	check_packed_field_symbols();
+
 	if (dump_write)
 		write_dump(dump_write);
 	if (sec_mismatch_count && !sec_mismatch_warn_only)
diff --git a/scripts/mod/packed_fields.c b/scripts/mod/packed_fields.c
new file mode 100644
index 000000000000..aa9dbd704e52
--- /dev/null
+++ b/scripts/mod/packed_fields.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024, Intel Corporation. */
+
+/* Code to validate struct packed_field_[sm] data, and perform sanity checks
+ * to ensure the packed field data is laid out correctly and fits into the
+ * relevant buffer size.
+ */
+
+#include <fnmatch.h>
+#include <hashtable.h>
+#include <inttypes.h>
+#include <stdint.h>
+#include <xalloc.h>
+
+#include "modpost.h"
+
+typedef uint16_t	u16;
+typedef uint8_t		u8;
+
+#define BITS_PER_BYTE	8
+
+/* Big exception to the "don't include kernel headers into userspace", which
+ * even potentially has different endianness and word sizes, since we handle
+ * those differences explicitly below
+ */
+#include "../../include/linux/packing_types.h"
+
+struct packed_field_elem {
+	uint64_t startbit;
+	uint64_t endbit;
+	uint64_t offset;
+	uint64_t size;
+};
+
+enum field_type {
+	UNKNOWN_SECTION,
+	PACKED_FIELD_S,
+	PACKED_FIELD_M,
+};
+
+static size_t field_type_to_size(enum field_type type)
+{
+	switch (type) {
+	case PACKED_FIELD_S:
+		return sizeof(struct packed_field_s);
+	case PACKED_FIELD_M:
+		return sizeof(struct packed_field_m);
+	default:
+		error("attempted to get field size for unknown packed field type %u\n",
+		      type);
+		return 0;
+	}
+}
+
+static void get_field_contents(const void *data, enum field_type type,
+			       struct packed_field_elem *elem)
+{
+	switch (type) {
+	case PACKED_FIELD_S: {
+		const struct packed_field_s *data_field = data;
+
+		elem->startbit = TO_NATIVE(data_field->startbit);
+		elem->endbit = TO_NATIVE(data_field->endbit);
+		elem->offset = TO_NATIVE(data_field->offset);
+		elem->size = TO_NATIVE(data_field->size);
+		return;
+	}
+	case PACKED_FIELD_M: {
+		const struct packed_field_m *data_field = data;
+
+		elem->startbit = TO_NATIVE(data_field->startbit);
+		elem->endbit = TO_NATIVE(data_field->endbit);
+		elem->offset = TO_NATIVE(data_field->offset);
+		elem->size = TO_NATIVE(data_field->size);
+		return;
+	}
+	default:
+		error("attempted to get field contents for unknown packed field type %u\n",
+		      type);
+	}
+}
+
+struct field_symbol {
+	struct hlist_node hnode;
+	enum field_type type;
+	size_t buffer_size;
+	size_t data_size;
+	void *data;
+	struct module *mod;
+	char *name;
+};
+
+static HASHTABLE_DEFINE(field_hashtable, 1U << 10);
+
+static struct field_symbol *alloc_field(char *name, struct module *mod)
+{
+	struct field_symbol *f = xmalloc(sizeof(*f));
+
+	memset(f, 0, sizeof(*f));
+	f->mod = mod;
+	f->name = name;
+
+	return f;
+}
+
+static void hash_add_field(struct field_symbol *field)
+{
+	hash_add(field_hashtable, &field->hnode, tdb_hash(field->name));
+}
+
+static struct field_symbol *find_field(const char *name, struct module *mod)
+{
+	struct field_symbol *f;
+
+	hash_for_each_possible(field_hashtable, f, hnode, tdb_hash(name)) {
+		if (strcmp(f->name, name) == 0 && f->mod == mod)
+			return f;
+	}
+	return NULL;
+}
+
+void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
+				Elf_Sym *sym, const char *symname)
+{
+	unsigned int secindex = get_secindex(info, sym);
+	enum field_type type = UNKNOWN_SECTION;
+	bool is_size_symbol = false;
+	struct field_symbol *f;
+	const char *section;
+	char *name;
+
+	/* Skip symbols without a name */
+	if (*symname == '\0')
+		return;
+
+	/* Skip symbols with invalid sections */
+	if (secindex >= info->num_sections)
+		return;
+
+	section = sec_name(info, secindex);
+
+	if (strcmp(section, ".rodata.packed_fields_s") == 0)
+		type = PACKED_FIELD_S;
+	else if (strcmp(section, ".rodata.packed_fields_m") == 0)
+		type = PACKED_FIELD_M;
+
+	/* Other sections don't relate to packed fields */
+	if (type == UNKNOWN_SECTION)
+		return;
+
+	name = xstrdup(symname);
+
+	/* Extract original field name from the size symbol */
+	if (!fnmatch("__*_buffer_sz", name, 0)) {
+		name += strlen("__");
+		name[strlen(name) - strlen("_buffer_sz")] = '\0';
+		is_size_symbol = true;
+	}
+
+	f = find_field(name, mod);
+	if (!f) {
+		f = alloc_field(name, mod);
+		f->type = type;
+		hash_add_field(f);
+	}
+
+	if (f->type != type) {
+		error("%s and %s have mismatched packed field sections\n",
+		      f->name, symname);
+		return;
+	}
+
+	if (is_size_symbol) {
+		size_t *size_data = sym_get_data(info, sym);
+		size_t size = TO_NATIVE(*size_data);
+
+		if (f->buffer_size && f->buffer_size != size) {
+			error("%s has buffer size %zu, but symbol %s says the size should be %zu\n",
+			      f->name, f->buffer_size, symname, size);
+		}
+
+		f->buffer_size = size;
+	} else {
+		if (f->data)
+			error("%s has multiple data symbols???\n",
+			      f->name);
+
+		f->data_size = sym->st_size;
+		f->data = xmalloc(f->data_size);
+		memcpy(f->data, sym_get_data(info, sym), f->data_size);
+	}
+}
+
+enum element_order {
+	FIRST_ELEMENT,
+	SECOND_ELEMENT,
+	ASCENDING_ORDER,
+	DESCENDING_ORDER,
+};
+
+static void check_packed_field_array(const struct field_symbol *f)
+{
+	struct packed_field_elem previous_elem = {};
+	size_t field_size = field_type_to_size(f->type);
+	enum element_order order = FIRST_ELEMENT;
+	void *data_ptr;
+	int count;
+
+	/* check that the data is a multiple of the size */
+	if (f->data_size % field_size != 0) {
+		error("symbol %s of module %s has size %zu which is not a multiple of the field size (%zu)\n",
+		      f->name, f->mod->name, f->data_size, field_size);
+		return;
+	}
+
+	data_ptr = f->data;
+	count = 0;
+
+	while (data_ptr < f->data + f->data_size) {
+		struct packed_field_elem elem = {};
+
+		get_field_contents(data_ptr, f->type, &elem);
+
+		if (elem.startbit < elem.endbit)
+			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") must be larger than endbit (%" PRIu64 ")\n",
+			      f->name, f->mod->name, count, elem.startbit,
+			      elem.endbit);
+
+		if (elem.startbit >= BITS_PER_BYTE * f->buffer_size)
+			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") puts field outsize of the packed buffer size (%" PRIu64 ")\n",
+			      f->name, f->mod->name, count, elem.startbit,
+			      f->buffer_size);
+
+		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size)
+			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") and endbit (%" PRIu64 ") indicate a field of width (%" PRIu64 ") which does not fit into the field size (%" PRIu64 ")\n",
+			      f->name, f->mod->name, count, elem.startbit,
+			      elem.endbit, elem.startbit - elem.endbit,
+			      elem.size);
+
+		if (elem.size != 1 && elem.size != 2 && elem.size != 4 && elem.size != 8)
+			error("\"%s\" [%s.ko] element %u size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
+			      f->name, f->mod->name, count, elem.size);
+
+		switch (order) {
+		case FIRST_ELEMENT:
+			order = SECOND_ELEMENT;
+			break;
+		case SECOND_ELEMENT:
+			order = previous_elem.startbit < elem.startbit ?
+				ASCENDING_ORDER : DESCENDING_ORDER;
+			break;
+		default:
+			break;
+		}
+
+		switch (order) {
+		case ASCENDING_ORDER:
+			if (previous_elem.startbit >= elem.startbit)
+				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element startbit is %" PRIu64 "\n",
+				      f->name, f->mod->name, count,
+				      elem.startbit, previous_elem.startbit);
+			if (previous_elem.endbit >= elem.endbit)
+				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element endbit is %" PRIu64 "\n",
+				      f->name, f->mod->name, count, elem.endbit,
+				      previous_elem.endbit);
+
+			break;
+		case DESCENDING_ORDER:
+			if (previous_elem.startbit <= elem.startbit)
+				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in descending order, but previous element startbit is %" PRIu64 "\n",
+				      f->name, f->mod->name, count,
+				      elem.startbit, previous_elem.startbit);
+			if (previous_elem.endbit <= elem.endbit)
+				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in descending order, but previous element endbit is %" PRIu64 "\n",
+				      f->name, f->mod->name, count,
+				      elem.endbit, previous_elem.endbit);
+			break;
+		default:
+			break;
+		}
+
+		previous_elem = elem;
+		data_ptr += field_size;
+		count++;
+	}
+}
+
+void check_packed_field_symbols(void)
+{
+	struct field_symbol *f;
+
+	hash_for_each(field_hashtable, f, hnode)
+		check_packed_field_array(f);
+}
diff --git a/Documentation/core-api/packing.rst b/Documentation/core-api/packing.rst
index 821691f23c54..abfdc429fc26 100644
--- a/Documentation/core-api/packing.rst
+++ b/Documentation/core-api/packing.rst
@@ -235,3 +235,58 @@ programmer against incorrect API use.  The errors are not expected to occur
 during runtime, therefore it is reasonable for xxx_packing() to return void
 and simply swallow those errors. Optionally it can dump stack or print the
 error description.
+
+The pack_fields() and unpack_fields() macros automatically select the
+appropriate function at compile time based on the type of the fields array
+passed in.
+
+Packed Fields
+-------------
+
+Drivers are encouraged to use the ``pack_fields()`` and ``unpack_fields()``
+APIs over using ``pack()``, ``unpack()``, or ``packing()``.
+
+These APIs use field definitions in arrays of ``struct packed_field_s`` or
+``struct packed_field_m`` stored as ``.rodata``. This significantly reduces
+the code footprint required to pack or unpack many fields. In addition,
+sanity checks on the field definitions are handled at compile time via
+modpost warnings, rather than only when the offending code is executed.
+
+Example
+-------
+
+.. code-block:: c
+
+   struct data {
+        u64 field3;
+        u32 field4;
+        u16 field1;
+        u8 field2;
+   };
+
+   #define SIZE 13
+
+   typdef struct __packed { u8 buf[SIZE]; } packed_buf_t;
+
+   DECLARE_PACKED_FIELDS_S(fields, SIZE) = {
+           PACKED_FIELD(100, 90, struct data, field1),
+           PACKED_FIELD(90, 87, struct data, field2),
+           PACKED_FIELD(86, 30, struct data, field3),
+           PACKED_FIELD(29, 0, struct data, field4),
+   };
+
+   void unpack_your_data(const packed_buf_t *buf, struct data *unpacked)
+   {
+           BUILD_BUG_ON(sizeof(*buf) != SIZE;
+
+           unpack_fields(buf, sizeof(*buf), unpacked, fields,
+                         QUIRK_LITTLE_ENDIAN);
+   }
+
+   void pack_your_data(const struct data *unpacked, packed_buf_t *buf)
+   {
+           BUILD_BUG_ON(sizeof(*buf) != SIZE;
+
+           pack_fields(buf, sizeof(*buf), unpacked, fields,
+                       QUIRK_LITTLE_ENDIAN);
+   }
diff --git a/MAINTAINERS b/MAINTAINERS
index c51fd742cbc8..5649263504c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17391,8 +17391,10 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	Documentation/core-api/packing.rst
 F:	include/linux/packing.h
+F:	include/linux/packing_types.h
 F:	lib/packing.c
 F:	lib/packing_test.c
+F:	scripts/mod/packed_fields.c
 
 PADATA PARALLEL EXECUTION MECHANISM
 M:	Steffen Klassert <steffen.klassert@secunet.com>
diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index c729bc936bae..aa729b6000b6 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -4,7 +4,7 @@ CFLAGS_REMOVE_empty.o += $(CC_FLAGS_LTO)
 hostprogs-always-y	+= modpost mk_elfconfig
 always-y		+= empty.o
 
-modpost-objs	:= modpost.o file2alias.o sumversion.o symsearch.o
+modpost-objs	:= modpost.o file2alias.o sumversion.o symsearch.o packed_fields.o
 
 devicetable-offsets-file := devicetable-offsets.h
 
@@ -15,7 +15,7 @@ targets += $(devicetable-offsets-file) devicetable-offsets.s
 
 # dependencies on generated files need to be listed explicitly
 
-$(obj)/modpost.o $(obj)/file2alias.o $(obj)/sumversion.o $(obj)/symsearch.o: $(obj)/elfconfig.h
+$(obj)/modpost.o $(obj)/file2alias.o $(obj)/sumversion.o $(obj)/symsearch.o $(obj)/packed_fields.o: $(obj)/elfconfig.h
 $(obj)/file2alias.o: $(obj)/$(devicetable-offsets-file)
 
 quiet_cmd_elfconfig = MKELF   $@

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 4/9] ice: remove int_q_state from ice_tlan_ctx

[Jacob Keller]

The int_q_state field of the ice_tlan_ctx structure represents the internal
queue state. However, we never actually need to assign this or read this
during normal operation. In fact, trying to unpack it would not be possible
as it is larger than a u64. Remove this field from the ice_tlan_ctx
structure, and remove its packing field from the ice_tlan_ctx_info array.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h | 1 -
 drivers/net/ethernet/intel/ice/ice_common.c    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
index 611577ebc29d..0e8ed8c226e6 100644
--- a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
+++ b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
@@ -590,7 +590,6 @@ struct ice_tlan_ctx {
 	u8 drop_ena;
 	u8 cache_prof_idx;
 	u8 pkt_shaper_prof_idx;
-	u8 int_q_state;	/* width not needed - internal - DO NOT WRITE!!! */
 };
 
 #endif /* _ICE_LAN_TX_RX_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index b22e71dc59d4..0f5a80269a7b 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1467,7 +1467,6 @@ const struct ice_ctx_ele ice_tlan_ctx_info[] = {
 	ICE_CTX_STORE(ice_tlan_ctx, drop_ena,			1,	165),
 	ICE_CTX_STORE(ice_tlan_ctx, cache_prof_idx,		2,	166),
 	ICE_CTX_STORE(ice_tlan_ctx, pkt_shaper_prof_idx,	3,	168),
-	ICE_CTX_STORE(ice_tlan_ctx, int_q_state,		122,	171),
 	{ 0 }
 };
 

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 5/9] ice: use structures to keep track of queue context size

[Jacob Keller]

The ice Tx and Rx queue context are currently stored as arrays of bytes
with defined size (ICE_RXQ_CTX_SZ and ICE_TXQ_CTX_SZ). The packed queue
context is often passed to other functions as a simple u8 * pointer, which
does not allow tracking the size. This makes the queue context API easy to
misuse, as you can pass an arbitrary u8 array or pointer.

Introduce wrapper typedefs which use a __packed structure that has the
proper fixed size for the Tx and Rx context buffers. This enables the
compiler to track the size of the value and ensures that passing the wrong
buffer size will be detected by the compiler.

The existing APIs do not benefit much from this change, however the
wrapping structures will be used to simplify the arguments of new packing
functions based on the recently introduced pack_fields API.

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>
---
 drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 11 +++++++++--
 drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h  |  2 --
 drivers/net/ethernet/intel/ice/ice_base.c       |  2 +-
 drivers/net/ethernet/intel/ice/ice_common.c     | 24 +++++++++++-------------
 4 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 1f01f3501d6b..a0ec9c97c2d7 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -12,6 +12,13 @@
 #define ICE_AQC_TOPO_MAX_LEVEL_NUM	0x9
 #define ICE_AQ_SET_MAC_FRAME_SIZE_MAX	9728
 
+#define ICE_RXQ_CTX_SIZE_DWORDS		8
+#define ICE_RXQ_CTX_SZ			(ICE_RXQ_CTX_SIZE_DWORDS * sizeof(u32))
+#define ICE_TXQ_CTX_SZ			22
+
+typedef struct __packed { u8 buf[ICE_RXQ_CTX_SZ]; } ice_rxq_ctx_buf_t;
+typedef struct __packed { u8 buf[ICE_TXQ_CTX_SZ]; } ice_txq_ctx_buf_t;
+
 struct ice_aqc_generic {
 	__le32 param0;
 	__le32 param1;
@@ -2067,10 +2074,10 @@ struct ice_aqc_add_txqs_perq {
 	__le16 txq_id;
 	u8 rsvd[2];
 	__le32 q_teid;
-	u8 txq_ctx[22];
+	ice_txq_ctx_buf_t txq_ctx;
 	u8 rsvd2[2];
 	struct ice_aqc_txsched_elem info;
-};
+} __packed;
 
 /* The format of the command buffer for Add Tx LAN Queues (0x0C30)
  * is an array of the following structs. Please note that the length of
diff --git a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
index 0e8ed8c226e6..a76e5b0e7861 100644
--- a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
+++ b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
@@ -371,8 +371,6 @@ enum ice_rx_flex_desc_status_error_1_bits {
 	ICE_RX_FLEX_DESC_STATUS1_LAST /* this entry must be last!!! */
 };
 
-#define ICE_RXQ_CTX_SIZE_DWORDS		8
-#define ICE_RXQ_CTX_SZ			(ICE_RXQ_CTX_SIZE_DWORDS * sizeof(u32))
 #define ICE_TX_CMPLTNQ_CTX_SIZE_DWORDS	22
 #define ICE_TX_DRBELL_Q_CTX_SIZE_DWORDS	5
 #define GLTCLAN_CQ_CNTX(i, CQ)		(GLTCLAN_CQ_CNTX0(CQ) + ((i) * 0x0800))
diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 3a8e156d7d86..260942877968 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -909,7 +909,7 @@ ice_vsi_cfg_txq(struct ice_vsi *vsi, struct ice_tx_ring *ring,
 	ice_setup_tx_ctx(ring, &tlan_ctx, pf_q);
 	/* copy context contents into the qg_buf */
 	qg_buf->txqs[0].txq_id = cpu_to_le16(pf_q);
-	ice_set_ctx(hw, (u8 *)&tlan_ctx, qg_buf->txqs[0].txq_ctx,
+	ice_set_ctx(hw, (u8 *)&tlan_ctx, (u8 *)&qg_buf->txqs[0].txq_ctx,
 		    ice_tlan_ctx_info);
 
 	/* init queue specific tail reg. It is referred as
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 0f5a80269a7b..48d95cb49864 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1359,29 +1359,27 @@ int ice_reset(struct ice_hw *hw, enum ice_reset_req req)
 /**
  * ice_copy_rxq_ctx_to_hw
  * @hw: pointer to the hardware structure
- * @ice_rxq_ctx: pointer to the rxq context
+ * @rxq_ctx: pointer to the packed Rx queue context
  * @rxq_index: the index of the Rx queue
  *
  * Copies rxq context from dense structure to HW register space
  */
-static int
-ice_copy_rxq_ctx_to_hw(struct ice_hw *hw, u8 *ice_rxq_ctx, u32 rxq_index)
+static int ice_copy_rxq_ctx_to_hw(struct ice_hw *hw,
+				  const ice_rxq_ctx_buf_t *rxq_ctx,
+				  u32 rxq_index)
 {
 	u8 i;
 
-	if (!ice_rxq_ctx)
-		return -EINVAL;
-
 	if (rxq_index > QRX_CTRL_MAX_INDEX)
 		return -EINVAL;
 
 	/* Copy each dword separately to HW */
 	for (i = 0; i < ICE_RXQ_CTX_SIZE_DWORDS; i++) {
-		wr32(hw, QRX_CONTEXT(i, rxq_index),
-		     *((u32 *)(ice_rxq_ctx + (i * sizeof(u32)))));
+		u32 ctx = ((const u32 *)rxq_ctx)[i];
 
-		ice_debug(hw, ICE_DBG_QCTX, "qrxdata[%d]: %08X\n", i,
-			  *((u32 *)(ice_rxq_ctx + (i * sizeof(u32)))));
+		wr32(hw, QRX_CONTEXT(i, rxq_index), ctx);
+
+		ice_debug(hw, ICE_DBG_QCTX, "qrxdata[%d]: %08X\n", i, ctx);
 	}
 
 	return 0;
@@ -1426,15 +1424,15 @@ static const struct ice_ctx_ele ice_rlan_ctx_info[] = {
 int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 		      u32 rxq_index)
 {
-	u8 ctx_buf[ICE_RXQ_CTX_SZ] = { 0 };
+	ice_rxq_ctx_buf_t buf = {};
 
 	if (!rlan_ctx)
 		return -EINVAL;
 
 	rlan_ctx->prefena = 1;
 
-	ice_set_ctx(hw, (u8 *)rlan_ctx, ctx_buf, ice_rlan_ctx_info);
-	return ice_copy_rxq_ctx_to_hw(hw, ctx_buf, rxq_index);
+	ice_set_ctx(hw, (u8 *)rlan_ctx, (u8 *)&buf, ice_rlan_ctx_info);
+	return ice_copy_rxq_ctx_to_hw(hw, &buf, rxq_index);
 }
 
 /* LAN Tx Queue Context */

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 6/9] ice: use <linux/packing.h> for Tx and Rx queue context data

[Jacob Keller]

The ice driver needs to write the Tx and Rx queue context when programming
Tx and Rx queues. This is currently done using some bespoke custom logic
via the ice_set_ctx() and its helper functions, along with bit position
definitions in the ice_tlan_ctx_info and ice_rlan_ctx_info structures.

This logic does work, but is problematic for several reasons:

1) ice_set_ctx requires a helper function for each byte size being packed,
   as it uses a separate function to pack u8, u16, u32, and u64 fields.
   This requires 4 functions which contain near-duplicate logic with the
   types changed out.

2) The logic in the ice_pack_ctx_word, ice_pack_ctx_dword, and
   ice_pack_ctx_qword does not handle values which straddle alignment
   boundaries very well. This requires that several fields in the
   ice_tlan_ctx_info and ice_rlan_ctx_info be a size larger than their bit
   size should require.

3) Future support for live migration will require adding unpacking
   functions to take the packed hardware context and unpack it into the
   ice_rlan_ctx and ice_tlan_ctx structures. Implementing this would
   require implementing ice_get_ctx, and its associated helper functions,
   which essentially doubles the amount of code required.

The Linux kernel has had a packing library that can handle this logic since
commit 554aae35007e ("lib: Add support for generic packing operations").
The library was recently extended with support for packing or unpacking an
array of fields, with a similar structure as the ice_ctx_ele structure.

Replace the ice-specific ice_set_ctx() logic with the recently added
pack_fields and packed_field_s infrastructure from <linux/packing.h>

For API simplicity, the Tx and Rx queue context are programmed using
separate ice_pack_txq_ctx() and ice_pack_rxq_ctx(). This avoids needing to
export the packed_field_s arrays. The functions can pointers to the
appropriate ice_txq_ctx_buf_t and ice_rxq_ctx_buf_t types, ensuring that
only buffers of the appropriate size are passed.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.h    |   5 +-
 drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h |  14 --
 drivers/net/ethernet/intel/ice/ice_base.c      |   3 +-
 drivers/net/ethernet/intel/ice/ice_common.c    | 247 +++++--------------------
 drivers/net/ethernet/intel/Kconfig             |   1 +
 5 files changed, 46 insertions(+), 224 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 27208a60cece..a68bea3934e3 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -92,9 +92,8 @@ ice_aq_set_rss_key(struct ice_hw *hw, u16 vsi_handle,
 bool ice_check_sq_alive(struct ice_hw *hw, struct ice_ctl_q_info *cq);
 int ice_aq_q_shutdown(struct ice_hw *hw, bool unloading);
 void ice_fill_dflt_direct_cmd_desc(struct ice_aq_desc *desc, u16 opcode);
-extern const struct ice_ctx_ele ice_tlan_ctx_info[];
-int ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx,
-		const struct ice_ctx_ele *ce_info);
+
+void ice_pack_txq_ctx(const struct ice_tlan_ctx *ctx, ice_txq_ctx_buf_t *buf);
 
 extern struct mutex ice_global_cfg_lock_sw;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
index a76e5b0e7861..31d4a445d640 100644
--- a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
+++ b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
@@ -408,20 +408,6 @@ struct ice_rlan_ctx {
 	u8 prefena;	/* NOTE: normally must be set to 1 at init */
 };
 
-struct ice_ctx_ele {
-	u16 offset;
-	u16 size_of;
-	u16 width;
-	u16 lsb;
-};
-
-#define ICE_CTX_STORE(_struct, _ele, _width, _lsb) {	\
-	.offset = offsetof(struct _struct, _ele),	\
-	.size_of = sizeof_field(struct _struct, _ele),	\
-	.width = _width,				\
-	.lsb = _lsb,					\
-}
-
 /* for hsplit_0 field of Rx RLAN context */
 enum ice_rlan_ctx_rx_hsplit_0 {
 	ICE_RLAN_RX_HSPLIT_0_NO_SPLIT		= 0,
diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 260942877968..0a325dec804e 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -909,8 +909,7 @@ ice_vsi_cfg_txq(struct ice_vsi *vsi, struct ice_tx_ring *ring,
 	ice_setup_tx_ctx(ring, &tlan_ctx, pf_q);
 	/* copy context contents into the qg_buf */
 	qg_buf->txqs[0].txq_id = cpu_to_le16(pf_q);
-	ice_set_ctx(hw, (u8 *)&tlan_ctx, (u8 *)&qg_buf->txqs[0].txq_ctx,
-		    ice_tlan_ctx_info);
+	ice_pack_txq_ctx(&tlan_ctx, &qg_buf->txqs[0].txq_ctx);
 
 	/* init queue specific tail reg. It is referred as
 	 * transmit comm scheduler queue doorbell.
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 48d95cb49864..8d32a751868e 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -6,6 +6,7 @@
 #include "ice_adminq_cmd.h"
 #include "ice_flow.h"
 #include "ice_ptp_hw.h"
+#include <linux/packing.h>
 
 #define ICE_PF_RESET_WAIT_COUNT	300
 #define ICE_MAX_NETLIST_SIZE	10
@@ -1385,9 +1386,12 @@ static int ice_copy_rxq_ctx_to_hw(struct ice_hw *hw,
 	return 0;
 }
 
+#define ICE_CTX_STORE(struct_name, struct_field, width, lsb) \
+	PACKED_FIELD((lsb) + (width) - 1, (lsb), struct struct_name, struct_field)
+
 /* LAN Rx Queue Context */
-static const struct ice_ctx_ele ice_rlan_ctx_info[] = {
-	/* Field		Width	LSB */
+DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ) = {
+				 /* Field		Width	LSB */
 	ICE_CTX_STORE(ice_rlan_ctx, head,		13,	0),
 	ICE_CTX_STORE(ice_rlan_ctx, cpuid,		8,	13),
 	ICE_CTX_STORE(ice_rlan_ctx, base,		57,	32),
@@ -1408,9 +1412,25 @@ static const struct ice_ctx_ele ice_rlan_ctx_info[] = {
 	ICE_CTX_STORE(ice_rlan_ctx, tphhead_ena,	1,	196),
 	ICE_CTX_STORE(ice_rlan_ctx, lrxqthresh,		3,	198),
 	ICE_CTX_STORE(ice_rlan_ctx, prefena,		1,	201),
-	{ 0 }
 };
 
+/**
+ * ice_pack_rxq_ctx - Pack Rx queue context into a HW buffer
+ * @ctx: the Rx queue context to pack
+ * @buf: the HW buffer to pack into
+ *
+ * Pack the Rx queue context from the CPU-friendly unpacked buffer into its
+ * bit-packed HW layout.
+ */
+static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
+			     ice_rxq_ctx_buf_t *buf)
+{
+	BUILD_BUG_ON(sizeof(*buf) != ICE_RXQ_CTX_SZ);
+
+	pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
+		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
+}
+
 /**
  * ice_write_rxq_ctx
  * @hw: pointer to the hardware structure
@@ -1431,12 +1451,13 @@ int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 
 	rlan_ctx->prefena = 1;
 
-	ice_set_ctx(hw, (u8 *)rlan_ctx, (u8 *)&buf, ice_rlan_ctx_info);
+	ice_pack_rxq_ctx(rlan_ctx, &buf);
+
 	return ice_copy_rxq_ctx_to_hw(hw, &buf, rxq_index);
 }
 
 /* LAN Tx Queue Context */
-const struct ice_ctx_ele ice_tlan_ctx_info[] = {
+DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ) = {
 				    /* Field			Width	LSB */
 	ICE_CTX_STORE(ice_tlan_ctx, base,			57,	0),
 	ICE_CTX_STORE(ice_tlan_ctx, port_num,			3,	57),
@@ -1465,9 +1486,24 @@ const struct ice_ctx_ele ice_tlan_ctx_info[] = {
 	ICE_CTX_STORE(ice_tlan_ctx, drop_ena,			1,	165),
 	ICE_CTX_STORE(ice_tlan_ctx, cache_prof_idx,		2,	166),
 	ICE_CTX_STORE(ice_tlan_ctx, pkt_shaper_prof_idx,	3,	168),
-	{ 0 }
 };
 
+/**
+ * ice_pack_txq_ctx - Pack Tx queue context into a HW buffer
+ * @ctx: the Tx queue context to pack
+ * @buf: the HW buffer to pack into
+ *
+ * Pack the Tx queue context from the CPU-friendly unpacked buffer into its
+ * bit-packed HW layout.
+ */
+void ice_pack_txq_ctx(const struct ice_tlan_ctx *ctx, ice_txq_ctx_buf_t *buf)
+{
+	BUILD_BUG_ON(sizeof(*buf) != ICE_TXQ_CTX_SZ);
+
+	pack_fields(buf, sizeof(*buf), ctx, ice_tlan_ctx_fields,
+		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
+}
+
 /* Sideband Queue command wrappers */
 
 /**
@@ -4545,205 +4581,6 @@ ice_aq_add_rdma_qsets(struct ice_hw *hw, u8 num_qset_grps,
 
 /* End of FW Admin Queue command wrappers */
 
-/**
- * ice_pack_ctx_byte - write a byte to a packed context structure
- * @src_ctx: unpacked source context structure
- * @dest_ctx: packed destination context data
- * @ce_info: context element description
- */
-static void ice_pack_ctx_byte(u8 *src_ctx, u8 *dest_ctx,
-			      const struct ice_ctx_ele *ce_info)
-{
-	u8 src_byte, dest_byte, mask;
-	u8 *from, *dest;
-	u16 shift_width;
-
-	/* copy from the next struct field */
-	from = src_ctx + ce_info->offset;
-
-	/* prepare the bits and mask */
-	shift_width = ce_info->lsb % 8;
-	mask = GENMASK(ce_info->width - 1 + shift_width, shift_width);
-
-	src_byte = *from;
-	src_byte <<= shift_width;
-	src_byte &= mask;
-
-	/* get the current bits from the target bit string */
-	dest = dest_ctx + (ce_info->lsb / 8);
-
-	memcpy(&dest_byte, dest, sizeof(dest_byte));
-
-	dest_byte &= ~mask;	/* get the bits not changing */
-	dest_byte |= src_byte;	/* add in the new bits */
-
-	/* put it all back */
-	memcpy(dest, &dest_byte, sizeof(dest_byte));
-}
-
-/**
- * ice_pack_ctx_word - write a word to a packed context structure
- * @src_ctx: unpacked source context structure
- * @dest_ctx: packed destination context data
- * @ce_info: context element description
- */
-static void ice_pack_ctx_word(u8 *src_ctx, u8 *dest_ctx,
-			      const struct ice_ctx_ele *ce_info)
-{
-	u16 src_word, mask;
-	__le16 dest_word;
-	u8 *from, *dest;
-	u16 shift_width;
-
-	/* copy from the next struct field */
-	from = src_ctx + ce_info->offset;
-
-	/* prepare the bits and mask */
-	shift_width = ce_info->lsb % 8;
-	mask = GENMASK(ce_info->width - 1 + shift_width, shift_width);
-
-	/* don't swizzle the bits until after the mask because the mask bits
-	 * will be in a different bit position on big endian machines
-	 */
-	src_word = *(u16 *)from;
-	src_word <<= shift_width;
-	src_word &= mask;
-
-	/* get the current bits from the target bit string */
-	dest = dest_ctx + (ce_info->lsb / 8);
-
-	memcpy(&dest_word, dest, sizeof(dest_word));
-
-	dest_word &= ~(cpu_to_le16(mask));	/* get the bits not changing */
-	dest_word |= cpu_to_le16(src_word);	/* add in the new bits */
-
-	/* put it all back */
-	memcpy(dest, &dest_word, sizeof(dest_word));
-}
-
-/**
- * ice_pack_ctx_dword - write a dword to a packed context structure
- * @src_ctx: unpacked source context structure
- * @dest_ctx: packed destination context data
- * @ce_info: context element description
- */
-static void ice_pack_ctx_dword(u8 *src_ctx, u8 *dest_ctx,
-			       const struct ice_ctx_ele *ce_info)
-{
-	u32 src_dword, mask;
-	__le32 dest_dword;
-	u8 *from, *dest;
-	u16 shift_width;
-
-	/* copy from the next struct field */
-	from = src_ctx + ce_info->offset;
-
-	/* prepare the bits and mask */
-	shift_width = ce_info->lsb % 8;
-	mask = GENMASK(ce_info->width - 1 + shift_width, shift_width);
-
-	/* don't swizzle the bits until after the mask because the mask bits
-	 * will be in a different bit position on big endian machines
-	 */
-	src_dword = *(u32 *)from;
-	src_dword <<= shift_width;
-	src_dword &= mask;
-
-	/* get the current bits from the target bit string */
-	dest = dest_ctx + (ce_info->lsb / 8);
-
-	memcpy(&dest_dword, dest, sizeof(dest_dword));
-
-	dest_dword &= ~(cpu_to_le32(mask));	/* get the bits not changing */
-	dest_dword |= cpu_to_le32(src_dword);	/* add in the new bits */
-
-	/* put it all back */
-	memcpy(dest, &dest_dword, sizeof(dest_dword));
-}
-
-/**
- * ice_pack_ctx_qword - write a qword to a packed context structure
- * @src_ctx: unpacked source context structure
- * @dest_ctx: packed destination context data
- * @ce_info: context element description
- */
-static void ice_pack_ctx_qword(u8 *src_ctx, u8 *dest_ctx,
-			       const struct ice_ctx_ele *ce_info)
-{
-	u64 src_qword, mask;
-	__le64 dest_qword;
-	u8 *from, *dest;
-	u16 shift_width;
-
-	/* copy from the next struct field */
-	from = src_ctx + ce_info->offset;
-
-	/* prepare the bits and mask */
-	shift_width = ce_info->lsb % 8;
-	mask = GENMASK_ULL(ce_info->width - 1 + shift_width, shift_width);
-
-	/* don't swizzle the bits until after the mask because the mask bits
-	 * will be in a different bit position on big endian machines
-	 */
-	src_qword = *(u64 *)from;
-	src_qword <<= shift_width;
-	src_qword &= mask;
-
-	/* get the current bits from the target bit string */
-	dest = dest_ctx + (ce_info->lsb / 8);
-
-	memcpy(&dest_qword, dest, sizeof(dest_qword));
-
-	dest_qword &= ~(cpu_to_le64(mask));	/* get the bits not changing */
-	dest_qword |= cpu_to_le64(src_qword);	/* add in the new bits */
-
-	/* put it all back */
-	memcpy(dest, &dest_qword, sizeof(dest_qword));
-}
-
-/**
- * ice_set_ctx - set context bits in packed structure
- * @hw: pointer to the hardware structure
- * @src_ctx:  pointer to a generic non-packed context structure
- * @dest_ctx: pointer to memory for the packed structure
- * @ce_info: List of Rx context elements
- */
-int ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx,
-		const struct ice_ctx_ele *ce_info)
-{
-	int f;
-
-	for (f = 0; ce_info[f].width; f++) {
-		/* We have to deal with each element of the FW response
-		 * using the correct size so that we are correct regardless
-		 * of the endianness of the machine.
-		 */
-		if (ce_info[f].width > (ce_info[f].size_of * BITS_PER_BYTE)) {
-			ice_debug(hw, ICE_DBG_QCTX, "Field %d width of %d bits larger than size of %d byte(s) ... skipping write\n",
-				  f, ce_info[f].width, ce_info[f].size_of);
-			continue;
-		}
-		switch (ce_info[f].size_of) {
-		case sizeof(u8):
-			ice_pack_ctx_byte(src_ctx, dest_ctx, &ce_info[f]);
-			break;
-		case sizeof(u16):
-			ice_pack_ctx_word(src_ctx, dest_ctx, &ce_info[f]);
-			break;
-		case sizeof(u32):
-			ice_pack_ctx_dword(src_ctx, dest_ctx, &ce_info[f]);
-			break;
-		case sizeof(u64):
-			ice_pack_ctx_qword(src_ctx, dest_ctx, &ce_info[f]);
-			break;
-		default:
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
 /**
  * ice_get_lan_q_ctx - get the LAN queue context for the given VSI and TC
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 20bc40eec487..24ec9a4f1ffa 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -292,6 +292,7 @@ config ICE
 	select DIMLIB
 	select LIBIE
 	select NET_DEVLINK
+	select PACKING
 	select PLDMFW
 	select DPLL
 	help

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 7/9] ice: reduce size of queue context fields

[Jacob Keller]

The ice_rlan_ctx and ice_tlan_ctx structures have some fields which are
intentionally sized larger necessary relative to the packed sizes the data
must fit into. This was done because the original ice_set_ctx() function
and its helpers did not correctly handle packing when the packed bits
straddled a byte. This is no longer the case with the use of the
<linux/packing.h> implementation.

Save some bytes in these structures by sizing the variables to the number
of bytes the actual bitpacked fields fit into.

There are a couple of gaps left in the structure, which is a result of the
fields being in the order they appear in the packed bit layout, but where
alignment forces some extra gaps. We could fix this, saving ~8 bytes from
each structure. However, these structures are not used heavily, and the
resulting savings is minimal:

$ bloat-o-meter ice-before-reorder.ko ice-after-reorder.ko
add/remove: 0/0 grow/shrink: 1/1 up/down: 26/-70 (-44)
Function                                     old     new   delta
ice_vsi_cfg_txq                             1873    1899     +26
ice_setup_rx_ctx.constprop                  1529    1459     -70
Total: Before=1459555, After=1459511, chg -0.00%

Thus, the fields are left in the same order as the packed bit layout,
despite the gaps this causes.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h | 32 ++++++++------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
index 31d4a445d640..1479b45738af 100644
--- a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
+++ b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
@@ -375,23 +375,17 @@ enum ice_rx_flex_desc_status_error_1_bits {
 #define ICE_TX_DRBELL_Q_CTX_SIZE_DWORDS	5
 #define GLTCLAN_CQ_CNTX(i, CQ)		(GLTCLAN_CQ_CNTX0(CQ) + ((i) * 0x0800))
 
-/* RLAN Rx queue context data
- *
- * The sizes of the variables may be larger than needed due to crossing byte
- * boundaries. If we do not have the width of the variable set to the correct
- * size then we could end up shifting bits off the top of the variable when the
- * variable is at the top of a byte and crosses over into the next byte.
- */
+/* RLAN Rx queue context data */
 struct ice_rlan_ctx {
 	u16 head;
-	u16 cpuid; /* bigger than needed, see above for reason */
+	u8 cpuid;
 #define ICE_RLAN_BASE_S 7
 	u64 base;
 	u16 qlen;
 #define ICE_RLAN_CTX_DBUF_S 7
-	u16 dbuf; /* bigger than needed, see above for reason */
+	u8 dbuf;
 #define ICE_RLAN_CTX_HBUF_S 6
-	u16 hbuf; /* bigger than needed, see above for reason */
+	u8 hbuf;
 	u8 dtype;
 	u8 dsize;
 	u8 crcstrip;
@@ -399,12 +393,12 @@ struct ice_rlan_ctx {
 	u8 hsplit_0;
 	u8 hsplit_1;
 	u8 showiv;
-	u32 rxmax; /* bigger than needed, see above for reason */
+	u16 rxmax;
 	u8 tphrdesc_ena;
 	u8 tphwdesc_ena;
 	u8 tphdata_ena;
 	u8 tphhead_ena;
-	u16 lrxqthresh; /* bigger than needed, see above for reason */
+	u8 lrxqthresh;
 	u8 prefena;	/* NOTE: normally must be set to 1 at init */
 };
 
@@ -535,18 +529,12 @@ enum ice_tx_ctx_desc_eipt_offload {
 #define ICE_LAN_TXQ_MAX_QGRPS	127
 #define ICE_LAN_TXQ_MAX_QDIS	1023
 
-/* Tx queue context data
- *
- * The sizes of the variables may be larger than needed due to crossing byte
- * boundaries. If we do not have the width of the variable set to the correct
- * size then we could end up shifting bits off the top of the variable when the
- * variable is at the top of a byte and crosses over into the next byte.
- */
+/* Tx queue context data */
 struct ice_tlan_ctx {
 #define ICE_TLAN_CTX_BASE_S	7
 	u64 base;		/* base is defined in 128-byte units */
 	u8 port_num;
-	u16 cgd_num;		/* bigger than needed, see above for reason */
+	u8 cgd_num;
 	u8 pf_num;
 	u16 vmvf_num;
 	u8 vmvf_type;
@@ -557,7 +545,7 @@ struct ice_tlan_ctx {
 	u8 tsyn_ena;
 	u8 internal_usage_flag;
 	u8 alt_vlan;
-	u16 cpuid;		/* bigger than needed, see above for reason */
+	u8 cpuid;
 	u8 wb_mode;
 	u8 tphrd_desc;
 	u8 tphrd;
@@ -566,7 +554,7 @@ struct ice_tlan_ctx {
 	u16 qnum_in_func;
 	u8 itr_notification_mode;
 	u8 adjust_prof_id;
-	u32 qlen;		/* bigger than needed, see above for reason */
+	u16 qlen;
 	u8 quanta_prof_idx;
 	u8 tso_ena;
 	u16 tso_qnum;

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 8/9] ice: move prefetch enable to ice_setup_rx_ctx

[Jacob Keller]

The ice_write_rxq_ctx() function is responsible for programming the Rx
Queue context into hardware. It receives the configuration in unpacked form
via the ice_rlan_ctx structure.

This function unconditionally modifies the context to set the prefetch
enable bit. This was done by commit c31a5c25bb19 ("ice: Always set prefena
when configuring an Rx queue"). Setting this bit makes sense, since
prefetching descriptors is almost always the preferred behavior.

However, the ice_write_rxq_ctx() function is not the place that actually
defines the queue context. We initialize the Rx Queue context in
ice_setup_rx_ctx(). It is surprising to have the Rx queue context changed
by a function who's responsibility is to program the given context to
hardware.

Following the principle of least surprise, move the setting of the prefetch
enable bit out of ice_write_rxq_ctx() and into the ice_setup_rx_ctx().

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_base.c   | 3 +++
 drivers/net/ethernet/intel/ice/ice_common.c | 9 +++------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 0a325dec804e..f1fbba19e4e4 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -453,6 +453,9 @@ static int ice_setup_rx_ctx(struct ice_rx_ring *ring)
 	/* Rx queue threshold in units of 64 */
 	rlan_ctx.lrxqthresh = 1;
 
+	/* Enable descriptor prefetch */
+	rlan_ctx.prefena = 1;
+
 	/* PF acts as uplink for switchdev; set flex descriptor with src_vsi
 	 * metadata and flags to allow redirecting to PR netdev
 	 */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 8d32a751868e..c6a6dbc0e80b 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1432,14 +1432,13 @@ static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
 }
 
 /**
- * ice_write_rxq_ctx
+ * ice_write_rxq_ctx - Write Rx Queue context to hardware
  * @hw: pointer to the hardware structure
  * @rlan_ctx: pointer to the rxq context
  * @rxq_index: the index of the Rx queue
  *
- * Converts rxq context from sparse to dense structure and then writes
- * it to HW register space and enables the hardware to prefetch descriptors
- * instead of only fetching them on demand
+ * Pack the sparse Rx Queue context into dense hardware format and write it
+ * into the HW register space.
  */
 int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 		      u32 rxq_index)
@@ -1449,8 +1448,6 @@ int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 	if (!rlan_ctx)
 		return -EINVAL;
 
-	rlan_ctx->prefena = 1;
-
 	ice_pack_rxq_ctx(rlan_ctx, &buf);
 
 	return ice_copy_rxq_ctx_to_hw(hw, &buf, rxq_index);

-- 
2.47.0.265.g4ca455297942

[PATCH net-next v3 9/9] ice: cleanup Rx queue context programming functions

[Jacob Keller]

The ice_copy_rxq_ctx_to_hw() and ice_write_rxq_ctx() functions perform some
defensive checks which are typically frowned upon by kernel style
guidelines.

In particular, NULL checks on buffers which point to the stack are
discouraged, especially when the functions are static and only called once.
Checks of this sort only serve to hide potential programming error, as we
will not produce the normal crash dump on a NULL access.

In addition, ice_copy_rxq_ctx_to_hw() cannot fail in another way, so could
be made void.

Future support for VF Live Migration will need to introduce an inverse
function for reading Rx queue context from HW registers to unpack it, as
well as functions to pack and unpack Tx queue context from HW.

Rather than copying these style issues into the new functions, lets first
cleanup the existing code.

For the ice_copy_rxq_ctx_to_hw() function:

 * Move the Rx queue index check out of this function.
 * Convert the function to a void return.
 * Use a simple int variable instead of a u8 for the for loop index, and
   initialize it inside the for loop.
 * Update the function description to better align with kernel doc style.

For the ice_write_rxq_ctx() function:

 * Move the Rx queue index check into this function.
 * Update the function description with a Returns: to align with kernel doc
   style.

These changes align the existing write functions to current kernel
style, and will align with the style of the new functions added when we
implement live migration in a future series.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index c6a6dbc0e80b..98845d790791 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1358,32 +1358,23 @@ int ice_reset(struct ice_hw *hw, enum ice_reset_req req)
 }
 
 /**
- * ice_copy_rxq_ctx_to_hw
+ * ice_copy_rxq_ctx_to_hw - Copy packed Rx queue context to HW registers
  * @hw: pointer to the hardware structure
  * @rxq_ctx: pointer to the packed Rx queue context
  * @rxq_index: the index of the Rx queue
- *
- * Copies rxq context from dense structure to HW register space
  */
-static int ice_copy_rxq_ctx_to_hw(struct ice_hw *hw,
-				  const ice_rxq_ctx_buf_t *rxq_ctx,
-				  u32 rxq_index)
+static void ice_copy_rxq_ctx_to_hw(struct ice_hw *hw,
+				   const ice_rxq_ctx_buf_t *rxq_ctx,
+				   u32 rxq_index)
 {
-	u8 i;
-
-	if (rxq_index > QRX_CTRL_MAX_INDEX)
-		return -EINVAL;
-
 	/* Copy each dword separately to HW */
-	for (i = 0; i < ICE_RXQ_CTX_SIZE_DWORDS; i++) {
+	for (int i = 0; i < ICE_RXQ_CTX_SIZE_DWORDS; i++) {
 		u32 ctx = ((const u32 *)rxq_ctx)[i];
 
 		wr32(hw, QRX_CONTEXT(i, rxq_index), ctx);
 
 		ice_debug(hw, ICE_DBG_QCTX, "qrxdata[%d]: %08X\n", i, ctx);
 	}
-
-	return 0;
 }
 
 #define ICE_CTX_STORE(struct_name, struct_field, width, lsb) \
@@ -1434,23 +1425,26 @@ static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
 /**
  * ice_write_rxq_ctx - Write Rx Queue context to hardware
  * @hw: pointer to the hardware structure
- * @rlan_ctx: pointer to the rxq context
+ * @rlan_ctx: pointer to the unpacked Rx queue context
  * @rxq_index: the index of the Rx queue
  *
  * Pack the sparse Rx Queue context into dense hardware format and write it
  * into the HW register space.
+ *
+ * Return: 0 on success, or -EINVAL if the Rx queue index is invalid.
  */
 int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 		      u32 rxq_index)
 {
 	ice_rxq_ctx_buf_t buf = {};
 
-	if (!rlan_ctx)
+	if (rxq_index > QRX_CTRL_MAX_INDEX)
 		return -EINVAL;
 
 	ice_pack_rxq_ctx(rlan_ctx, &buf);
+	ice_copy_rxq_ctx_to_hw(hw, &buf, rxq_index);
 
-	return ice_copy_rxq_ctx_to_hw(hw, &buf, rxq_index);
+	return 0;
 }
 
 /* LAN Tx Queue Context */

-- 
2.47.0.265.g4ca455297942

Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[kernel test robot]

Hi Jacob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a84e8c05f58305dfa808bc5465c5175c29d7c9b6]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacob-Keller/lib-packing-create-__pack-and-__unpack-variants-without-error-checking/20241108-040154
base:   a84e8c05f58305dfa808bc5465c5175c29d7c9b6
patch link:    https://lore.kernel.org/r/20241107-packing-pack-fields-and-ice-implementation-v3-3-27c566ac2436%40intel.com
patch subject: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()
config: x86_64-randconfig-121-20241108 (https://download.01.org/0day-ci/archive/20241108/202411081548.EnYrguKQ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241108/202411081548.EnYrguKQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411081548.EnYrguKQ-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> lib/packing_test.c:412:1: sparse: sparse: symbol '__test_fields_buffer_sz' was not declared. Should it be static?
>> lib/packing_test.c:412:1: sparse: sparse: symbol 'test_fields' was not declared. Should it be static?

vim +/__test_fields_buffer_sz +412 lib/packing_test.c

   411	
 > 412	DECLARE_PACKED_FIELDS_S(test_fields, sizeof(packed_buf_t)) = {
   413		PACKED_FIELD(63, 61, struct test_data, field1),
   414		PACKED_FIELD(60, 52, struct test_data, field2),
   415		PACKED_FIELD(51, 28, struct test_data, field3),
   416		PACKED_FIELD(27, 14, struct test_data, field4),
   417		PACKED_FIELD(13, 9, struct test_data, field5),
   418		PACKED_FIELD(8, 0, struct test_data, field6),
   419	};
   420	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v3 6/9] ice: use <linux/packing.h> for Tx and Rx queue context data

[kernel test robot]

Hi Jacob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a84e8c05f58305dfa808bc5465c5175c29d7c9b6]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacob-Keller/lib-packing-create-__pack-and-__unpack-variants-without-error-checking/20241108-040154
base:   a84e8c05f58305dfa808bc5465c5175c29d7c9b6
patch link:    https://lore.kernel.org/r/20241107-packing-pack-fields-and-ice-implementation-v3-6-27c566ac2436%40intel.com
patch subject: [PATCH net-next v3 6/9] ice: use <linux/packing.h> for Tx and Rx queue context data
config: x86_64-randconfig-122-20241108 (https://download.01.org/0day-ci/archive/20241108/202411081511.T7pLZhW4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241108/202411081511.T7pLZhW4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411081511.T7pLZhW4-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/intel/ice/ice_common.c:1393:1: sparse: sparse: symbol '__ice_rlan_ctx_fields_buffer_sz' was not declared. Should it be static?
>> drivers/net/ethernet/intel/ice/ice_common.c:1393:1: sparse: sparse: symbol 'ice_rlan_ctx_fields' was not declared. Should it be static?
>> drivers/net/ethernet/intel/ice/ice_common.c:1460:1: sparse: sparse: symbol '__ice_tlan_ctx_fields_buffer_sz' was not declared. Should it be static?
>> drivers/net/ethernet/intel/ice/ice_common.c:1460:1: sparse: sparse: symbol 'ice_tlan_ctx_fields' was not declared. Should it be static?

vim +/__ice_rlan_ctx_fields_buffer_sz +1393 drivers/net/ethernet/intel/ice/ice_common.c

  1388	
  1389	#define ICE_CTX_STORE(struct_name, struct_field, width, lsb) \
  1390		PACKED_FIELD((lsb) + (width) - 1, (lsb), struct struct_name, struct_field)
  1391	
  1392	/* LAN Rx Queue Context */
> 1393	DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ) = {
  1394					 /* Field		Width	LSB */
  1395		ICE_CTX_STORE(ice_rlan_ctx, head,		13,	0),
  1396		ICE_CTX_STORE(ice_rlan_ctx, cpuid,		8,	13),
  1397		ICE_CTX_STORE(ice_rlan_ctx, base,		57,	32),
  1398		ICE_CTX_STORE(ice_rlan_ctx, qlen,		13,	89),
  1399		ICE_CTX_STORE(ice_rlan_ctx, dbuf,		7,	102),
  1400		ICE_CTX_STORE(ice_rlan_ctx, hbuf,		5,	109),
  1401		ICE_CTX_STORE(ice_rlan_ctx, dtype,		2,	114),
  1402		ICE_CTX_STORE(ice_rlan_ctx, dsize,		1,	116),
  1403		ICE_CTX_STORE(ice_rlan_ctx, crcstrip,		1,	117),
  1404		ICE_CTX_STORE(ice_rlan_ctx, l2tsel,		1,	119),
  1405		ICE_CTX_STORE(ice_rlan_ctx, hsplit_0,		4,	120),
  1406		ICE_CTX_STORE(ice_rlan_ctx, hsplit_1,		2,	124),
  1407		ICE_CTX_STORE(ice_rlan_ctx, showiv,		1,	127),
  1408		ICE_CTX_STORE(ice_rlan_ctx, rxmax,		14,	174),
  1409		ICE_CTX_STORE(ice_rlan_ctx, tphrdesc_ena,	1,	193),
  1410		ICE_CTX_STORE(ice_rlan_ctx, tphwdesc_ena,	1,	194),
  1411		ICE_CTX_STORE(ice_rlan_ctx, tphdata_ena,	1,	195),
  1412		ICE_CTX_STORE(ice_rlan_ctx, tphhead_ena,	1,	196),
  1413		ICE_CTX_STORE(ice_rlan_ctx, lrxqthresh,		3,	198),
  1414		ICE_CTX_STORE(ice_rlan_ctx, prefena,		1,	201),
  1415	};
  1416	
  1417	/**
  1418	 * ice_pack_rxq_ctx - Pack Rx queue context into a HW buffer
  1419	 * @ctx: the Rx queue context to pack
  1420	 * @buf: the HW buffer to pack into
  1421	 *
  1422	 * Pack the Rx queue context from the CPU-friendly unpacked buffer into its
  1423	 * bit-packed HW layout.
  1424	 */
  1425	static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
  1426				     ice_rxq_ctx_buf_t *buf)
  1427	{
  1428		BUILD_BUG_ON(sizeof(*buf) != ICE_RXQ_CTX_SZ);
  1429	
  1430		pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
  1431			    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
  1432	}
  1433	
  1434	/**
  1435	 * ice_write_rxq_ctx
  1436	 * @hw: pointer to the hardware structure
  1437	 * @rlan_ctx: pointer to the rxq context
  1438	 * @rxq_index: the index of the Rx queue
  1439	 *
  1440	 * Converts rxq context from sparse to dense structure and then writes
  1441	 * it to HW register space and enables the hardware to prefetch descriptors
  1442	 * instead of only fetching them on demand
  1443	 */
  1444	int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
  1445			      u32 rxq_index)
  1446	{
  1447		ice_rxq_ctx_buf_t buf = {};
  1448	
  1449		if (!rlan_ctx)
  1450			return -EINVAL;
  1451	
  1452		rlan_ctx->prefena = 1;
  1453	
  1454		ice_pack_rxq_ctx(rlan_ctx, &buf);
  1455	
  1456		return ice_copy_rxq_ctx_to_hw(hw, &buf, rxq_index);
  1457	}
  1458	
  1459	/* LAN Tx Queue Context */
> 1460	DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ) = {
  1461					    /* Field			Width	LSB */
  1462		ICE_CTX_STORE(ice_tlan_ctx, base,			57,	0),
  1463		ICE_CTX_STORE(ice_tlan_ctx, port_num,			3,	57),
  1464		ICE_CTX_STORE(ice_tlan_ctx, cgd_num,			5,	60),
  1465		ICE_CTX_STORE(ice_tlan_ctx, pf_num,			3,	65),
  1466		ICE_CTX_STORE(ice_tlan_ctx, vmvf_num,			10,	68),
  1467		ICE_CTX_STORE(ice_tlan_ctx, vmvf_type,			2,	78),
  1468		ICE_CTX_STORE(ice_tlan_ctx, src_vsi,			10,	80),
  1469		ICE_CTX_STORE(ice_tlan_ctx, tsyn_ena,			1,	90),
  1470		ICE_CTX_STORE(ice_tlan_ctx, internal_usage_flag,	1,	91),
  1471		ICE_CTX_STORE(ice_tlan_ctx, alt_vlan,			1,	92),
  1472		ICE_CTX_STORE(ice_tlan_ctx, cpuid,			8,	93),
  1473		ICE_CTX_STORE(ice_tlan_ctx, wb_mode,			1,	101),
  1474		ICE_CTX_STORE(ice_tlan_ctx, tphrd_desc,			1,	102),
  1475		ICE_CTX_STORE(ice_tlan_ctx, tphrd,			1,	103),
  1476		ICE_CTX_STORE(ice_tlan_ctx, tphwr_desc,			1,	104),
  1477		ICE_CTX_STORE(ice_tlan_ctx, cmpq_id,			9,	105),
  1478		ICE_CTX_STORE(ice_tlan_ctx, qnum_in_func,		14,	114),
  1479		ICE_CTX_STORE(ice_tlan_ctx, itr_notification_mode,	1,	128),
  1480		ICE_CTX_STORE(ice_tlan_ctx, adjust_prof_id,		6,	129),
  1481		ICE_CTX_STORE(ice_tlan_ctx, qlen,			13,	135),
  1482		ICE_CTX_STORE(ice_tlan_ctx, quanta_prof_idx,		4,	148),
  1483		ICE_CTX_STORE(ice_tlan_ctx, tso_ena,			1,	152),
  1484		ICE_CTX_STORE(ice_tlan_ctx, tso_qnum,			11,	153),
  1485		ICE_CTX_STORE(ice_tlan_ctx, legacy_int,			1,	164),
  1486		ICE_CTX_STORE(ice_tlan_ctx, drop_ena,			1,	165),
  1487		ICE_CTX_STORE(ice_tlan_ctx, cache_prof_idx,		2,	166),
  1488		ICE_CTX_STORE(ice_tlan_ctx, pkt_shaper_prof_idx,	3,	168),
  1489	};
  1490	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[Vladimir Oltean]

On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
> +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
> +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
> +	const struct packed_field_s name[] __pf_section_s

This will need sorting out - how to make this declaration macro
compatible with the "static" keyword. The obvious way (to group the
field array and the buffer size into a structure) doesn't work. It loses
the ARRAY_SIZE() of the fields, which is important to the pack_fields()
and unpack_fields() wrappers.

Maybe a different tool is needed for checking that no packed fields
exceed the buffer size? Forcing the buffer size be a symbol just because
that's what modpost works with seems unnatural.

If we knew the position of the largest field array element in C, and if
we enforced that all pack_fields() use a strong type (size included) for
the packed buffer, we'd have all information inside the pack_fields()
macro, because we only need to compare the largest field against the
buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
inside the pack_fields() macro itself. And have the compile-time checks
spill over between C and modpost...

Not to mention, pack_fields() would have one argument less: pbuflen.

> +
> +/* Medium packed field. Use with bit offsets < 65536, buffers < 8KB and
> + * unpacked structures < 64KB.
> + */
> +struct packed_field_m {
> +	GEN_PACKED_FIELD_MEMBERS(u16);
> +};
> +
> +#define __pf_section_m __section(".rodata.packed_fields_m")
> +
> +#define DECLARE_PACKED_FIELDS_M(name, buffer_sz) \
> +	const size_t __ ## name ## _buffer_sz __pf_section_m = buffer_sz; \
> +	const struct packed_field_m name[] __pf_section_m
> +
> +#define PACKED_FIELD(start, end, struct_name, struct_field) \
> +{ \
> +	(start), \
> +	(end), \
> +	offsetof(struct_name, struct_field), \
> +	sizeof_field(struct_name, struct_field), \
> +}
> +
> +#endif
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index ada3a36cc4bc..013bf4be2642 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -160,6 +160,19 @@ static inline bool is_valid_name(struct elf_info *elf, Elf_Sym *sym)
>  	return !is_mapping_symbol(name);
>  }
>  
> +/* This is based on the hash algorithm from gdbm, via tdb */
> +static inline unsigned int tdb_hash(const char *name)
> +{
> +	unsigned value;	/* Used to compute the hash value.  */
> +	unsigned   i;	/* Used to cycle through random values. */
> +
> +	/* Set the initial value from the key size. */
> +	for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
> +		value = (value + (((unsigned char *)name)[i] << (i*5 % 24)));
> +
> +	return (1103515243 * value + 12345);
> +}
> +

Code movement should be in separate changes.

>  /* symsearch.c */
>  void symsearch_init(struct elf_info *elf);
>  void symsearch_finish(struct elf_info *elf);
> @@ -175,12 +188,18 @@ void add_moddevtable(struct buffer *buf, struct module *mod);
>  /* sumversion.c */
>  void get_src_version(const char *modname, char sum[], unsigned sumlen);
>  
> +/* packed_fields.c */
> +void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
> +				Elf_Sym *sym, const char *symname);
> +void check_packed_field_symbols(void);
> +
>  /* from modpost.c */
>  extern bool target_is_big_endian;
>  extern bool host_is_big_endian;
>  char *read_text_file(const char *filename);
>  char *get_line(char **stringp);
>  void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym);
> +const char *sec_name(const struct elf_info *info, unsigned int secindex);
>  
>  void __attribute__((format(printf, 2, 3)))
>  modpost_log(bool is_error, const char *fmt, ...);
> diff --git a/lib/packing.c b/lib/packing.c
> index 2bf81951dfc8..45164f73fe5b 100644
> --- a/lib/packing.c
> +++ b/lib/packing.c
> @@ -5,10 +5,37 @@
>  #include <linux/packing.h>
>  #include <linux/module.h>
>  #include <linux/bitops.h>
> +#include <linux/bits.h>
>  #include <linux/errno.h>
>  #include <linux/types.h>
>  #include <linux/bitrev.h>
>  
> +#define __pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks)	\
> +	({									\
> +		for (size_t i = 0; i < (num_fields); i++) {			\
> +			typeof(&(fields)[0]) field = &(fields)[i];		\
> +			u64 uval;						\
> +										\
> +			uval = ustruct_field_to_u64(ustruct, field->offset, field->size); \
> +										\
> +			__pack(pbuf, uval, field->startbit, field->endbit,	\
> +			       pbuflen, quirks);				\
> +		}								\
> +	})
> +
> +#define __unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks)	\
> +	({									\
> +		for (size_t i = 0; i < (num_fields); i++) {			\
> +			typeof(&(fields)[0]) field = &fields[i];		\
> +			u64 uval;						\
> +										\
> +			__unpack(pbuf, &uval, field->startbit, field->endbit,	\
> +				 pbuflen, quirks);				\
> +										\
> +			u64_to_ustruct_field(ustruct, field->offset, field->size, uval); \
> +		}								\
> +	})
> +
>  /**
>   * calculate_box_addr - Determine physical location of byte in buffer
>   * @box: Index of byte within buffer seen as a logical big-endian big number
> @@ -168,8 +195,8 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen,
>  }
>  EXPORT_SYMBOL(pack);
>  
> -static void __unpack(const void *pbuf, u64 *uval, size_t startbit,
> -		     size_t endbit, size_t pbuflen, u8 quirks)
> +static void __unpack(const void *pbuf, u64 *uval, size_t startbit, size_t endbit,
> +		     size_t pbuflen, u8 quirks)
>  {
>  	/* Logical byte indices corresponding to the
>  	 * start and end of the field.
> @@ -322,4 +349,122 @@ int packing(void *pbuf, u64 *uval, int startbit, int endbit, size_t pbuflen,
>  }
>  EXPORT_SYMBOL(packing);
>  
> +static u64 ustruct_field_to_u64(const void *ustruct, size_t field_offset,
> +				size_t field_size)
> +{
> +	switch (field_size) {
> +	case 1:
> +		return *((u8 *)(ustruct + field_offset));
> +	case 2:
> +		return *((u16 *)(ustruct + field_offset));
> +	case 4:
> +		return *((u32 *)(ustruct + field_offset));
> +	default:
> +		return *((u64 *)(ustruct + field_offset));
> +	}
> +}
> +
> +static void u64_to_ustruct_field(void *ustruct, size_t field_offset,
> +				 size_t field_size, u64 uval)
> +{
> +	switch (field_size) {
> +	case 1:
> +		*((u8 *)(ustruct + field_offset)) = uval;
> +		break;
> +	case 2:
> +		*((u16 *)(ustruct + field_offset)) = uval;
> +		break;
> +	case 4:
> +		*((u32 *)(ustruct + field_offset)) = uval;
> +		break;
> +	default:
> +		*((u64 *)(ustruct + field_offset)) = uval;
> +		break;
> +	}
> +}
> +
> +/**
> + * pack_fields_s - Pack array of small fields
> + *
> + * @pbuf: Pointer to a buffer holding the packed value.
> + * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
> + * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
> + *	     It is expected (but not checked) that this has the same data type
> + *	     as all struct packed_field_s definitions.
> + * @fields: Array of small packed fields definition. They must not overlap.
> + * @num_fields: Length of @fields array.
> + * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
> + *	    QUIRK_MSB_ON_THE_RIGHT.
> + */
> +void pack_fields_s(void *pbuf, size_t pbuflen, const void *ustruct,
> +		   const struct packed_field_s *fields, size_t num_fields,
> +		   u8 quirks)
> +{
> +	__pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
> +}
> +EXPORT_SYMBOL(pack_fields_s);
> +
> +/**
> + * pack_fields_m - Pack array of medium fields
> + *
> + * @pbuf: Pointer to a buffer holding the packed value.
> + * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
> + * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
> + *	     It is expected (but not checked) that this has the same data type
> + *	     as all struct packed_field_s definitions.
> + * @fields: Array of medium packed fields definition. They must not overlap.
> + * @num_fields: Length of @fields array.
> + * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
> + *	    QUIRK_MSB_ON_THE_RIGHT.
> + */
> +void pack_fields_m(void *pbuf, size_t pbuflen, const void *ustruct,
> +		   const struct packed_field_m *fields, size_t num_fields,
> +		   u8 quirks)
> +{
> +	__pack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
> +}
> +EXPORT_SYMBOL(pack_fields_m);
> +
> +/**
> + * unpack_fields_s - Unpack array of small fields
> + *
> + * @pbuf: Pointer to a buffer holding the packed value.
> + * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
> + * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
> + *	     It is expected (but not checked) that this has the same data type
> + *	     as all struct packed_field_s definitions.
> + * @fields: Array of small packed fields definition. They must not overlap.
> + * @num_fields: Length of @fields array.
> + * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
> + *	    QUIRK_MSB_ON_THE_RIGHT.
> + */
> +void unpack_fields_s(const void *pbuf, size_t pbuflen, void *ustruct,
> +		     const struct packed_field_s *fields, size_t num_fields,
> +		     u8 quirks)
> +{
> +	__unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
> +}
> +EXPORT_SYMBOL(unpack_fields_s);
> +
> +/**
> + * unpack_fields_m - Unpack array of medium fields
> + *
> + * @pbuf: Pointer to a buffer holding the packed value.
> + * @pbuflen: The length in bytes of the packed buffer pointed to by @pbuf.
> + * @ustruct: Pointer to CPU-readable structure holding the unpacked value.
> + *	     It is expected (but not checked) that this has the same data type
> + *	     as all struct packed_field_s definitions.
> + * @fields: Array of medium packed fields definition. They must not overlap.
> + * @num_fields: Length of @fields array.
> + * @quirks: A bit mask of QUIRK_LITTLE_ENDIAN, QUIRK_LSW32_IS_FIRST and
> + *	    QUIRK_MSB_ON_THE_RIGHT.
> + */
> +void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
> +		     const struct packed_field_m *fields, size_t num_fields,
> +		     u8 quirks)
> +{
> +	__unpack_fields(pbuf, pbuflen, ustruct, fields, num_fields, quirks);
> +}
> +EXPORT_SYMBOL(unpack_fields_m);
> +
>  MODULE_DESCRIPTION("Generic bitfield packing and unpacking");
> diff --git a/lib/packing_test.c b/lib/packing_test.c
> index b38ea43c03fd..ff5be660de01 100644
> --- a/lib/packing_test.c
> +++ b/lib/packing_test.c

I appreciate the test.

> @@ -396,9 +396,70 @@ static void packing_test_unpack(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, uval, params->uval);
>  }
>  
> +#define PACKED_BUF_SIZE 8
> +
> +typedef struct __packed { u8 buf[PACKED_BUF_SIZE]; } packed_buf_t;
> +
> +struct test_data {
> +	u8 field1;
> +	u16 field2;
> +	u32 field3;
> +	u16 field4;
> +	u8 field5;
> +	u16 field6;

If you group the u8s with the u8s and with the odd u16, and the
remaining two u16s together, you should get a structure with less
padding.

> +};
> +
> +DECLARE_PACKED_FIELDS_S(test_fields, sizeof(packed_buf_t)) = {
> +	PACKED_FIELD(63, 61, struct test_data, field1),
> +	PACKED_FIELD(60, 52, struct test_data, field2),
> +	PACKED_FIELD(51, 28, struct test_data, field3),
> +	PACKED_FIELD(27, 14, struct test_data, field4),
> +	PACKED_FIELD(13, 9, struct test_data, field5),
> +	PACKED_FIELD(8, 0, struct test_data, field6),
> +};
> +
> +static void packing_test_pack_fields(struct kunit *test)
> +{
> +	const struct test_data data = {
> +		.field1 = 0x2,
> +		.field2 = 0x100,
> +		.field3 = 0xF00050,
> +		.field4 = 0x7D3,
> +		.field5 = 0x9,
> +		.field6 = 0x10B,
> +	};
> +	packed_buf_t buf = {};

Reverse Christmas tree (should come after "expect").

> +	packed_buf_t expect = {
> +		.buf = { 0x50, 0x0F, 0x00, 0x05, 0x01, 0xF4, 0xD3, 0x0B },
> +	};
> +
> +	pack_fields(&buf, sizeof(buf), &data, test_fields, 0);
> +
> +	KUNIT_EXPECT_MEMEQ(test, &expect, &buf, sizeof(buf));
> +}
> +
> +static void packing_test_unpack_fields(struct kunit *test)
> +{
> +	const packed_buf_t buf = {
> +		.buf = { 0x17, 0x28, 0x10, 0x19, 0x3D, 0xA9, 0x07, 0x9C },
> +	};
> +	struct test_data data = {};

Reverse Christmas tree.

> +
> +	unpack_fields(&buf, sizeof(buf), &data, test_fields, 0);
> +
> +	KUNIT_EXPECT_EQ(test, 0, data.field1);
> +	KUNIT_EXPECT_EQ(test, 0x172, data.field2);
> +	KUNIT_EXPECT_EQ(test, 0x810193, data.field3);
> +	KUNIT_EXPECT_EQ(test, 0x36A4, data.field4);
> +	KUNIT_EXPECT_EQ(test, 0x3, data.field5);
> +	KUNIT_EXPECT_EQ(test, 0x19C, data.field6);
> +}
> +
>  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),
> +	KUNIT_CASE(packing_test_pack_fields),
> +	KUNIT_CASE(packing_test_unpack_fields),
>  	{},
>  };
>  
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 107393a8c48a..3f707bba18d7 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -209,19 +209,6 @@ struct symbol {
>  
>  static HASHTABLE_DEFINE(symbol_hashtable, 1U << 10);
>  
> -/* This is based on the hash algorithm from gdbm, via tdb */
> -static inline unsigned int tdb_hash(const char *name)
> -{
> -	unsigned value;	/* Used to compute the hash value.  */
> -	unsigned   i;	/* Used to cycle through random values. */
> -
> -	/* Set the initial value from the key size. */
> -	for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
> -		value = (value + (((unsigned char *)name)[i] << (i*5 % 24)));
> -
> -	return (1103515243 * value + 12345);
> -}
> -
>  /**
>   * Allocate a new symbols for use in the hash of exported symbols or
>   * the list of unresolved symbols per module
> @@ -327,7 +314,7 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
>  				      sechdr->sh_name);
>  }
>  
> -static const char *sec_name(const struct elf_info *info, unsigned int secindex)
> +const char *sec_name(const struct elf_info *info, unsigned int secindex)
>  {
>  	/*
>  	 * If sym->st_shndx is a special section index, there is no
> @@ -1602,6 +1589,7 @@ static void read_symbols(const char *modname)
>  
>  		handle_symbol(mod, &info, sym, symname);
>  		handle_moddevtable(mod, &info, sym, symname);
> +		handle_packed_field_symbol(mod, &info, sym, symname);
>  	}
>  
>  	check_sec_ref(mod, &info);
> @@ -2222,6 +2210,8 @@ int main(int argc, char **argv)
>  	if (missing_namespace_deps)
>  		write_namespace_deps_files(missing_namespace_deps);
>  
> +	check_packed_field_symbols();
> +
>  	if (dump_write)
>  		write_dump(dump_write);
>  	if (sec_mismatch_count && !sec_mismatch_warn_only)
> diff --git a/scripts/mod/packed_fields.c b/scripts/mod/packed_fields.c
> new file mode 100644
> index 000000000000..aa9dbd704e52
> --- /dev/null
> +++ b/scripts/mod/packed_fields.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024, Intel Corporation. */
> +
> +/* Code to validate struct packed_field_[sm] data, and perform sanity checks
> + * to ensure the packed field data is laid out correctly and fits into the
> + * relevant buffer size.
> + */
> +
> +#include <fnmatch.h>
> +#include <hashtable.h>
> +#include <inttypes.h>
> +#include <stdint.h>
> +#include <xalloc.h>
> +
> +#include "modpost.h"
> +
> +typedef uint16_t	u16;
> +typedef uint8_t		u8;
> +
> +#define BITS_PER_BYTE	8
> +
> +/* Big exception to the "don't include kernel headers into userspace", which
> + * even potentially has different endianness and word sizes, since we handle
> + * those differences explicitly below
> + */
> +#include "../../include/linux/packing_types.h"
> +
> +struct packed_field_elem {
> +	uint64_t startbit;
> +	uint64_t endbit;
> +	uint64_t offset;
> +	uint64_t size;
> +};
> +
> +enum field_type {
> +	UNKNOWN_SECTION,
> +	PACKED_FIELD_S,
> +	PACKED_FIELD_M,
> +};
> +
> +static size_t field_type_to_size(enum field_type type)
> +{
> +	switch (type) {
> +	case PACKED_FIELD_S:
> +		return sizeof(struct packed_field_s);
> +	case PACKED_FIELD_M:
> +		return sizeof(struct packed_field_m);
> +	default:
> +		error("attempted to get field size for unknown packed field type %u\n",
> +		      type);
> +		return 0;
> +	}
> +}
> +
> +static void get_field_contents(const void *data, enum field_type type,
> +			       struct packed_field_elem *elem)
> +{
> +	switch (type) {
> +	case PACKED_FIELD_S: {
> +		const struct packed_field_s *data_field = data;
> +
> +		elem->startbit = TO_NATIVE(data_field->startbit);
> +		elem->endbit = TO_NATIVE(data_field->endbit);
> +		elem->offset = TO_NATIVE(data_field->offset);
> +		elem->size = TO_NATIVE(data_field->size);
> +		return;
> +	}
> +	case PACKED_FIELD_M: {
> +		const struct packed_field_m *data_field = data;
> +
> +		elem->startbit = TO_NATIVE(data_field->startbit);
> +		elem->endbit = TO_NATIVE(data_field->endbit);
> +		elem->offset = TO_NATIVE(data_field->offset);
> +		elem->size = TO_NATIVE(data_field->size);
> +		return;
> +	}
> +	default:
> +		error("attempted to get field contents for unknown packed field type %u\n",
> +		      type);
> +	}
> +}
> +
> +struct field_symbol {
> +	struct hlist_node hnode;
> +	enum field_type type;
> +	size_t buffer_size;
> +	size_t data_size;
> +	void *data;
> +	struct module *mod;
> +	char *name;
> +};
> +
> +static HASHTABLE_DEFINE(field_hashtable, 1U << 10);
> +
> +static struct field_symbol *alloc_field(char *name, struct module *mod)
> +{
> +	struct field_symbol *f = xmalloc(sizeof(*f));
> +
> +	memset(f, 0, sizeof(*f));
> +	f->mod = mod;
> +	f->name = name;
> +
> +	return f;
> +}
> +
> +static void hash_add_field(struct field_symbol *field)
> +{
> +	hash_add(field_hashtable, &field->hnode, tdb_hash(field->name));
> +}
> +
> +static struct field_symbol *find_field(const char *name, struct module *mod)
> +{
> +	struct field_symbol *f;
> +
> +	hash_for_each_possible(field_hashtable, f, hnode, tdb_hash(name)) {
> +		if (strcmp(f->name, name) == 0 && f->mod == mod)
> +			return f;
> +	}
> +	return NULL;
> +}
> +
> +void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
> +				Elf_Sym *sym, const char *symname)
> +{
> +	unsigned int secindex = get_secindex(info, sym);
> +	enum field_type type = UNKNOWN_SECTION;
> +	bool is_size_symbol = false;
> +	struct field_symbol *f;
> +	const char *section;
> +	char *name;
> +
> +	/* Skip symbols without a name */
> +	if (*symname == '\0')
> +		return;
> +
> +	/* Skip symbols with invalid sections */
> +	if (secindex >= info->num_sections)
> +		return;
> +
> +	section = sec_name(info, secindex);
> +
> +	if (strcmp(section, ".rodata.packed_fields_s") == 0)
> +		type = PACKED_FIELD_S;
> +	else if (strcmp(section, ".rodata.packed_fields_m") == 0)
> +		type = PACKED_FIELD_M;
> +
> +	/* Other sections don't relate to packed fields */
> +	if (type == UNKNOWN_SECTION)
> +		return;
> +
> +	name = xstrdup(symname);
> +
> +	/* Extract original field name from the size symbol */
> +	if (!fnmatch("__*_buffer_sz", name, 0)) {
> +		name += strlen("__");
> +		name[strlen(name) - strlen("_buffer_sz")] = '\0';
> +		is_size_symbol = true;
> +	}
> +
> +	f = find_field(name, mod);
> +	if (!f) {
> +		f = alloc_field(name, mod);
> +		f->type = type;
> +		hash_add_field(f);
> +	}
> +
> +	if (f->type != type) {
> +		error("%s and %s have mismatched packed field sections\n",
> +		      f->name, symname);
> +		return;
> +	}
> +
> +	if (is_size_symbol) {
> +		size_t *size_data = sym_get_data(info, sym);
> +		size_t size = TO_NATIVE(*size_data);
> +
> +		if (f->buffer_size && f->buffer_size != size) {
> +			error("%s has buffer size %zu, but symbol %s says the size should be %zu\n",
> +			      f->name, f->buffer_size, symname, size);
> +		}
> +
> +		f->buffer_size = size;
> +	} else {
> +		if (f->data)
> +			error("%s has multiple data symbols???\n",
> +			      f->name);
> +
> +		f->data_size = sym->st_size;
> +		f->data = xmalloc(f->data_size);
> +		memcpy(f->data, sym_get_data(info, sym), f->data_size);
> +	}
> +}
> +
> +enum element_order {
> +	FIRST_ELEMENT,
> +	SECOND_ELEMENT,
> +	ASCENDING_ORDER,
> +	DESCENDING_ORDER,
> +};

If you still keep this for next versions, this should go at the top,
before function definitions.

> +
> +static void check_packed_field_array(const struct field_symbol *f)
> +{
> +	struct packed_field_elem previous_elem = {};
> +	size_t field_size = field_type_to_size(f->type);
> +	enum element_order order = FIRST_ELEMENT;
> +	void *data_ptr;
> +	int count;

Stored as signed, printed as unsigned.

> +
> +	/* check that the data is a multiple of the size */
> +	if (f->data_size % field_size != 0) {
> +		error("symbol %s of module %s has size %zu which is not a multiple of the field size (%zu)\n",
> +		      f->name, f->mod->name, f->data_size, field_size);
> +		return;
> +	}
> +
> +	data_ptr = f->data;
> +	count = 0;

Initialization be wrapped into the declaration.

> +
> +	while (data_ptr < f->data + f->data_size) {
> +		struct packed_field_elem elem = {};
> +
> +		get_field_contents(data_ptr, f->type, &elem);
> +
> +		if (elem.startbit < elem.endbit)
> +			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") must be larger than endbit (%" PRIu64 ")\n",
> +			      f->name, f->mod->name, count, elem.startbit,
> +			      elem.endbit);
> +
> +		if (elem.startbit >= BITS_PER_BYTE * f->buffer_size)
> +			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") puts field outsize of the packed buffer size (%" PRIu64 ")\n",
> +			      f->name, f->mod->name, count, elem.startbit,
> +			      f->buffer_size);
> +
> +		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size)
> +			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") and endbit (%" PRIu64 ") indicate a field of width (%" PRIu64 ") which does not fit into the field size (%" PRIu64 ")\n",
> +			      f->name, f->mod->name, count, elem.startbit,
> +			      elem.endbit, elem.startbit - elem.endbit,
> +			      elem.size);

elem.size is in bytes but the field width is in bits. The error message
is confusing because it's not clear what is wrong.

> +
> +		if (elem.size != 1 && elem.size != 2 && elem.size != 4 && elem.size != 8)
> +			error("\"%s\" [%s.ko] element %u size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
> +			      f->name, f->mod->name, count, elem.size);
> +
> +		switch (order) {
> +		case FIRST_ELEMENT:
> +			order = SECOND_ELEMENT;
> +			break;
> +		case SECOND_ELEMENT:
> +			order = previous_elem.startbit < elem.startbit ?
> +				ASCENDING_ORDER : DESCENDING_ORDER;
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		switch (order) {
> +		case ASCENDING_ORDER:

I don't see why ASCENDING_ORDER and DESCENDING_ORDER are handled as part
of a different switch/case statement.

> +			if (previous_elem.startbit >= elem.startbit)
> +				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element startbit is %" PRIu64 "\n",
> +				      f->name, f->mod->name, count,
> +				      elem.startbit, previous_elem.startbit);
> +			if (previous_elem.endbit >= elem.endbit)
> +				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element endbit is %" PRIu64 "\n",
> +				      f->name, f->mod->name, count, elem.endbit,
> +				      previous_elem.endbit);
> +
> +			break;
> +		case DESCENDING_ORDER:
> +			if (previous_elem.startbit <= elem.startbit)
> +				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in descending order, but previous element startbit is %" PRIu64 "\n",
> +				      f->name, f->mod->name, count,
> +				      elem.startbit, previous_elem.startbit);
> +			if (previous_elem.endbit <= elem.endbit)
> +				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in descending order, but previous element endbit is %" PRIu64 "\n",
> +				      f->name, f->mod->name, count,
> +				      elem.endbit, previous_elem.endbit);
> +			break;

The end goal was never to enforce ascending or descending order of the
start and end of the fields. The point was to enforce that the fields do
not overlap, which this does _not_ do. The rule for detecting overlap of
intervals [a, b] and [c, d] is that max(a, c) <= min(b, d).

Enforcing ascending/descending order is just a way of reducing the
complexity of the overlap check from O(n^2) to O(n). But the overlap
check itself is missing.

> +		default:
> +			break;
> +		}
> +
> +		previous_elem = elem;
> +		data_ptr += field_size;
> +		count++;
> +	}
> +}
> +
> +void check_packed_field_symbols(void)
> +{
> +	struct field_symbol *f;
> +
> +	hash_for_each(field_hashtable, f, hnode)
> +		check_packed_field_array(f);
> +}

Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

On Fri, Nov 08, 2024 at 01:24:07PM +0200, Vladimir Oltean wrote:
> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
> > +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
> > +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
> > +	const struct packed_field_s name[] __pf_section_s
> 
> This will need sorting out - how to make this declaration macro
> compatible with the "static" keyword. The obvious way (to group the
> field array and the buffer size into a structure) doesn't work. It loses
> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
> and unpack_fields() wrappers.
> 
> Maybe a different tool is needed for checking that no packed fields
> exceed the buffer size? Forcing the buffer size be a symbol just because
> that's what modpost works with seems unnatural.
> 
> If we knew the position of the largest field array element in C, and if
> we enforced that all pack_fields() use a strong type (size included) for
> the packed buffer, we'd have all information inside the pack_fields()
> macro, because we only need to compare the largest field against the
> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
> inside the pack_fields() macro itself. And have the compile-time checks
> spill over between C and modpost...
> 
> Not to mention, pack_fields() would have one argument less: pbuflen.

I was thinking something like this (attached). I still don't like
modpost more than the assertions in C code, because it imposes more
constraints upon the library user which didn't exist before. Though
without the extra restrictions added in this patch (just ascending order
for packed fields + strong type for packed buffer), I don't think the
modpost variant is going to work, or is going to become extremely complex.

[-- Attachment #2: 0001-pack_fields-changes.patch --]
[-- Type: text/x-diff, Size: 13961 bytes --]

From d9bf88a24518afa96fe55beda040a3605b81610a Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 8 Nov 2024 17:07:16 +0200
Subject: [PATCH] pack_fields() changes

Remove ability to declare packed field array in ascending and descending
order, over just ascending.

Actually implement the overlap check in modpost. The simple check:
"elem.endbit <= previous_elem.startbit" enforces both ascending order
and non-overlapping checks.

Move the check that fields don't exceed the buffer size inside the
pack_fields() and unpack_fields() macro. With the assumption that the
packed field array is sorted (enforced by modpost), we only need to
check this for the last element of the array.

Remove the packed buffer size from the DECLARE_PACKED_FIELDS_*() macros,
and thus from the .rodata.packed_fields_* sections. Also remove the
modpost code that looks for those sizes.

Modify the pack_fields() API to always expect a strong type as the
"pbuf" argument, because we take sizeof(*pbuf) as the packed buffer
size. Remove the pbuflen from the required argument list, and adapt ice
accordingly.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c |  16 +--
 include/linux/packing.h                     |  32 +++--
 include/linux/packing_types.h               |  15 +-
 scripts/mod/packed_fields.c                 | 148 ++++++--------------
 4 files changed, 76 insertions(+), 135 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 98845d790791..631963427082 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1381,7 +1381,7 @@ static void ice_copy_rxq_ctx_to_hw(struct ice_hw *hw,
 	PACKED_FIELD((lsb) + (width) - 1, (lsb), struct struct_name, struct_field)
 
 /* LAN Rx Queue Context */
-DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ) = {
+static const DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields) = {
 				 /* Field		Width	LSB */
 	ICE_CTX_STORE(ice_rlan_ctx, head,		13,	0),
 	ICE_CTX_STORE(ice_rlan_ctx, cpuid,		8,	13),
@@ -1416,10 +1416,8 @@ DECLARE_PACKED_FIELDS_S(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ) = {
 static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
 			     ice_rxq_ctx_buf_t *buf)
 {
-	BUILD_BUG_ON(sizeof(*buf) != ICE_RXQ_CTX_SZ);
-
-	pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
-		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
+	pack_fields(buf, ctx, ice_rlan_ctx_fields, QUIRK_LITTLE_ENDIAN |
+		    QUIRK_LSW32_IS_FIRST);
 }
 
 /**
@@ -1448,7 +1446,7 @@ int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
 }
 
 /* LAN Tx Queue Context */
-DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ) = {
+static const DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields) = {
 				    /* Field			Width	LSB */
 	ICE_CTX_STORE(ice_tlan_ctx, base,			57,	0),
 	ICE_CTX_STORE(ice_tlan_ctx, port_num,			3,	57),
@@ -1489,10 +1487,8 @@ DECLARE_PACKED_FIELDS_S(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ) = {
  */
 void ice_pack_txq_ctx(const struct ice_tlan_ctx *ctx, ice_txq_ctx_buf_t *buf)
 {
-	BUILD_BUG_ON(sizeof(*buf) != ICE_TXQ_CTX_SZ);
-
-	pack_fields(buf, sizeof(*buf), ctx, ice_tlan_ctx_fields,
-		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
+	pack_fields(buf, ctx, ice_tlan_ctx_fields, QUIRK_LITTLE_ENDIAN |
+		    QUIRK_LSW32_IS_FIRST);
 }
 
 /* Sideband Queue command wrappers */
diff --git a/include/linux/packing.h b/include/linux/packing.h
index b66c4809e121..d4988131e13a 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -35,16 +35,26 @@ void unpack_fields_m(const void *pbuf, size_t pbuflen, void *ustruct,
 		     const struct packed_field_m *fields, size_t num_fields,
 		     u8 quirks);
 
-#define pack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
-	_Generic((fields), \
-		 const struct packed_field_s * : pack_fields_s, \
-		 const struct packed_field_m * : pack_fields_m \
-		)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
-
-#define unpack_fields(pbuf, pbuflen, ustruct, fields, quirks) \
-	_Generic((fields), \
-		 const struct packed_field_s * : unpack_fields_s, \
-		 const struct packed_field_m * : unpack_fields_m \
-		)(pbuf, pbuflen, ustruct, fields, ARRAY_SIZE(fields), quirks)
+#define pack_fields(pbuf, ustruct, fields, quirks) \
+	({ typeof(fields[0]) *__f = fields; \
+	   size_t pbuflen = sizeof(*pbuf); \
+	   size_t num_fields = ARRAY_SIZE(fields); \
+	   BUILD_BUG_ON(__f[num_fields - 1].startbit >= BITS_PER_BYTE * pbuflen); \
+	   _Generic(__f, \
+		    const struct packed_field_s * : pack_fields_s, \
+		    const struct packed_field_m * : pack_fields_m \
+		   )(pbuf, pbuflen, ustruct, __f, num_fields, quirks); \
+	})
+
+#define unpack_fields(pbuf, ustruct, fields, quirks) \
+	({ typeof(fields[0]) *__f = fields; \
+	   size_t pbuflen = sizeof(*pbuf); \
+	   size_t num_fields = ARRAY_SIZE(fields); \
+	   BUILD_BUG_ON(__f[num_fields - 1].startbit >= BITS_PER_BYTE * pbuflen); \
+	   _Generic(__f, \
+		    const struct packed_field_s * : unpack_fields_s, \
+		    const struct packed_field_m * : unpack_fields_m \
+		   )(pbuf, pbuflen, ustruct, __f, num_fields, quirks); \
+	})
 
 #endif
diff --git a/include/linux/packing_types.h b/include/linux/packing_types.h
index 49ae4329a494..c761d93be235 100644
--- a/include/linux/packing_types.h
+++ b/include/linux/packing_types.h
@@ -24,12 +24,6 @@ struct packed_field_s {
 	GEN_PACKED_FIELD_MEMBERS(u8);
 };
 
-#define __pf_section_s __section(".rodata.packed_fields_s")
-
-#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
-	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
-	const struct packed_field_s name[] __pf_section_s
-
 /* Medium packed field. Use with bit offsets < 65536, buffers < 8KB and
  * unpacked structures < 64KB.
  */
@@ -37,11 +31,10 @@ struct packed_field_m {
 	GEN_PACKED_FIELD_MEMBERS(u16);
 };
 
-#define __pf_section_m __section(".rodata.packed_fields_m")
-
-#define DECLARE_PACKED_FIELDS_M(name, buffer_sz) \
-	const size_t __ ## name ## _buffer_sz __pf_section_m = buffer_sz; \
-	const struct packed_field_m name[] __pf_section_m
+#define DECLARE_PACKED_FIELDS_S(name) \
+	struct packed_field_s name[] __section(".rodata.packed_fields_s")
+#define DECLARE_PACKED_FIELDS_M(name) \
+	struct packed_field_m name[] __section(".rodata.packed_fields_m")
 
 #define PACKED_FIELD(start, end, struct_name, struct_field) \
 { \
diff --git a/scripts/mod/packed_fields.c b/scripts/mod/packed_fields.c
index aa9dbd704e52..5927b0c7f404 100644
--- a/scripts/mod/packed_fields.c
+++ b/scripts/mod/packed_fields.c
@@ -87,18 +87,18 @@ struct field_symbol {
 	size_t data_size;
 	void *data;
 	struct module *mod;
-	char *name;
+	const char *name;
 };
 
 static HASHTABLE_DEFINE(field_hashtable, 1U << 10);
 
-static struct field_symbol *alloc_field(char *name, struct module *mod)
+static struct field_symbol *alloc_field(const char *name, struct module *mod)
 {
 	struct field_symbol *f = xmalloc(sizeof(*f));
 
 	memset(f, 0, sizeof(*f));
 	f->mod = mod;
-	f->name = name;
+	f->name = xstrdup(name);
 
 	return f;
 }
@@ -124,10 +124,8 @@ void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
 {
 	unsigned int secindex = get_secindex(info, sym);
 	enum field_type type = UNKNOWN_SECTION;
-	bool is_size_symbol = false;
 	struct field_symbol *f;
 	const char *section;
-	char *name;
 
 	/* Skip symbols without a name */
 	if (*symname == '\0')
@@ -148,18 +146,9 @@ void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
 	if (type == UNKNOWN_SECTION)
 		return;
 
-	name = xstrdup(symname);
-
-	/* Extract original field name from the size symbol */
-	if (!fnmatch("__*_buffer_sz", name, 0)) {
-		name += strlen("__");
-		name[strlen(name) - strlen("_buffer_sz")] = '\0';
-		is_size_symbol = true;
-	}
-
-	f = find_field(name, mod);
+	f = find_field(symname, mod);
 	if (!f) {
-		f = alloc_field(name, mod);
+		f = alloc_field(symname, mod);
 		f->type = type;
 		hash_add_field(f);
 	}
@@ -170,118 +159,71 @@ void handle_packed_field_symbol(struct module *mod, struct elf_info *info,
 		return;
 	}
 
-	if (is_size_symbol) {
-		size_t *size_data = sym_get_data(info, sym);
-		size_t size = TO_NATIVE(*size_data);
-
-		if (f->buffer_size && f->buffer_size != size) {
-			error("%s has buffer size %zu, but symbol %s says the size should be %zu\n",
-			      f->name, f->buffer_size, symname, size);
-		}
-
-		f->buffer_size = size;
-	} else {
-		if (f->data)
-			error("%s has multiple data symbols???\n",
-			      f->name);
-
-		f->data_size = sym->st_size;
-		f->data = xmalloc(f->data_size);
-		memcpy(f->data, sym_get_data(info, sym), f->data_size);
+	if (f->data) {
+		error("%s has multiple data symbols???\n",
+		      f->name);
 	}
-}
 
-enum element_order {
-	FIRST_ELEMENT,
-	SECOND_ELEMENT,
-	ASCENDING_ORDER,
-	DESCENDING_ORDER,
-};
+	f->data_size = sym->st_size;
+	f->data = xmalloc(f->data_size);
+	memcpy(f->data, sym_get_data(info, sym), f->data_size);
+}
 
 static void check_packed_field_array(const struct field_symbol *f)
 {
-	struct packed_field_elem previous_elem = {};
 	size_t field_size = field_type_to_size(f->type);
-	enum element_order order = FIRST_ELEMENT;
+	struct packed_field_elem previous_elem = {};
 	void *data_ptr;
-	int count;
+	size_t count;
 
 	/* check that the data is a multiple of the size */
-	if (f->data_size % field_size != 0) {
-		error("symbol %s of module %s has size %zu which is not a multiple of the field size (%zu)\n",
-		      f->name, f->mod->name, f->data_size, field_size);
+	if (f->data_size % field_size) {
+		error("[%s] symbol %s has size %zu which is not a multiple of the field size (%zu)\n",
+		      f->mod->name, f->name, f->data_size, field_size);
 		return;
 	}
 
-	data_ptr = f->data;
-	count = 0;
-
-	while (data_ptr < f->data + f->data_size) {
+	for (data_ptr = f->data, count = 0;
+	     data_ptr < f->data + f->data_size;
+	     data_ptr += field_size, count++) {
 		struct packed_field_elem elem = {};
 
 		get_field_contents(data_ptr, f->type, &elem);
 
-		if (elem.startbit < elem.endbit)
-			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") must be larger than endbit (%" PRIu64 ")\n",
-			      f->name, f->mod->name, count, elem.startbit,
-			      elem.endbit);
+		if (elem.size != 1 && elem.size != 2 &&
+		    elem.size != 4 && elem.size != 8) {
+			error("[%s] \"%s\" field %zu unpacked size (%" PRIu64
+			      ") must be 1, 2, 4, or 8 bytes\n",
+			      f->mod->name, f->name, count, elem.size);
+		}
 
-		if (elem.startbit >= BITS_PER_BYTE * f->buffer_size)
-			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") puts field outsize of the packed buffer size (%" PRIu64 ")\n",
-			      f->name, f->mod->name, count, elem.startbit,
-			      f->buffer_size);
+		if (elem.startbit < elem.endbit) {
+			error("[%s] \"%s\" field %zu (%" PRIu64 "-%" PRIu64
+			      "): start bit must be >= end bit\n",
+			      f->mod->name, f->name, count, elem.startbit,
+			      elem.endbit);
+		}
 
-		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size)
-			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") and endbit (%" PRIu64 ") indicate a field of width (%" PRIu64 ") which does not fit into the field size (%" PRIu64 ")\n",
-			      f->name, f->mod->name, count, elem.startbit,
+		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size) {
+			error("[%s] \"%s\" field %zu (%" PRIu64 "-%" PRIu64
+			      ") has a width of %" PRIu64
+			      " which does not fit into the unpacked structure field (%"
+			      PRIu64 " bytes)\n",
+			      f->mod->name, f->name, count, elem.startbit,
 			      elem.endbit, elem.startbit - elem.endbit,
 			      elem.size);
-
-		if (elem.size != 1 && elem.size != 2 && elem.size != 4 && elem.size != 8)
-			error("\"%s\" [%s.ko] element %u size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
-			      f->name, f->mod->name, count, elem.size);
-
-		switch (order) {
-		case FIRST_ELEMENT:
-			order = SECOND_ELEMENT;
-			break;
-		case SECOND_ELEMENT:
-			order = previous_elem.startbit < elem.startbit ?
-				ASCENDING_ORDER : DESCENDING_ORDER;
-			break;
-		default:
-			break;
 		}
 
-		switch (order) {
-		case ASCENDING_ORDER:
-			if (previous_elem.startbit >= elem.startbit)
-				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element startbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count,
-				      elem.startbit, previous_elem.startbit);
-			if (previous_elem.endbit >= elem.endbit)
-				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element endbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count, elem.endbit,
-				      previous_elem.endbit);
-
-			break;
-		case DESCENDING_ORDER:
-			if (previous_elem.startbit <= elem.startbit)
-				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in descending order, but previous element startbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count,
-				      elem.startbit, previous_elem.startbit);
-			if (previous_elem.endbit <= elem.endbit)
-				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in descending order, but previous element endbit is %" PRIu64 "\n",
-				      f->name, f->mod->name, count,
-				      elem.endbit, previous_elem.endbit);
-			break;
-		default:
-			break;
+		if (count && elem.endbit <= previous_elem.startbit) {
+			error("[%s] \"%s\" field %zu (%" PRIu64 "-%" PRIu64
+			      ") overlaps with previous field (%" PRIu64 "-%" PRIu64
+			      ") or array is not declared in ascending order\n",
+			      f->mod->name, f->name, count, elem.startbit,
+			      elem.endbit, previous_elem.startbit,
+			      previous_elem.endbit);
 		}
 
 		previous_elem = elem;
-		data_ptr += field_size;
-		count++;
 	}
 }
 
-- 
2.43.0

Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[Jacob Keller]

On 11/8/2024 3:24 AM, Vladimir Oltean wrote:
> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
>> +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
>> +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
>> +	const struct packed_field_s name[] __pf_section_s
> 
> This will need sorting out - how to make this declaration macro
> compatible with the "static" keyword. The obvious way (to group the
> field array and the buffer size into a structure) doesn't work. It loses
> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
> and unpack_fields() wrappers.
> 

Yea. I didn't see any static warnings on my setup but i forgot to check
with sparse.

> Maybe a different tool is needed for checking that no packed fields
> exceed the buffer size? Forcing the buffer size be a symbol just because
> that's what modpost works with seems unnatural.
> 

True, I could move that check to a different spot.

> If we knew the position of the largest field array element in C, and if
> we enforced that all pack_fields() use a strong type (size included) for
> the packed buffer, we'd have all information inside the pack_fields()
> macro, because we only need to compare the largest field against the
> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
> inside the pack_fields() macro itself. And have the compile-time checks
> spill over between C and modpost...
> 

I think thats reasonable.

> Not to mention, pack_fields() would have one argument less: pbuflen.
> 

Yea, I think enforcing the sized type like that via structure is a
reasonable restriction.

>> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
>> index ada3a36cc4bc..013bf4be2642 100644
>> --- a/scripts/mod/modpost.h
>> +++ b/scripts/mod/modpost.h
>> @@ -160,6 +160,19 @@ static inline bool is_valid_name(struct elf_info *elf, Elf_Sym *sym)
>>  	return !is_mapping_symbol(name);
>>  }
>>  
>> +/* This is based on the hash algorithm from gdbm, via tdb */
>> +static inline unsigned int tdb_hash(const char *name)
>> +{
>> +	unsigned value;	/* Used to compute the hash value.  */
>> +	unsigned   i;	/* Used to cycle through random values. */
>> +
>> +	/* Set the initial value from the key size. */
>> +	for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
>> +		value = (value + (((unsigned char *)name)[i] << (i*5 % 24)));
>> +
>> +	return (1103515243 * value + 12345);
>> +}
>> +
> 
> Code movement should be in separate changes.
> 

Sure.

>> diff --git a/lib/packing_test.c b/lib/packing_test.c
>> index b38ea43c03fd..ff5be660de01 100644
>> --- a/lib/packing_test.c
>> +++ b/lib/packing_test.c
> 
> I appreciate the test.
> 

Yea. I figured the addition of a test is good, though I didn't see the
need to compound it with tests for every combination of the flags, since
we cover those with pack() and unpack() tests.

>> @@ -396,9 +396,70 @@ static void packing_test_unpack(struct kunit *test)
>>  	KUNIT_EXPECT_EQ(test, uval, params->uval);
>>  }
>>  
>> +#define PACKED_BUF_SIZE 8
>> +
>> +typedef struct __packed { u8 buf[PACKED_BUF_SIZE]; } packed_buf_t;
>> +
>> +struct test_data {
>> +	u8 field1;
>> +	u16 field2;
>> +	u32 field3;
>> +	u16 field4;
>> +	u8 field5;
>> +	u16 field6;
> 
> If you group the u8s with the u8s and with the odd u16, and the
> remaining two u16s together, you should get a structure with less
> padding.
> 

I kept them as ordered by the order they appear in the packed data, but
yes re-ordering is safe, and does safe a few bytes.

>> +};
>> +
>> +DECLARE_PACKED_FIELDS_S(test_fields, sizeof(packed_buf_t)) = {
>> +	PACKED_FIELD(63, 61, struct test_data, field1),
>> +	PACKED_FIELD(60, 52, struct test_data, field2),
>> +	PACKED_FIELD(51, 28, struct test_data, field3),
>> +	PACKED_FIELD(27, 14, struct test_data, field4),
>> +	PACKED_FIELD(13, 9, struct test_data, field5),
>> +	PACKED_FIELD(8, 0, struct test_data, field6),
>> +};
>> +
>> +static void packing_test_pack_fields(struct kunit *test)
>> +{
>> +	const struct test_data data = {
>> +		.field1 = 0x2,
>> +		.field2 = 0x100,
>> +		.field3 = 0xF00050,
>> +		.field4 = 0x7D3,
>> +		.field5 = 0x9,
>> +		.field6 = 0x10B,
>> +	};
>> +	packed_buf_t buf = {};
> 
> Reverse Christmas tree (should come after "expect").
> 

Good point. I'll fix these.

>> +enum element_order {
>> +	FIRST_ELEMENT,
>> +	SECOND_ELEMENT,
>> +	ASCENDING_ORDER,
>> +	DESCENDING_ORDER,
>> +};
> 
> If you still keep this for next versions, this should go at the top,
> before function definitions.
> 

Sure.

>> +
>> +static void check_packed_field_array(const struct field_symbol *f)
>> +{
>> +	struct packed_field_elem previous_elem = {};
>> +	size_t field_size = field_type_to_size(f->type);
>> +	enum element_order order = FIRST_ELEMENT;
>> +	void *data_ptr;
>> +	int count;
> 
> Stored as signed, printed as unsigned.
> 
>> +
>> +	/* check that the data is a multiple of the size */
>> +	if (f->data_size % field_size != 0) {
>> +		error("symbol %s of module %s has size %zu which is not a multiple of the field size (%zu)\n",
>> +		      f->name, f->mod->name, f->data_size, field_size);
>> +		return;
>> +	}
>> +
>> +	data_ptr = f->data;
>> +	count = 0;
> 
> Initialization be wrapped into the declaration.
> 
>> +
>> +	while (data_ptr < f->data + f->data_size) {
>> +		struct packed_field_elem elem = {};
>> +
>> +		get_field_contents(data_ptr, f->type, &elem);
>> +
>> +		if (elem.startbit < elem.endbit)
>> +			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") must be larger than endbit (%" PRIu64 ")\n",
>> +			      f->name, f->mod->name, count, elem.startbit,
>> +			      elem.endbit);
>> +
>> +		if (elem.startbit >= BITS_PER_BYTE * f->buffer_size)
>> +			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") puts field outsize of the packed buffer size (%" PRIu64 ")\n",
>> +			      f->name, f->mod->name, count, elem.startbit,
>> +			      f->buffer_size);
>> +
>> +		if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size)
>> +			error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") and endbit (%" PRIu64 ") indicate a field of width (%" PRIu64 ") which does not fit into the field size (%" PRIu64 ")\n",
>> +			      f->name, f->mod->name, count, elem.startbit,
>> +			      elem.endbit, elem.startbit - elem.endbit,
>> +			      elem.size);
> 
> elem.size is in bytes but the field width is in bits. The error message
> is confusing because it's not clear what is wrong.
> 

True, I can convert everything to bits.

>> +
>> +		if (elem.size != 1 && elem.size != 2 && elem.size != 4 && elem.size != 8)
>> +			error("\"%s\" [%s.ko] element %u size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
>> +			      f->name, f->mod->name, count, elem.size);
>> +
>> +		switch (order) {
>> +		case FIRST_ELEMENT:
>> +			order = SECOND_ELEMENT;
>> +			break;
>> +		case SECOND_ELEMENT:
>> +			order = previous_elem.startbit < elem.startbit ?
>> +				ASCENDING_ORDER : DESCENDING_ORDER;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +
>> +		switch (order) {
>> +		case ASCENDING_ORDER:
> 
> I don't see why ASCENDING_ORDER and DESCENDING_ORDER are handled as part
> of a different switch/case statement.
> 
I wanted to check both start/end bit at the SECOND_FIELD case, because I
had naively assumed i was covering overlap with that... :D Should have
thought through it more.

>> +			if (previous_elem.startbit >= elem.startbit)
>> +				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element startbit is %" PRIu64 "\n",
>> +				      f->name, f->mod->name, count,
>> +				      elem.startbit, previous_elem.startbit);
>> +			if (previous_elem.endbit >= elem.endbit)
>> +				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element endbit is %" PRIu64 "\n",
>> +				      f->name, f->mod->name, count, elem.endbit,
>> +				      previous_elem.endbit);
>> +
>> +			break;
>> +		case DESCENDING_ORDER:
>> +			if (previous_elem.startbit <= elem.startbit)
>> +				error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in descending order, but previous element startbit is %" PRIu64 "\n",
>> +				      f->name, f->mod->name, count,
>> +				      elem.startbit, previous_elem.startbit);
>> +			if (previous_elem.endbit <= elem.endbit)
>> +				error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in descending order, but previous element endbit is %" PRIu64 "\n",
>> +				      f->name, f->mod->name, count,
>> +				      elem.endbit, previous_elem.endbit);
>> +			break;
> 
> The end goal was never to enforce ascending or descending order of the
> start and end of the fields. The point was to enforce that the fields do
> not overlap, which this does _not_ do. The rule for detecting overlap of
> intervals [a, b] and [c, d] is that max(a, c) <= min(b, d).
> 

I do think that keeping them ordered is a good thing too.

> Enforcing ascending/descending order is just a way of reducing the
> complexity of the overlap check from O(n^2) to O(n). But the overlap
> check itself is missing.
> 

Yea, you're right.

PACKED_FIELD(64, 30, ...),
PACKED_FIELD(60, 24, ...),

which would pass the test, but which overlaps and is wrong.

If I put the overlap check to a check outside of the switch, then just
keep the ascending/descending checks in the same switch case we should
be correct?

Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack_fields()

[Jacob Keller]

On 11/8/2024 10:31 AM, Vladimir Oltean wrote:
> On Fri, Nov 08, 2024 at 01:24:07PM +0200, Vladimir Oltean wrote:
>> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
>>> +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
>>> +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
>>> +	const struct packed_field_s name[] __pf_section_s
>>
>> This will need sorting out - how to make this declaration macro
>> compatible with the "static" keyword. The obvious way (to group the
>> field array and the buffer size into a structure) doesn't work. It loses
>> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
>> and unpack_fields() wrappers.
>>
>> Maybe a different tool is needed for checking that no packed fields
>> exceed the buffer size? Forcing the buffer size be a symbol just because
>> that's what modpost works with seems unnatural.
>>
>> If we knew the position of the largest field array element in C, and if
>> we enforced that all pack_fields() use a strong type (size included) for
>> the packed buffer, we'd have all information inside the pack_fields()
>> macro, because we only need to compare the largest field against the
>> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
>> inside the pack_fields() macro itself. And have the compile-time checks
>> spill over between C and modpost...
>>
>> Not to mention, pack_fields() would have one argument less: pbuflen.
> 
> I was thinking something like this (attached). I still don't like
> modpost more than the assertions in C code, because it imposes more
> constraints upon the library user which didn't exist before. Though
> without the extra restrictions added in this patch (just ascending order
> for packed fields + strong type for packed buffer), I don't think the
> modpost variant is going to work, or is going to become extremely complex.

To me, the restrictions seem acceptable. You can always wrap an
arbitrary void buffer into a sized type via a structure.

I do agree that modpost isn't perfect with respect to the C code, but I
dislike the sheer size of the macros generated and the complexity added
to the build system.

My preferred solution is unfortunately not available in C: having the
compiler evaluate a constant function. There is a C++ extension that
does that, but it doesn't seem like GCC has enabled such support in the
C even as an extension.

I'd prefer to keep allowing both ascending and descending order, because
I think those are both valid orderings depending on how you're thinking
about the fields (little vs big endian).