| Message ID | 20200610195332.2612233-6-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: ipa: endpoint configuration fixes | expand |
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.
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. >
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");
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