| Message ID | 20210810160213.2257424-1-elder@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ipa: always inline ipa_aggr_granularity_val() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Tue, Aug 10, 2021 at 11:02:13AM -0500, Alex Elder wrote: > It isn't required, but all callers of ipa_aggr_granularity_val() > pass a constant value (IPA_AGGR_GRANULARITY) as the usec argument. > Two of those callers are in ipa_validate_build(), with the result > being passed to BUILD_BUG_ON(). > > Evidently the "sparc64-linux-gcc" compiler (at least) doesn't always > inline ipa_aggr_granularity_val(), so the result of the function is > not constant at compile time, and that leads to build errors. > > Define the function with the __always_inline attribute to avoid the > errors. And given that the function is inline, we can switch the > WARN_ON() there to be BUILD_BUG_ON(). > > Fixes: 5bc5588466a1f ("net: ipa: use WARN_ON() rather than assertions") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Alex Elder <elder@linaro.org> > --- > > David/Jakub, this fixes a bug in a commit in net-next/master. -Alex > > drivers/net/ipa/ipa_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c > index 25bbb456e0078..f90b3521e266b 100644 > --- a/drivers/net/ipa/ipa_main.c > +++ b/drivers/net/ipa/ipa_main.c > @@ -255,9 +255,9 @@ ipa_hardware_config_qsb(struct ipa *ipa, const struct ipa_data *data) > * less than the number of timer ticks in the requested period. 0 is not > * a valid granularity value. > */ > -static u32 ipa_aggr_granularity_val(u32 usec) > +static __always_inline u32 ipa_aggr_granularity_val(u32 usec) > { > - WARN_ON(!usec); > + BUILD_BUG_ON(!usec); So what exactly are you checking here if all callers pass same value? It is in-kernel API, declared as static inside one module. There is no need to protect from itself. Thanks > > return DIV_ROUND_CLOSEST(usec * TIMER_FREQUENCY, USEC_PER_SEC) - 1; > } > -- > 2.27.0 >
On 8/11/21 7:02 AM, Leon Romanovsky wrote: > On Tue, Aug 10, 2021 at 11:02:13AM -0500, Alex Elder wrote: >> It isn't required, but all callers of ipa_aggr_granularity_val() >> pass a constant value (IPA_AGGR_GRANULARITY) as the usec argument. >> Two of those callers are in ipa_validate_build(), with the result >> being passed to BUILD_BUG_ON(). >> >> Evidently the "sparc64-linux-gcc" compiler (at least) doesn't always >> inline ipa_aggr_granularity_val(), so the result of the function is >> not constant at compile time, and that leads to build errors. >> >> Define the function with the __always_inline attribute to avoid the >> errors. And given that the function is inline, we can switch the >> WARN_ON() there to be BUILD_BUG_ON(). >> >> Fixes: 5bc5588466a1f ("net: ipa: use WARN_ON() rather than assertions") >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- >> >> David/Jakub, this fixes a bug in a commit in net-next/master. -Alex >> >> drivers/net/ipa/ipa_main.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c >> index 25bbb456e0078..f90b3521e266b 100644 >> --- a/drivers/net/ipa/ipa_main.c >> +++ b/drivers/net/ipa/ipa_main.c >> @@ -255,9 +255,9 @@ ipa_hardware_config_qsb(struct ipa *ipa, const struct ipa_data *data) >> * less than the number of timer ticks in the requested period. 0 is not >> * a valid granularity value. >> */ >> -static u32 ipa_aggr_granularity_val(u32 usec) >> +static __always_inline u32 ipa_aggr_granularity_val(u32 usec) >> { >> - WARN_ON(!usec); >> + BUILD_BUG_ON(!usec); > > So what exactly are you checking here if all callers pass same value? > It is in-kernel API, declared as static inside one module. There is no > need to protect from itself. Yeah that's a good point. It can just as well be removed. I think the check was added before I knew it was only going to be used with a single constant value. That said, the point was to check at runtime a required constraint. I'll post version 2 that simply removes it. Thanks. -Alex > > Thanks > >> >> return DIV_ROUND_CLOSEST(usec * TIMER_FREQUENCY, USEC_PER_SEC) - 1; >> } >> -- >> 2.27.0 >>
On Wed, Aug 11, 2021 at 07:06:01AM -0500, Alex Elder wrote: > On 8/11/21 7:02 AM, Leon Romanovsky wrote: > > On Tue, Aug 10, 2021 at 11:02:13AM -0500, Alex Elder wrote: > >> It isn't required, but all callers of ipa_aggr_granularity_val() > >> pass a constant value (IPA_AGGR_GRANULARITY) as the usec argument. > >> Two of those callers are in ipa_validate_build(), with the result > >> being passed to BUILD_BUG_ON(). > >> > >> Evidently the "sparc64-linux-gcc" compiler (at least) doesn't always > >> inline ipa_aggr_granularity_val(), so the result of the function is > >> not constant at compile time, and that leads to build errors. > >> > >> Define the function with the __always_inline attribute to avoid the > >> errors. And given that the function is inline, we can switch the > >> WARN_ON() there to be BUILD_BUG_ON(). > >> > >> Fixes: 5bc5588466a1f ("net: ipa: use WARN_ON() rather than assertions") > >> Reported-by: kernel test robot <lkp@intel.com> > >> Signed-off-by: Alex Elder <elder@linaro.org> > >> --- > >> > >> David/Jakub, this fixes a bug in a commit in net-next/master. -Alex > >> > >> drivers/net/ipa/ipa_main.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c > >> index 25bbb456e0078..f90b3521e266b 100644 > >> --- a/drivers/net/ipa/ipa_main.c > >> +++ b/drivers/net/ipa/ipa_main.c > >> @@ -255,9 +255,9 @@ ipa_hardware_config_qsb(struct ipa *ipa, const struct ipa_data *data) > >> * less than the number of timer ticks in the requested period. 0 is not > >> * a valid granularity value. > >> */ > >> -static u32 ipa_aggr_granularity_val(u32 usec) > >> +static __always_inline u32 ipa_aggr_granularity_val(u32 usec) > >> { > >> - WARN_ON(!usec); > >> + BUILD_BUG_ON(!usec); > > > > So what exactly are you checking here if all callers pass same value? > > It is in-kernel API, declared as static inside one module. There is no > > need to protect from itself. > > Yeah that's a good point. It can just as well be removed. > I think the check was added before I knew it was only going > to be used with a single constant value. That said, the > point was to check at runtime a required constraint. > > I'll post version 2 that simply removes it. Thanks. Thanks > > -Alex > > > > > Thanks > > > >> > >> return DIV_ROUND_CLOSEST(usec * TIMER_FREQUENCY, USEC_PER_SEC) - 1; > >> } > >> -- > >> 2.27.0 > >> >
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c index 25bbb456e0078..f90b3521e266b 100644 --- a/drivers/net/ipa/ipa_main.c +++ b/drivers/net/ipa/ipa_main.c @@ -255,9 +255,9 @@ ipa_hardware_config_qsb(struct ipa *ipa, const struct ipa_data *data) * less than the number of timer ticks in the requested period. 0 is not * a valid granularity value. */ -static u32 ipa_aggr_granularity_val(u32 usec) +static __always_inline u32 ipa_aggr_granularity_val(u32 usec) { - WARN_ON(!usec); + BUILD_BUG_ON(!usec); return DIV_ROUND_CLOSEST(usec * TIMER_FREQUENCY, USEC_PER_SEC) - 1; }
It isn't required, but all callers of ipa_aggr_granularity_val() pass a constant value (IPA_AGGR_GRANULARITY) as the usec argument. Two of those callers are in ipa_validate_build(), with the result being passed to BUILD_BUG_ON(). Evidently the "sparc64-linux-gcc" compiler (at least) doesn't always inline ipa_aggr_granularity_val(), so the result of the function is not constant at compile time, and that leads to build errors. Define the function with the __always_inline attribute to avoid the errors. And given that the function is inline, we can switch the WARN_ON() there to be BUILD_BUG_ON(). Fixes: 5bc5588466a1f ("net: ipa: use WARN_ON() rather than assertions") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Alex Elder <elder@linaro.org> --- David/Jakub, this fixes a bug in a commit in net-next/master. -Alex drivers/net/ipa/ipa_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)