[PATCH v2] rtlwifi: rtl8192cu: Remove variable self-assignment in rf.c
In _rtl92c_get_txpower_writeval_by_regulatory() the variable writeVal is assigned to itself in an if ... else statement, apparently only to document that the branch condition is handled and that a previously read value should be returned unmodified. The self-assignment causes clang to raise the following warning: drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c:304:13: error: explicitly assigning value of variable of type 'u32' (aka 'unsigned int') to itself [-Werror,-Wself-assign] writeVal = writeVal; Delete the branch with the self-assignment. Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- Changes in v2: - Delete the 'else if' branch entirely drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c index 9cff6bc4049c..cf551785eb08 100644 --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c @@ -299,9 +299,6 @@ static void _rtl92c_get_txpower_writeval_by_regulatory(struct ieee80211_hw *hw, writeVal = 0x; if (rtlpriv->dm.dynamic_txhighpower_lvl == TXHIGHPWRLEVEL_BT1) writeVal = writeVal - 0x06060606; - else if (rtlpriv->dm.dynamic_txhighpower_lvl == -TXHIGHPWRLEVEL_BT2) - writeVal = writeVal; *(p_outwriteval + rf) = writeVal; } } -- 2.16.0.rc1.238.g530d649a79-goog
Re: [PATCH] rtlwifi: rtl8192cu: Remove variable self-assignment in rf.c
El Wed, Feb 07, 2018 at 02:35:59PM -0600 Larry Finger ha dit: > On 02/07/2018 02:26 PM, Matthias Kaehlcke wrote: > > In _rtl92c_get_txpower_writeval_by_regulatory() the variable writeVal > > is assigned to itself in an if ... else statement, apparently only to > > document that the branch condition is handled and that a previously read > > value should be returned unmodified. The self-assignment causes clang to > > raise the following warning: > > > > drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c:304:13: > >error: explicitly assigning value of variable of type 'u32' > > (aka 'unsigned int') to itself [-Werror,-Wself-assign] > >writeVal = writeVal; > > > > Replace the self-assignment with a semicolon, which still serves to > > document the 'handling' of the branch condition. > > > > Signed-off-by: Matthias Kaehlcke <m...@chromium.org> > > --- > > drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c > > b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c > > index 9cff6bc4049c..4db92496c122 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c > > @@ -301,7 +301,7 @@ static void > > _rtl92c_get_txpower_writeval_by_regulatory(struct ieee80211_hw *hw, > > writeVal = writeVal - 0x06060606; > > else if (rtlpriv->dm.dynamic_txhighpower_lvl == > > TXHIGHPWRLEVEL_BT2) > > - writeVal = writeVal; > > + ; > > *(p_outwriteval + rf) = writeVal; > > } > > } > > > > As the branch condition does nothing, why not remove it and save the > compiler's optimizer a bit of work? The code looks strange, but it matches > the rest of Realtek's USB drivers. Sure, I am happy to change it to whatever the authors/maintainers prefer. I'll wait a bit before respinning for if others feel strongly about keeping the branch.
[PATCH] rtlwifi: rtl8192cu: Remove variable self-assignment in rf.c
In _rtl92c_get_txpower_writeval_by_regulatory() the variable writeVal is assigned to itself in an if ... else statement, apparently only to document that the branch condition is handled and that a previously read value should be returned unmodified. The self-assignment causes clang to raise the following warning: drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c:304:13: error: explicitly assigning value of variable of type 'u32' (aka 'unsigned int') to itself [-Werror,-Wself-assign] writeVal = writeVal; Replace the self-assignment with a semicolon, which still serves to document the 'handling' of the branch condition. Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c index 9cff6bc4049c..4db92496c122 100644 --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c @@ -301,7 +301,7 @@ static void _rtl92c_get_txpower_writeval_by_regulatory(struct ieee80211_hw *hw, writeVal = writeVal - 0x06060606; else if (rtlpriv->dm.dynamic_txhighpower_lvl == TXHIGHPWRLEVEL_BT2) - writeVal = writeVal; + ; *(p_outwriteval + rf) = writeVal; } } -- 2.16.0.rc1.238.g530d649a79-goog
[PATCH] nl80211: Fix enum type of variable in nl80211_put_sta_rate()
rate_flg is of type 'enum nl80211_attrs', however it is assigned with 'enum nl80211_rate_info' values. Change the type of rate_flg accordingly. Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- net/wireless/nl80211.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 2312dc2ffdb9..9af21a21ea6b 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -4151,7 +4151,7 @@ static bool nl80211_put_sta_rate(struct sk_buff *msg, struct rate_info *info, struct nlattr *rate; u32 bitrate; u16 bitrate_compat; - enum nl80211_attrs rate_flg; + enum nl80211_rate_info rate_flg; rate = nla_nest_start(msg, attr); if (!rate) -- 2.12.2.762.g0e3151a226-goog
[PATCH] mac80211: ibss: Fix channel type enum in ieee80211_sta_join_ibss()
cfg80211_chandef_create() expects an 'enum nl80211_channel_type' as channel type however in ieee80211_sta_join_ibss() NL80211_CHAN_WIDTH_20_NOHT is passed in two occasions, which is of the enum type 'nl80211_chan_width'. Change the value to NL80211_CHAN_NO_HT (20 MHz, non-HT channel) of the channel type enum. Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- net/mac80211/ibss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c index 98999d3d5262..e957351976a2 100644 --- a/net/mac80211/ibss.c +++ b/net/mac80211/ibss.c @@ -425,7 +425,7 @@ static void ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata, case NL80211_CHAN_WIDTH_5: case NL80211_CHAN_WIDTH_10: cfg80211_chandef_create(, cbss->channel, - NL80211_CHAN_WIDTH_20_NOHT); + NL80211_CHAN_NO_HT); chandef.width = sdata->u.ibss.chandef.width; break; case NL80211_CHAN_WIDTH_80: @@ -437,7 +437,7 @@ static void ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata, default: /* fall back to 20 MHz for unsupported modes */ cfg80211_chandef_create(, cbss->channel, - NL80211_CHAN_WIDTH_20_NOHT); + NL80211_CHAN_NO_HT); break; } -- 2.12.2.762.g0e3151a226-goog
[PATCH v2] cfg80211: Fix array-bounds warning in fragment copy
__ieee80211_amsdu_copy_frag intentionally initializes a pointer to
array[-1] to increment it later to valid values. clang rightfully
generates an array-bounds warning on the initialization statement.
Initialize the pointer to array[0] and change the algorithm from
increment before to increment after consume.
Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
Note: Resent to include linux-wireless in cc
net/wireless/util.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 68e5f2ecee1a..52795ae5337f 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct
sk_buff *frame,
int offset, int len)
{
struct skb_shared_info *sh = skb_shinfo(skb);
- const skb_frag_t *frag = >frags[-1];
+ const skb_frag_t *frag = >frags[0];
struct page *frag_page;
void *frag_ptr;
int frag_len, frag_size;
@@ -672,10 +672,10 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct
sk_buff *frame,
while (offset >= frag_size) {
offset -= frag_size;
- frag++;
frag_page = skb_frag_page(frag);
frag_ptr = skb_frag_address(frag);
frag_size = skb_frag_size(frag);
+ frag++;
}
frag_ptr += offset;
@@ -687,12 +687,12 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct
sk_buff *frame,
len -= cur_len;
while (len > 0) {
- frag++;
frag_len = skb_frag_size(frag);
cur_len = min(len, frag_len);
__frame_add_frag(frame, skb_frag_page(frag),
skb_frag_address(frag), cur_len, frag_len);
len -= cur_len;
+ frag++;
}
}
--
2.12.2.715.g7642488e1d-goog
Re: [PATCH] ath9k: Add cast to u8 to FREQ2FBIN macro
Hi Joe,
El Thu, Apr 06, 2017 at 02:29:20PM -0700 Joe Perches ha dit:
> On Thu, 2017-04-06 at 14:21 -0700, Matthias Kaehlcke wrote:
> > The macro results are assigned to u8 variables/fields. Adding the cast
> > fixes plenty of clang warnings about "implicit conversion from 'int' to
> > 'u8'".
> >
> > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> > ---
> > drivers/net/wireless/ath/ath9k/eeprom.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h
> > b/drivers/net/wireless/ath/ath9k/eeprom.h
> > index 30bf722e33ed..31390af6c33e 100644
> > --- a/drivers/net/wireless/ath/ath9k/eeprom.h
> > +++ b/drivers/net/wireless/ath/ath9k/eeprom.h
> > @@ -106,7 +106,7 @@
> > #define AR9285_RDEXT_DEFAULT0x1F
> >
> > #define ATH9K_POW_SM(_r, _s) (((_r) & 0x3f) << (_s))
> > -#define FREQ2FBIN(x, y)((y) ? ((x) - 2300) : (((x) - 4800) /
> > 5))
> > +#define FREQ2FBIN(x, y)(u8)((y) ? ((x) - 2300) : (((x) - 4800)
> > / 5))
>
> Maybe better to use:
>
> static inline u8 FREQ2FBIN(int x, int y)
> {
> if (y)
> return x - 2300;
> return (x - 4800) / 5;
> }
Thanks for your suggestion! Unfortunately in this case an inline
function is not suitable since FREQ2FBIN() is mostly used for
structure initialization:
static const struct ar9300_eeprom ar9300_default = {
...
.calFreqPier2G = {
FREQ2FBIN(2412, 1),
FREQ2FBIN(2437, 1),
FREQ2FBIN(2472, 1),
},
...
Cheers
Matthias
[PATCH v2] mac80211: Fix clang warning about constant operand in logical operation
When clang detects a non-boolean constant in a logical operation it generates a 'constant-logical-operand' warning. In ieee80211_try_rate_control_ops_get() the result of strlen() is used in a logical operation, clang resolves the expression to an (integer) constant at compile time when clang's builtin strlen function is used. Change the condition to check for strlen() > 0 to make the constant operand boolean and thus avoid the warning. Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- Changes in v2: - Changed expression to strlen() > 0 to make constant operand boolean instead of splitting up condition - Updated commit message net/mac80211/rate.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c index 206698bc93f4..694faf6ab574 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -173,9 +173,11 @@ ieee80211_rate_control_ops_get(const char *name) /* try default if specific alg requested but not found */ ops = ieee80211_try_rate_control_ops_get(ieee80211_default_rc_algo); - /* try built-in one if specific alg requested but not found */ - if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT)) + /* Note: check for > 0 is intentional to avoid clang warning */ + if (!ops && (strlen(CONFIG_MAC80211_RC_DEFAULT) > 0)) + /* try built-in one if specific alg requested but not found */ ops = ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT); + kernel_param_unlock(THIS_MODULE); return ops; -- 2.12.2.715.g7642488e1d-goog
Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation
El Thu, Apr 06, 2017 at 11:12:25PM +0200 Johannes Berg ha dit:
> On Thu, 2017-04-06 at 12:24 -0700, Matthias Kaehlcke wrote:
>
> > I agree that the code looks worse :( I hoped to find a fix using a
> > preprocessor condition but wasn't successful.
>
> It's actually easy - just remove the 'default ""' from Kconfig, and
> then the symbol won't be defined at all if it doesn't get a proper
> value. Then you can ifdef the whole thing.
Thanks, it would also require to move the initialization of
ieee80211_default_rc_algo into an ifdef. If you can live with such a
solution I'm happy to change it.
> > Some projects (like Chrome OS) build their kernel with all warnings
> > being treated as errors. Besides changing the 'offending' code the
> > alternatives are to disable the warning completely or to tell clang
> > not to use the builtin(s). IMO changing the code is the preferable
> > solution, especially since this is so far the only occurrence of the
> > warning that I have encountered.
> >
> > I used goto instead of nested ifs since other functions in this file
> > use the same pattern. If nested ifs are preferred I can change that.
>
> I don't really buy either argument. The warning is simply bogus - I'm
> very surprised you don't hit it with more similar macros or cases, like
> for example CONFIG_ENABLED(). Try
>
> git grep 'IS_ENABLED(' | grep '&&'
>
> and you'll find lots of places that seem like they should trigger this
> warning.
Indeed the warning is not triggered by these constructs. It seems
clang only emits the warning when the constant operand is not boolean.
> You're advocating to make the code worse - not very significantly in
> this case, but still - just to quiet a compiler warning.
For Chrome OS we need to quiet the warning in one way or another,
otherwise our builds will fail due to warnings being treated as
errors. I'm reluctant to disable the warning completely since it can
be useful to detect certain errors, e.g. the use of a logical operator
when bitwise was intended.
And yes, in this case I would favor the slight deterioration of a
small section of code over losing a potentially useful check on the
entire kernel code.
I agree that this is a bit of a corner case, the code is definitely
not buggy by itself, ideally clang would detect that the constant is
a result of its own optimization and skip the warning.
Obviously we can always apply a patch locally, but ideally we try to
upstream changes that seem of general use so that others and our
future selves can benefit from it.
I have no intention to insist on this, we can live with a local patch
if you don't think it is useful.
Cheers
Matthias
[PATCH] ath9k: Add cast to u8 to FREQ2FBIN macro
The macro results are assigned to u8 variables/fields. Adding the cast fixes plenty of clang warnings about "implicit conversion from 'int' to 'u8'". Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- drivers/net/wireless/ath/ath9k/eeprom.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h index 30bf722e33ed..31390af6c33e 100644 --- a/drivers/net/wireless/ath/ath9k/eeprom.h +++ b/drivers/net/wireless/ath/ath9k/eeprom.h @@ -106,7 +106,7 @@ #define AR9285_RDEXT_DEFAULT0x1F #define ATH9K_POW_SM(_r, _s) (((_r) & 0x3f) << (_s)) -#define FREQ2FBIN(x, y)((y) ? ((x) - 2300) : (((x) - 4800) / 5)) +#define FREQ2FBIN(x, y)(u8)((y) ? ((x) - 2300) : (((x) - 4800) / 5)) #define FBIN2FREQ(x, y)((y) ? (2300 + x) : (4800 + 5 * x)) #define ath9k_hw_use_flash(_ah)(!(_ah->ah_flags & AH_USE_EEPROM)) -- 2.12.2.715.g7642488e1d-goog
Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation
Hi Johannes, thanks for your comments El Thu, Apr 06, 2017 at 09:11:18PM +0200 Johannes Berg ha dit: > On Thu, 2017-04-06 at 11:56 -0700, Matthias Kaehlcke wrote: > > Clang raises a warning about the expression 'strlen(CONFIG_XXX)' > > being > > used in a logical operation. Clangs' builtin strlen function resolves > > the > > expression to a constant at compile time, which causes clang to > > generate > > a 'constant-logical-operand' warning. > > > > Split the if statement in two to avoid using the const expression in > > a logical operation. > > > I don't really see all much point in doing this for the warning's > sake... hopefully it doesn't actually generate worse code, but I think > the code ends up looking worse and people will forever wonder what the > goto is really doing there. I agree that the code looks worse :( I hoped to find a fix using a preprocessor condition but wasn't successful. Some projects (like Chrome OS) build their kernel with all warnings being treated as errors. Besides changing the 'offending' code the alternatives are to disable the warning completely or to tell clang not to use the builtin(s). IMO changing the code is the preferable solution, especially since this is so far the only occurrence of the warning that I have encountered. I used goto instead of nested ifs since other functions in this file use the same pattern. If nested ifs are preferred I can change that. Cheers Matthias
[PATCH] mac80211: Fix clang warning about constant operand in logical operation
Clang raises a warning about the expression 'strlen(CONFIG_XXX)' being used in a logical operation. Clangs' builtin strlen function resolves the expression to a constant at compile time, which causes clang to generate a 'constant-logical-operand' warning. Split the if statement in two to avoid using the const expression in a logical operation. Signed-off-by: Matthias Kaehlcke <m...@chromium.org> --- net/mac80211/rate.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c index 206698bc93f4..68ff202d6380 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -173,9 +173,14 @@ ieee80211_rate_control_ops_get(const char *name) /* try default if specific alg requested but not found */ ops = ieee80211_try_rate_control_ops_get(ieee80211_default_rc_algo); + if (ops) + goto unlock; + /* try built-in one if specific alg requested but not found */ - if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT)) + if (strlen(CONFIG_MAC80211_RC_DEFAULT)) ops = ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT); + +unlock: kernel_param_unlock(THIS_MODULE); return ops; -- 2.12.2.715.g7642488e1d-goog
