| Message ID | 20220711222919.2043613-1-justinstitt@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7819b3d1dab585602905b7a213ad17ec6de6b005 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | iwlwifi: mvm: fix clang -Wformat warnings | expand |
Any chance a maintainer could take a look at this patch? I am trying to get it through this cycle and we are so close to enabling the -Wformat option for Clang. There's only a handful of patches remaining until the patch enabling this warning can be sent!
On Mon, Jul 11, 2022 at 3:29 PM Justin Stitt <justinstitt@google.com> wrote: > > When building with Clang we encounter these warnings: > | drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:1108:47: error: > | format specifies type 'unsigned char' but the argument has type 's16' > | (aka 'short') [-Werror,-Wformat] IWL_DEBUG_INFO(mvm, "\tburst index: > | %hhu\n", res->ftm.burst_index); > - > | drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:1111:47: error: > | format specifies type 'unsigned char' but the argument has type 's32' > | (aka 'int') [-Werror,-Wformat] IWL_DEBUG_INFO(mvm, "\trssi spread: > | %hhu\n", res->ftm.rssi_spread); > > The previous format specifier `%hhu` describes a u8 but our arguments > are wider than this which means bits are potentially being lost. > > Variadic functions (printf-like) undergo default argument promotion. > Documentation/core-api/printk-formats.rst specifically recommends using > the promoted-to-type's format flag. > > As per C11 6.3.1.1: > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) `If an int > can represent all values of the original type ..., the value is > converted to an int; otherwise, it is converted to an unsigned int. > These are called the integer promotions.` Thus it makes sense to change > `%hhu` to `%d` for both instances of the warning. > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > Signed-off-by: Justin Stitt <justinstitt@google.com> Thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c > index 430044bc4755..e8702184c950 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c > @@ -1105,10 +1105,10 @@ static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index, > IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status); > IWL_DEBUG_INFO(mvm, "\tBSSID: %pM\n", res->addr); > IWL_DEBUG_INFO(mvm, "\thost time: %llu\n", res->host_time); > - IWL_DEBUG_INFO(mvm, "\tburst index: %hhu\n", res->ftm.burst_index); > + IWL_DEBUG_INFO(mvm, "\tburst index: %d\n", res->ftm.burst_index); > IWL_DEBUG_INFO(mvm, "\tsuccess num: %u\n", res->ftm.num_ftmr_successes); > IWL_DEBUG_INFO(mvm, "\trssi: %d\n", res->ftm.rssi_avg); > - IWL_DEBUG_INFO(mvm, "\trssi spread: %hhu\n", res->ftm.rssi_spread); > + IWL_DEBUG_INFO(mvm, "\trssi spread: %d\n", res->ftm.rssi_spread); > IWL_DEBUG_INFO(mvm, "\trtt: %lld\n", res->ftm.rtt_avg); > IWL_DEBUG_INFO(mvm, "\trtt var: %llu\n", res->ftm.rtt_variance); > IWL_DEBUG_INFO(mvm, "\trtt spread: %llu\n", res->ftm.rtt_spread); > -- > 2.37.0.144.g8ac04bfd2-goog >
Justin Stitt <justinstitt@google.com> writes: > Any chance a maintainer could take a look at this patch? I am trying > to get it through this cycle and we are so close to enabling the > -Wformat option for Clang. There's only a handful of patches remaining > until the patch enabling this warning can be sent! Gregory, can I take this directly to wireless-next? I assigned it to me in patchwork.
Justin Stitt <justinstitt@google.com> wrote: > When building with Clang we encounter these warnings: > | drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:1108:47: error: > | format specifies type 'unsigned char' but the argument has type 's16' > | (aka 'short') [-Werror,-Wformat] IWL_DEBUG_INFO(mvm, "\tburst index: > | %hhu\n", res->ftm.burst_index); > - > | drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:1111:47: error: > | format specifies type 'unsigned char' but the argument has type 's32' > | (aka 'int') [-Werror,-Wformat] IWL_DEBUG_INFO(mvm, "\trssi spread: > | %hhu\n", res->ftm.rssi_spread); > > The previous format specifier `%hhu` describes a u8 but our arguments > are wider than this which means bits are potentially being lost. > > Variadic functions (printf-like) undergo default argument promotion. > Documentation/core-api/printk-formats.rst specifically recommends using > the promoted-to-type's format flag. > > As per C11 6.3.1.1: > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) `If an int > can represent all values of the original type ..., the value is > converted to an int; otherwise, it is converted to an unsigned int. > These are called the integer promotions.` Thus it makes sense to change > `%hhu` to `%d` for both instances of the warning. > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > Signed-off-by: Justin Stitt <justinstitt@google.com> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Patch applied to wireless-next.git, thanks. 7819b3d1dab5 wifi: iwlwifi: mvm: fix clang -Wformat warnings
On Tue, 2022-07-26 at 17:27 +0300, Kalle Valo wrote: > Justin Stitt <justinstitt@google.com> writes: > > > Any chance a maintainer could take a look at this patch? I am trying > > to get it through this cycle and we are so close to enabling the > > -Wformat option for Clang. There's only a handful of patches remaining > > until the patch enabling this warning can be sent! > > Gregory, can I take this directly to wireless-next? I assigned it to me > in patchwork. > Sorry for the long delay. Yes, it looks like a good fix. Thanks!
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c index 430044bc4755..e8702184c950 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c @@ -1105,10 +1105,10 @@ static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index, IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status); IWL_DEBUG_INFO(mvm, "\tBSSID: %pM\n", res->addr); IWL_DEBUG_INFO(mvm, "\thost time: %llu\n", res->host_time); - IWL_DEBUG_INFO(mvm, "\tburst index: %hhu\n", res->ftm.burst_index); + IWL_DEBUG_INFO(mvm, "\tburst index: %d\n", res->ftm.burst_index); IWL_DEBUG_INFO(mvm, "\tsuccess num: %u\n", res->ftm.num_ftmr_successes); IWL_DEBUG_INFO(mvm, "\trssi: %d\n", res->ftm.rssi_avg); - IWL_DEBUG_INFO(mvm, "\trssi spread: %hhu\n", res->ftm.rssi_spread); + IWL_DEBUG_INFO(mvm, "\trssi spread: %d\n", res->ftm.rssi_spread); IWL_DEBUG_INFO(mvm, "\trtt: %lld\n", res->ftm.rtt_avg); IWL_DEBUG_INFO(mvm, "\trtt var: %llu\n", res->ftm.rtt_variance); IWL_DEBUG_INFO(mvm, "\trtt spread: %llu\n", res->ftm.rtt_spread);
When building with Clang we encounter these warnings: | drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:1108:47: error: | format specifies type 'unsigned char' but the argument has type 's16' | (aka 'short') [-Werror,-Wformat] IWL_DEBUG_INFO(mvm, "\tburst index: | %hhu\n", res->ftm.burst_index); - | drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:1111:47: error: | format specifies type 'unsigned char' but the argument has type 's32' | (aka 'int') [-Werror,-Wformat] IWL_DEBUG_INFO(mvm, "\trssi spread: | %hhu\n", res->ftm.rssi_spread); The previous format specifier `%hhu` describes a u8 but our arguments are wider than this which means bits are potentially being lost. Variadic functions (printf-like) undergo default argument promotion. Documentation/core-api/printk-formats.rst specifically recommends using the promoted-to-type's format flag. As per C11 6.3.1.1: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) `If an int can represent all values of the original type ..., the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.` Thus it makes sense to change `%hhu` to `%d` for both instances of the warning. Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Justin Stitt <justinstitt@google.com> --- drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)