[PATCH net 0/5] net: ipa: endpoint configuration fixes

[PATCH net 0/5] net: ipa: endpoint configuration fixes

[Alex Elder]

This series fixes four bugs in the configuration of IPA endpoints.
See the description of each for more information.  The last patch
changes a BUILD_BUG_ON() call into a runtime warning, because
violating the checked condition does not consitute a real bug.

					-Alex

Alex Elder (5):
  net: ipa: program metadata mask differently
  net: ipa: fix modem LAN RX endpoint id
  net: ipa: program upper nibbles of sequencer type
  net: ipa: header pad field only valid for AP->modem endpoint
  net: ipa: warn if gsi_trans structure is too big

 drivers/net/ipa/ipa_data-sc7180.c |  2 +-
 drivers/net/ipa/ipa_endpoint.c    | 97 ++++++++++++++++++-------------
 drivers/net/ipa/ipa_main.c        |  7 ++-
 drivers/net/ipa/ipa_reg.h         |  2 +
 4 files changed, 65 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH net 1/5] net: ipa: program metadata mask differently

[Alex Elder]

The way the mask value is programmed for QMAP RX endpoints was based
on some wrong assumptions about the way metadata containing the QMAP
mux_id value is formatted.  The metadata value supplied by the
modem is *not* in QMAP format, and in fact contains the mux_id we
want in its (big endian) low-order byte.  That byte must be written
by the IPA into offset 1 of the QMAP header it inserts before the
received packet.

QMAP TX endpoints *do* use a QMAP header as the metadata sent with
each packet.  The modem assumes this, and based on that assumes the
mux_id is in the second byte.  To match those assumptions we must
program the modem TX (QMAP) endpoint HDR register to indicate the
metadata will be found at offset 0 in the message header.

The previous configuration managed to work, but it was not working
correctly.  This patch fixes a bug whose symptom was receipt of
messages containing the wrong QMAP mux_id.

In fixing this, get rid of ipa_rmnet_mux_id_metadata_mask(), which
was more or less defined so there was a separate place to explain
what was happening as we generated the mask value.  Instead, put a
longer description of how this works above ipa_endpoint_init_hdr(),
and define the metadata mask to use as a simple constant.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 74 ++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 66649a806dd1..6015fabb4df5 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -32,6 +32,9 @@
 /* The amount of RX buffer space consumed by standard skb overhead */
 #define IPA_RX_BUFFER_OVERHEAD	(PAGE_SIZE - SKB_MAX_ORDER(NET_SKB_PAD, 0))
 
+/* Where to find the QMAP mux_id for a packet within modem-supplied metadata */
+#define IPA_ENDPOINT_QMAP_METADATA_MASK		0x000000ff /* host byte order */
+
 #define IPA_ENDPOINT_RESET_AGGR_RETRY_MAX	3
 #define IPA_AGGR_TIME_LIMIT_DEFAULT		1000	/* microseconds */
 
@@ -433,7 +436,25 @@ static void ipa_endpoint_init_cfg(struct ipa_endpoint *endpoint)
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
 
