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

Message ID 20200610195332.2612233-6-elder@linaro.org
State New
Headers show
Series
  • net: ipa: endpoint configuration fixes
Related show

Commit Message

Alex Elder June 10, 2020, 7:53 p.m.
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(-)

-- 
2.25.1

Comments

David Miller June 10, 2020, 11:36 p.m. | #1
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.
Alex Elder June 11, 2020, 12:58 p.m. | #2
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.

>

Patch

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");