| Message ID | 20211117063621.160695-1-ye.guojin@zte.com.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: rs: fixup the return value type of iwl_legacy_rate_to_fw_idx() | expand |
On Wed, 2021-11-17 at 06:36 +0000, cgel.zte@gmail.com wrote: > From: Ye Guojin <ye.guojin@zte.com.cn> > > This was found by coccicheck: > ./drivers/net/wireless/intel/iwlwifi/fw/rs.c, 147, 10-21, WARNING > Unsigned expression compared with zero legacy_rate < 0 [] > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/rs.c b/drivers/net/wireless/intel/iwlwifi/fw/rs.c [] > @@ -142,7 +142,7 @@ u32 iwl_new_rate_from_v1(u32 rate_v1) > } > /* if legacy format */ > } else { > - u32 legacy_rate = iwl_legacy_rate_to_fw_idx(rate_v1); > + int legacy_rate = iwl_legacy_rate_to_fw_idx(rate_v1); > > WARN_ON(legacy_rate < 0); Why not just remove the WARN_ON instead?
On Tue, 2021-11-16 at 22:41 -0800, Joe Perches wrote: > On Wed, 2021-11-17 at 06:36 +0000, cgel.zte@gmail.com wrote: > > From: Ye Guojin <ye.guojin@zte.com.cn> > > > > This was found by coccicheck: > > ./drivers/net/wireless/intel/iwlwifi/fw/rs.c, 147, 10-21, WARNING > > Unsigned expression compared with zero legacy_rate < 0 > [] > > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/rs.c b/drivers/net/wireless/intel/iwlwifi/fw/rs.c > [] > > @@ -142,7 +142,7 @@ u32 iwl_new_rate_from_v1(u32 rate_v1) > > } > > /* if legacy format */ > > } else { > > - u32 legacy_rate = iwl_legacy_rate_to_fw_idx(rate_v1); > > + int legacy_rate = iwl_legacy_rate_to_fw_idx(rate_v1); > > > > WARN_ON(legacy_rate < 0); > > Why not just remove the WARN_ON instead? > Well, iwl_legacy_rate_to_fw_idx() _tries_ to return -1 if we can't find the index. But there are a few more wrong things in this implementation: 1. the iwl_legacy_rate_to_fw_idx() function is only called inside the fw/rs.c file, so it should be static; 2. if we don't find the idx and return -1, we WARN but still use the value, which will cause the rate_v2 to be set to 0xffffffff, which I'm pretty sure is not the intention. So, this should be fixed properly, rather than just changing the function to return int. -- Cheers, Luca.
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/rs.h b/drivers/net/wireless/intel/iwlwifi/fw/api/rs.h index a09081d7ed45..7794cd6d289d 100644 --- a/drivers/net/wireless/intel/iwlwifi/fw/api/rs.h +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/rs.h @@ -710,7 +710,7 @@ struct iwl_lq_cmd { u8 iwl_fw_rate_idx_to_plcp(int idx); u32 iwl_new_rate_from_v1(u32 rate_v1); -u32 iwl_legacy_rate_to_fw_idx(u32 rate_n_flags); +int iwl_legacy_rate_to_fw_idx(u32 rate_n_flags); const struct iwl_rate_mcs_info *iwl_rate_mcs(int idx); const char *iwl_rs_pretty_ant(u8 ant); const char *iwl_rs_pretty_bw(int bw); diff --git a/drivers/net/wireless/intel/iwlwifi/fw/rs.c b/drivers/net/wireless/intel/iwlwifi/fw/rs.c index a21c3befd93b..3850881210e6 100644 --- a/drivers/net/wireless/intel/iwlwifi/fw/rs.c +++ b/drivers/net/wireless/intel/iwlwifi/fw/rs.c @@ -142,7 +142,7 @@ u32 iwl_new_rate_from_v1(u32 rate_v1) } /* if legacy format */ } else { - u32 legacy_rate = iwl_legacy_rate_to_fw_idx(rate_v1); + int legacy_rate = iwl_legacy_rate_to_fw_idx(rate_v1); WARN_ON(legacy_rate < 0); rate_v2 |= legacy_rate; @@ -172,7 +172,7 @@ u32 iwl_new_rate_from_v1(u32 rate_v1) } IWL_EXPORT_SYMBOL(iwl_new_rate_from_v1); -u32 iwl_legacy_rate_to_fw_idx(u32 rate_n_flags) +int iwl_legacy_rate_to_fw_idx(u32 rate_n_flags) { int rate = rate_n_flags & RATE_LEGACY_RATE_MSK_V1; int idx;