-static void ipa_endpoint_init_hdr(struct ipa_endpoint *endpoint)
+/**
+ * We program QMAP endpoints so each packet received is preceded by a QMAP
+ * header structure.  The QMAP header contains a 1-byte mux_id and 2-byte
+ * packet size field, and we have the IPA hardware populate both for each
+ * received packet.  The header is configured (in the HDR_EXT register)
+ * to use big endian format.
+ *
+ * The packet size is written into the QMAP header's pkt_len field.  That
+ * location is defined here using the HDR_OFST_PKT_SIZE field.
+ *
+ * The mux_id comes from a 4-byte metadata value supplied with each packet
+ * by the modem.  It is *not* a QMAP header, but it does contain the mux_id
+ * value that we want, in its low-order byte.  A bitmask defined in the
+ * endpoint's METADATA_MASK register defines which byte within the modem
+ * metadata contains the mux_id.  And the OFST_METADATA field programmed
+ * here indicates where the extracted byte should be placed within the QMAP
+ * header.
+ */
+void ipa_endpoint_init_hdr(struct ipa_endpoint *endpoint)
 {
 	u32 offset = IPA_REG_ENDP_INIT_HDR_N_OFFSET(endpoint->endpoint_id);
 	u32 val = 0;
@@ -441,25 +462,31 @@ static void ipa_endpoint_init_hdr(struct ipa_endpoint *endpoint)
 	if (endpoint->data->qmap) {
 		size_t header_size = sizeof(struct rmnet_map_header);
 
+		/* We might supply a checksum header after the QMAP header */
 		if (endpoint->toward_ipa && endpoint->data->checksum)
 			header_size += sizeof(struct rmnet_map_ul_csum_header);
-
 		val |= u32_encode_bits(header_size, HDR_LEN_FMASK);
-		/* metadata is the 4 byte rmnet_map header itself */
-		val |= HDR_OFST_METADATA_VALID_FMASK;
-		val |= u32_encode_bits(0, HDR_OFST_METADATA_FMASK);
-		/* HDR_ADDITIONAL_CONST_LEN is 0; (IPA->AP only) */
+
+		/* Define how to fill mux_id in a received QMAP header */
 		if (!endpoint->toward_ipa) {
-			u32 size_offset = offsetof(struct rmnet_map_header,
-						   pkt_len);
+			u32 off;	/* Field offset within header */
 
+			/* Where IPA will write the metadata value */
+			off = offsetof(struct rmnet_map_header, mux_id);
+			val |= u32_encode_bits(off, HDR_OFST_METADATA_FMASK);
+
+			/* Where IPA will write the length */
+			off = offsetof(struct rmnet_map_header, pkt_len);
 			val |= HDR_OFST_PKT_SIZE_VALID_FMASK;
-			val |= u32_encode_bits(size_offset,
-					       HDR_OFST_PKT_SIZE_FMASK);
+			val |= u32_encode_bits(off, HDR_OFST_PKT_SIZE_FMASK);
 		}
+		/* For QMAP TX, metadata offset is 0 (modem assumes this) */
+		val |= HDR_OFST_METADATA_VALID_FMASK;
+
+		/* HDR_ADDITIONAL_CONST_LEN is 0; (RX only) */
 		/* HDR_A5_MUX is 0 */
 		/* HDR_LEN_INC_DEAGG_HDR is 0 */
-		/* HDR_METADATA_REG_VALID is 0; (AP->IPA only) */
+		/* HDR_METADATA_REG_VALID is 0 (TX only) */
 	}
 
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
@@ -482,28 +509,6 @@ static void ipa_endpoint_init_hdr_ext(struct ipa_endpoint *endpoint)
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
 
-/**
- * Generate a metadata mask value that will select only the mux_id
- * field in an rmnet_map header structure.  The mux_id is at offset
- * 1 byte from the beginning of the structure, but the metadata
- * value is treated as a 4-byte unit.  So this mask must be computed
- * with endianness in mind.  Note that ipa_endpoint_init_hdr_metadata_mask()
- * will convert this value to the proper byte order.
- *
- * Marked __always_inline because this is really computing a
- * constant value.
- */
-static __always_inline __be32 ipa_rmnet_mux_id_metadata_mask(void)
-{
-	size_t mux_id_offset = offsetof(struct rmnet_map_header, mux_id);
-	u32 mux_id_mask = 0;
-	u8 *bytes;
-
-	bytes = (u8 *)&mux_id_mask;
-	bytes[mux_id_offset] = 0xff;	/* mux_id is 1 byte */
-
-	return cpu_to_be32(mux_id_mask);
-}
 
 static void ipa_endpoint_init_hdr_metadata_mask(struct ipa_endpoint *endpoint)
 {
@@ -513,8 +518,9 @@ static void ipa_endpoint_init_hdr_metadata_mask(struct ipa_endpoint *endpoint)
 
 	offset = IPA_REG_ENDP_INIT_HDR_METADATA_MASK_N_OFFSET(endpoint_id);
 
+	/* Note that HDR_ENDIANNESS indicates big endian header fields */
 	if (!endpoint->toward_ipa && endpoint->data->qmap)
-		val = ipa_rmnet_mux_id_metadata_mask();
+		val = cpu_to_be32(IPA_ENDPOINT_QMAP_METADATA_MASK);
 
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
-- 
2.25.1

Re: [PATCH net 1/5] net: ipa: program metadata mask differently

[Alex Elder]

On 6/10/20 7:19 PM, kernel test robot wrote:
> Hi Alex,
> 
> I love your patch! Perhaps something to improve:

Thanks kernel test robot!

Somehow the "static" specifier got dropped in my patch.

I will fix this when I post version 2, shortly.

					-Alex

> [auto build test WARNING on net/master]
> 
> url:    https://github.com/0day-ci/linux/commits/Alex-Elder/net-ipa-endpoint-configuration-fixes/20200611-035600
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 89dc68533b190117e1a2fb4298d88b96b3580abf
> config: arm64-allyesconfig (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
> 
> <<                  from drivers/net/ipa/ipa_endpoint.c:8:
>>> drivers/net/ipa/ipa_endpoint.c:457:6: warning: no previous prototype for 'ipa_endpoint_init_hdr' [-Wmissing-prototypes]
> 457 | void ipa_endpoint_init_hdr(struct ipa_endpoint *endpoint)
> |      ^~~~~~~~~~~~~~~~~~~~~
> In file included from include/linux/bits.h:23,
> from include/linux/bitops.h:5,
> from include/linux/kernel.h:12,
> from include/linux/list.h:9,
> from include/linux/rculist.h:10,
> from include/linux/pid.h:5,
> from include/linux/sched.h:14,
> from include/linux/ratelimit.h:6,
> from include/linux/dev_printk.h:16,
> from include/linux/device.h:15,
> from drivers/net/ipa/ipa_endpoint.c:8:
> drivers/net/ipa/ipa_endpoint.c: In function 'ipa_endpoint_config':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                            ^
> include/linux/build_bug.h:16:62: notkke: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
> drivers/net/ipa/ipa_endpoint.c:1546:12: note: in expansion of macro 'GENMASK'
> 1546 |  tx_mask = GENMASK(max - 1, 0);
> |            ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                                        ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
> drivers/net/ipa/ipa_endpoint.c:1546:12: note: in expansion of macro 'GENMASK'
> 1546 |  tx_mask = GENMASK(max - 1, 0);
> |            ^~~~~~~
> 
> vim +/ipa_endpoint_init_hdr +457 drivers/net/ipa/ipa_endpoint.c
> 
>    438	
>    439	/**
>    440	 * We program QMAP endpoints so each packet received is preceded by a QMAP
>    441	 * header structure.  The QMAP header contains a 1-byte mux_id and 2-byte
>    442	 * packet size field, and we have the IPA hardware populate both for each
>    443	 * received packet.  The header is configured (in the HDR_EXT register)
>    444	 * to use big endian format.
>    445	 *
>    446	 * The packet size is written into the QMAP header's pkt_len field.  That
>    447	 * location is defined here using the HDR_OFST_PKT_SIZE field.
>    448	 *
>    449	 * The mux_id comes from a 4-byte metadata value supplied with each packet
>    450	 * by the modem.  It is *not* a QMAP header, but it does contain the mux_id
>    451	 * value that we want, in its low-order byte.  A bitmask defined in the
>    452	 * endpoint's METADATA_MASK register defines which byte within the modem
>    453	 * metadata contains the mux_id.  And the OFST_METADATA field programmed
>    454	 * here indicates where the extracted byte should be placed within the QMAP
>    455	 * header.
>    456	 */
>  > 457	void ipa_endpoint_init_hdr(struct ipa_endpoint *endpoint)
>    458	{
>    459		u32 offset = IPA_REG_ENDP_INIT_HDR_N_OFFSET(endpoint->endpoint_id);
>    460		u32 val = 0;
>    461	
>    462		if (endpoint->data->qmap) {
>    463			size_t header_size = sizeof(struct rmnet_map_header);
>    464	
>    465			/* We might supply a checksum header after the QMAP header */
>    466			if (endpoint->toward_ipa && endpoint->data->checksum)
>    467				header_size += sizeof(struct rmnet_map_ul_csum_header);
>    468			val |= u32_encode_bits(header_size, HDR_LEN_FMASK);
>    469	
>    470			/* Define how to fill mux_id in a received QMAP header */
>    471			if (!endpoint->toward_ipa) {
>    472				u32 off;	/* Field offset within header */
>    473	
>    474				/* Where IPA will write the metadata value */
>    475				off = offsetof(struct rmnet_map_header, mux_id);
>    476				val |= u32_encode_bits(off, HDR_OFST_METADATA_FMASK);
>    477	
>    478				/* Where IPA will write the length */
>    479				off = offsetof(struct rmnet_map_header, pkt_len);
>    480				val |= HDR_OFST_PKT_SIZE_VALID_FMASK;
>    481				val |= u32_encode_bits(off, HDR_OFST_PKT_SIZE_FMASK);
>    482			}
>    483			/* For QMAP TX, metadata offset is 0 (modem assumes this) */
>    484			val |= HDR_OFST_METADATA_VALID_FMASK;
>    485	
>    486			/* HDR_ADDITIONAL_CONST_LEN is 0; (RX only) */
>    487			/* HDR_A5_MUX is 0 */
>    488			/* HDR_LEN_INC_DEAGG_HDR is 0 */
>    489			/* HDR_METADATA_REG_VALID is 0 (TX only) */
>    490		}
>    491	
>    492		iowrite32(val, endpoint->ipa->reg_virt + offset);
>    493	}
>    494	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

[PATCH net 2/5] net: ipa: fix modem LAN RX endpoint id

[Alex Elder]

The endpoint id assigned to the modem LAN RX endpoint for the SC7180 SoC
is incorrect.  The erroneous value might have been copied from SDM845 and
never updated.  The correct endpoint id to use for this SoC is 11.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_data-sc7180.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_data-sc7180.c b/drivers/net/ipa/ipa_data-sc7180.c
index 43faa35ae726..d4c2bc7ad24b 100644
--- a/drivers/net/ipa/ipa_data-sc7180.c
+++ b/drivers/net/ipa/ipa_data-sc7180.c
@@ -106,7 +106,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 	[IPA_ENDPOINT_MODEM_LAN_RX] = {
 		.ee_id		= GSI_EE_MODEM,
 		.channel_id	= 3,
-		.endpoint_id	= 13,
+		.endpoint_id	= 11,
 		.toward_ipa	= false,
 	},
 	[IPA_ENDPOINT_MODEM_AP_TX] = {
-- 
2.25.1

[PATCH net 3/5] net: ipa: program upper nibbles of sequencer type

[Alex Elder]

The upper two nibbles of the sequencer type were not used for
SDM845, and were assumed to be 0.  But for SC7180 they are used, and
so they must be programmed by ipa_endpoint_init_seq().  Fix this bug.

IPA_SEQ_PKT_PROCESS_NO_DEC_NO_UCP_DMAP doesn't have a descriptive
comment, so add one.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 6 ++++--
 drivers/net/ipa/ipa_reg.h      | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 6015fabb4df5..59313ced7036 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -699,10 +699,12 @@ static void ipa_endpoint_init_seq(struct ipa_endpoint *endpoint)
 	u32 seq_type = endpoint->seq_type;
 	u32 val = 0;
 
+	/* Sequencer type is made up of four nibbles */
 	val |= u32_encode_bits(seq_type & 0xf, HPS_SEQ_TYPE_FMASK);
 	val |= u32_encode_bits((seq_type >> 4) & 0xf, DPS_SEQ_TYPE_FMASK);
-	/* HPS_REP_SEQ_TYPE is 0 */
-	/* DPS_REP_SEQ_TYPE is 0 */
+	/* The second two apply to replicated packets */
+	val |= u32_encode_bits((seq_type >> 8) & 0xf, HPS_REP_SEQ_TYPE_FMASK);
+	val |= u32_encode_bits((seq_type >> 12) & 0xf, DPS_REP_SEQ_TYPE_FMASK);
 
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index 3b8106aa277a..0a688d8c1d7c 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -455,6 +455,8 @@ enum ipa_mode {
  *	second packet processing pass + no decipher + microcontroller
  * @IPA_SEQ_DMA_DEC:		DMA + cipher/decipher
  * @IPA_SEQ_DMA_COMP_DECOMP:	DMA + compression/decompression
+ * @IPA_SEQ_PKT_PROCESS_NO_DEC_NO_UCP_DMAP:
+ *	packet processing + no decipher + no uCP + HPS REP DMA parser
  * @IPA_SEQ_INVALID:		invalid sequencer type
  *
  * The values defined here are broken into 4-bit nibbles that are written
-- 
2.25.1

[PATCH net 4/5] net: ipa: header pad field only valid for AP->modem endpoint

[Alex Elder]

Only QMAP endpoints should be configured to find a pad size field
within packet headers.  They are found in the first byte of the QMAP
header (and the hardware fills only the 6 bits in that byte that
constitute the pad_len field).

The RMNet driver assumes the pad_len field is valid for received
packets, so we want to ensure the pad_len field is filled in that
case.  That driver also assumes the length in the QMAP header
includes the pad bytes.

The RMNet driver does *not* pad the packets it sends, so the pad_len
field can be ignored.

Fix ipa_endpoint_init_hdr_ext() so it only marks the pad field
offset valid for QMAP RX endpoints, and in that case indicates
that the length field in the header includes the pad bytes.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 59313ced7036..b35e027003b3 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -467,7 +467,7 @@ void ipa_endpoint_init_hdr(struct ipa_endpoint *endpoint)
 			header_size += sizeof(struct rmnet_map_ul_csum_header);
 		val |= u32_encode_bits(header_size, HDR_LEN_FMASK);
 
-		/* Define how to fill mux_id in a received QMAP header */
+		/* Define how to fill fields in a received QMAP header */
 		if (!endpoint->toward_ipa) {
 			u32 off;	/* Field offset within header */
 
@@ -499,10 +499,21 @@ static void ipa_endpoint_init_hdr_ext(struct ipa_endpoint *endpoint)
 	u32 val = 0;
 
 	val |= HDR_ENDIANNESS_FMASK;		/* big endian */
-	val |= HDR_TOTAL_LEN_OR_PAD_VALID_FMASK;
-	/* HDR_TOTAL_LEN_OR_PAD is 0 (pad, not total_len) */
+
+	/* A QMAP header contains a 6 bit pad field at offset 0.  The RMNet
+	 * driver assumes this field is meaningful in packets it receives,
+	 * and assumes the header's payload length includes that padding.
+	 * The RMNet driver does *not* pad packets it sends, however, so
+	 * the pad field (although 0) should be ignored.
+	 */
+	if (endpoint->data->qmap && !endpoint->toward_ipa) {
+		val |= HDR_TOTAL_LEN_OR_PAD_VALID_FMASK;
+		/* HDR_TOTAL_LEN_OR_PAD is 0 (pad, not total_len) */
+		val |= HDR_PAYLOAD_LEN_INC_PADDING_FMASK;
+		/* HDR_TOTAL_LEN_OR_PAD_OFFSET is 0 */
+	}
+
 	/* HDR_PAYLOAD_LEN_INC_PADDING is 0 */
-	/* HDR_TOTAL_LEN_OR_PAD_OFFSET is 0 */
 	if (!endpoint->toward_ipa)
 		val |= u32_encode_bits(pad_align, HDR_PAD_TO_ALIGNMENT_FMASK);
 
-- 
2.25.1

[PATCH net 5/5] net: ipa: warn if gsi_trans structure is too big

[Alex Elder]

When the DEBUG_SPINLOCK and DEBUG_LOCK_ALLOC config options are
enabled, sizeof(raw_spinlock_t) grows considerably (from 4 bytes
to 56 bytes currently).  As a consequence the size of the gsi_trans
structure exceeds 128 bytes, and this triggers a BUILD_BUG_ON()
error.

These are useful configuration options to enable, so rather than
causing a build failure, just issue a warning message at run time
if the structure is larger than we'd prefer.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 76d5108b8403..94d9aa0e999b 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -669,9 +669,6 @@ static void ipa_validate_build(void)
 	 */
 	BUILD_BUG_ON(GSI_TLV_MAX > U8_MAX);
 
-	/* Exceeding 128 bytes makes the transaction pool *much* larger */
-	BUILD_BUG_ON(sizeof(struct gsi_trans) > 128);
-
 	/* This is used as a divisor */
 	BUILD_BUG_ON(!IPA_AGGR_GRANULARITY);
 #endif /* IPA_VALIDATE */
@@ -715,6 +712,10 @@ static int ipa_probe(struct platform_device *pdev)
 	int ret;
 
 	ipa_validate_build();
+	/* Exceeding 128 bytes makes the transaction pool *much* larger */
+	if (sizeof(struct gsi_trans) > 128)
+		dev_warn(dev, "WARNING: sizeof(struct gsi_trans) = %zu\n",
+			 sizeof(struct gsi_trans));
 
 	/* If we need Trust Zone, make sure it's available */
 	modem_init = of_property_read_bool(dev->of_node, "modem-init");
-- 
2.25.1

Re: [PATCH net 5/5] net: ipa: warn if gsi_trans structure is too big

[David Miller]

From: Alex Elder <elder@linaro.org>
Date: Wed, 10 Jun 2020 14:53:32 -0500

> When the DEBUG_SPINLOCK and DEBUG_LOCK_ALLOC config options are
> enabled, sizeof(raw_spinlock_t) grows considerably (from 4 bytes
> to 56 bytes currently).  As a consequence the size of the gsi_trans
> structure exceeds 128 bytes, and this triggers a BUILD_BUG_ON()
> error.
> 
> These are useful configuration options to enable, so rather than
> causing a build failure, just issue a warning message at run time
> if the structure is larger than we'd prefer.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

Please fix the problem or prevent the build of this module in such
configurations since obviously it will fail to load successfully.

It is completely unexpected for something to fail at run time that
could be detected at build time.

Re: [PATCH net 5/5] net: ipa: warn if gsi_trans structure is too big

[Alex Elder]

On 6/10/20 6:36 PM, David Miller wrote:
> From: Alex Elder <elder@linaro.org>
> Date: Wed, 10 Jun 2020 14:53:32 -0500
> 
>> When the DEBUG_SPINLOCK and DEBUG_LOCK_ALLOC config options are
>> enabled, sizeof(raw_spinlock_t) grows considerably (from 4 bytes
>> to 56 bytes currently).  As a consequence the size of the gsi_trans
>> structure exceeds 128 bytes, and this triggers a BUILD_BUG_ON()
>> error.
>>
>> These are useful configuration options to enable, so rather than
>> causing a build failure, just issue a warning message at run time
>> if the structure is larger than we'd prefer.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
> 
> Please fix the problem or prevent the build of this module in such
> configurations since obviously it will fail to load successfully.

It will not fail to load; this really shouldn't have been treated as
a BUG to begin with.  The condition can be detected at build time but
I'm not aware of a BUILD_WARN_ON() (which would probably break the
build anyway).  The check should at least have remained under the
control of IPA_VALIDATE, because it's really there for my benefit
so I'm told if the structure grows unexpectedly.

Your pushback on this has made me think a bit more about how much
of a problem this really is though, so I'll omit this last patch
in version 2 of this series that I will post today.  Then after a
little more consideration I'll post a revised version of this one
(or not).

Thanks.

					-Alex

> It is completely unexpected for something to fail at run time that
> could be detected at build time.
>