| Message ID | 20190531141412.18632-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f0822dfc5887a2f93ac2b72cd21ab307cee4226a |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtlwifi: remove redundant assignment to variable k | expand |
On 5/31/19 9:14 AM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The assignment of 0 to variable k is never read once we break out of > the loop, so the assignment is redundant and can be removed. > > Addresses-Coverity: ("Unused value") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/wireless/realtek/rtlwifi/efuse.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/efuse.c b/drivers/net/wireless/realtek/rtlwifi/efuse.c > index e68340dfd980..83e5318ca04f 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/efuse.c > +++ b/drivers/net/wireless/realtek/rtlwifi/efuse.c > @@ -117,10 +117,8 @@ u8 efuse_read_1byte(struct ieee80211_hw *hw, u16 address) > rtlpriv->cfg-> > maps[EFUSE_CTRL] + 3); > k++; > - if (k == 1000) { > - k = 0; > + if (k == 1000) > break; > - } > } > data = rtl_read_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL]); > return data; Colin, Your patch is not wrong, but it fails to address a basic deficiency of this code snippet - when an error is detected, there is no attempt to either fix the problem or report it upstream. As the data returned will be garbage if this condition happens, we might as well replace the "break" with "return 0xFF", as well as deleting the "k = 0" line. Most of the callers of efuse_read_1byte() ignore the returned result when bits 0 and 4 are set, thus returning all 8 bits is not a bad fixup. My suspicion is that this test is in the code merely to prevent an potential unterminated "while" loop, and that this condition is extremely unlikely to happen. Larry
On Fri, 2019-05-31 at 12:29 -0500, Larry Finger wrote: > On 5/31/19 9:14 AM, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > The assignment of 0 to variable k is never read once we break out of > > the loop, so the assignment is redundant and can be removed. > > > > Addresses-Coverity: ("Unused value") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/net/wireless/realtek/rtlwifi/efuse.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/efuse.c b/drivers/net/wireless/realtek/rtlwifi/efuse.c > > index e68340dfd980..83e5318ca04f 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/efuse.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/efuse.c > > @@ -117,10 +117,8 @@ u8 efuse_read_1byte(struct ieee80211_hw *hw, u16 address) > > rtlpriv->cfg-> > > maps[EFUSE_CTRL] + 3); > > k++; > > - if (k == 1000) { > > - k = 0; > > + if (k == 1000) > > break; > > - } > > } > > data = rtl_read_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL]); > > return data; > > Colin, > > Your patch is not wrong, but it fails to address a basic deficiency of this code > snippet - when an error is detected, there is no attempt to either fix the > problem or report it upstream. As the data returned will be garbage if this > condition happens, we might as well replace the "break" with "return 0xFF", as > well as deleting the "k = 0" line. Most of the callers of efuse_read_1byte() > ignore the returned result when bits 0 and 4 are set, thus returning all 8 bits > is not a bad fixup. > > My suspicion is that this test is in the code merely to prevent an potential > unterminated "while" loop, and that this condition is extremely unlikely to happen. > > Larry The function is also overly verbose with many unnecessary rtlpriv->cfg->maps dereferences. I'd've written it more like: --- drivers/net/wireless/realtek/rtlwifi/efuse.c | 57 +++++++++++----------------- 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/drivers/net/wireless/realtek/rtlwifi/efuse.c b/drivers/net/wireless/realtek/rtlwifi/efuse.c index e68340dfd980..db253f45e87d 100644 --- a/drivers/net/wireless/realtek/rtlwifi/efuse.c +++ b/drivers/net/wireless/realtek/rtlwifi/efuse.c @@ -87,46 +87,33 @@ void efuse_initialize(struct ieee80211_hw *hw) u8 efuse_read_1byte(struct ieee80211_hw *hw, u16 address) { struct rtl_priv *rtlpriv = rtl_priv(hw); - u8 data; u8 bytetemp; u8 temp; - u32 k = 0; - const u32 efuse_len = - rtlpriv->cfg->maps[EFUSE_REAL_CONTENT_SIZE]; - - if (address < efuse_len) { - temp = address & 0xFF; - rtl_write_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL] + 1, - temp); - bytetemp = rtl_read_byte(rtlpriv, - rtlpriv->cfg->maps[EFUSE_CTRL] + 2); - temp = ((address >> 8) & 0x03) | (bytetemp & 0xFC); - rtl_write_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL] + 2, - temp); - - bytetemp = rtl_read_byte(rtlpriv, - rtlpriv->cfg->maps[EFUSE_CTRL] + 3); - temp = bytetemp & 0x7F; - rtl_write_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL] + 3, - temp); + int k = 0; + u32 *maps = rtlpriv->cfg->maps; + const u32 efuse_len = maps[EFUSE_REAL_CONTENT_SIZE]; - bytetemp = rtl_read_byte(rtlpriv, - rtlpriv->cfg->maps[EFUSE_CTRL] + 3); - while (!(bytetemp & 0x80)) { - bytetemp = rtl_read_byte(rtlpriv, - rtlpriv->cfg-> - maps[EFUSE_CTRL] + 3); - k++; - if (k == 1000) { - k = 0; - break; - } - } - data = rtl_read_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL]); - return data; - } else + if (address >= efuse_len) return 0xFF; + temp = address & 0xFF; + rtl_write_byte(rtlpriv, maps[EFUSE_CTRL] + 1, temp); + bytetemp = rtl_read_byte(rtlpriv, maps[EFUSE_CTRL] + 2); + temp = ((address >> 8) & 0x03) | (bytetemp & 0xFC); + rtl_write_byte(rtlpriv, maps[EFUSE_CTRL] + 2, temp); + + bytetemp = rtl_read_byte(rtlpriv, maps[EFUSE_CTRL] + 3); + temp = bytetemp & 0x7F; + rtl_write_byte(rtlpriv, maps[EFUSE_CTRL] + 3, temp); + + bytetemp = rtl_read_byte(rtlpriv, maps[EFUSE_CTRL] + 3); + while (!(bytetemp & 0x80)) { + bytetemp = rtl_read_byte(rtlpriv, maps[EFUSE_CTRL] + 3); + if (++k >= 1000) + return 0xFF; /* Likely defect */ + } + + return rtl_read_byte(rtlpriv, maps[EFUSE_CTRL]); } EXPORT_SYMBOL(efuse_read_1byte);
Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > The assignment of 0 to variable k is never read once we break out of > the loop, so the assignment is redundant and can be removed. > > Addresses-Coverity: ("Unused value") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Patch applied to wireless-drivers-next.git, thanks. f0822dfc5887 rtlwifi: remove redundant assignment to variable k
diff --git a/drivers/net/wireless/realtek/rtlwifi/efuse.c b/drivers/net/wireless/realtek/rtlwifi/efuse.c index e68340dfd980..83e5318ca04f 100644 --- a/drivers/net/wireless/realtek/rtlwifi/efuse.c +++ b/drivers/net/wireless/realtek/rtlwifi/efuse.c @@ -117,10 +117,8 @@ u8 efuse_read_1byte(struct ieee80211_hw *hw, u16 address) rtlpriv->cfg-> maps[EFUSE_CTRL] + 3); k++; - if (k == 1000) { - k = 0; + if (k == 1000) break; - } } data = rtl_read_byte(rtlpriv, rtlpriv->cfg->maps[EFUSE_CTRL]); return data;