| Message ID | 5687E203.1070404@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hello, I have taken another look at the implementation of the function "rsi_send_mgmt_pkt". https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/net/wireless/rsi/rsi_91x_pkt.c?id=e8c58e7a5a106c3d557fccd01cd4d1128f9bab38#n114 I find the following statement combination interesting there. … u8 vap_id = 0; … msg[7] |= cpu_to_le16(vap_id << 8); … I would appreciate a further clarification. Does a shift operation for a variable which contains zero indicate an open issue? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus,
[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.4-rc7 next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/net-rsi-Fine-tuning-for-two-function-implementations/20160102-224740
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: x86_64-randconfig-s2-01030012 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/net/wireless/rsi/rsi_91x_pkt.c: In function 'rsi_send_mgmt_pkt':
>> drivers/net/wireless/rsi/rsi_91x_pkt.c:211:2: warning: 'status' may be used uninitialized in this function [-Wmaybe-uninitialized]
rsi_indicate_tx_status(common->priv, skb, status);
^
vim +/status +211 drivers/net/wireless/rsi/rsi_91x_pkt.c
dad0d04f Fariya Fatima 2014-03-16 195 /* Indicate to firmware to give cfm */
dad0d04f Fariya Fatima 2014-03-16 196 if ((skb->data[16] == IEEE80211_STYPE_PROBE_REQ) && (!bss->assoc)) {
dad0d04f Fariya Fatima 2014-03-16 197 msg[1] |= cpu_to_le16(BIT(10));
dad0d04f Fariya Fatima 2014-03-16 198 msg[7] = cpu_to_le16(PROBEREQ_CONFIRM);
dad0d04f Fariya Fatima 2014-03-16 199 common->mgmt_q_block = true;
dad0d04f Fariya Fatima 2014-03-16 200 }
dad0d04f Fariya Fatima 2014-03-16 201
dad0d04f Fariya Fatima 2014-03-16 202 msg[7] |= cpu_to_le16(vap_id << 8);
dad0d04f Fariya Fatima 2014-03-16 203
dad0d04f Fariya Fatima 2014-03-16 204 status = adapter->host_intf_write_pkt(common->priv,
dad0d04f Fariya Fatima 2014-03-16 205 (u8 *)msg,
dad0d04f Fariya Fatima 2014-03-16 206 skb->len);
dad0d04f Fariya Fatima 2014-03-16 207 if (status)
dad0d04f Fariya Fatima 2014-03-16 208 rsi_dbg(ERR_ZONE, "%s: Failed to write the packet\n", __func__);
dad0d04f Fariya Fatima 2014-03-16 209
dad0d04f Fariya Fatima 2014-03-16 210 err:
dad0d04f Fariya Fatima 2014-03-16 @211 rsi_indicate_tx_status(common->priv, skb, status);
dad0d04f Fariya Fatima 2014-03-16 212 return status;
dad0d04f Fariya Fatima 2014-03-16 213 }
:::::: The code at line 211 was first introduced by commit
:::::: dad0d04fa7ba41ce603a01e8e64967650303e9a2 rsi: Add RS9113 wireless driver
:::::: TO: Fariya Fatima <fariyaf@gmail.com>
:::::: CC: John W. Linville <linville@tuxdriver.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
These patches are labour intensive to review because you can't just do it in the email client. Also you were not able to review it properly yourself and introduced a bug. I am often remove initializers but it's normally because I am changing something else which makes it worthwhile. This patch is the correct thing but it's not "worthwhile". It is not a good use of my time. Please stop sending cleanup patches, Markus. Just send fixes. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Btw, GCC misses a lot of uninitialized variable bugs. I have a Smatch check which sometimes catches the bugs that GCC misses but you should not rely on the tools here. These patches need to be reviewed manually. And the "goto err" before the initialization makes everything more complicated (that's actually what caused the bug in this patch, in fact). regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> These patches are labour intensive to review because you can't just do > it in the email client. Thanks for your general interest. > Also you were not able to review it properly yourself and introduced > a bug. I admit that it can happen during my software development that I overlook implementation details somehow. > I am often remove initializers but it's normally because I am changing > something else which makes it worthwhile. It is nice to hear that you are also occasionally looking for similar update candidates. > This patch is the correct thing but it's not "worthwhile". I find this view interesting. > Please stop sending cleanup patches, Markus. Just send fixes. How often will source code clean-up fix something? May I resend a consistent patch series for the source file "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 04, 2016 at 11:44:15AM +0100, SF Markus Elfring wrote: > > Please stop sending cleanup patches, Markus. Just send fixes. > > How often will source code clean-up fix something? > > > May I resend a consistent patch series for the source file > "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future? If you were sending checkpatch.pl fixes that would be easier to deal with but you are sending hundreds of "controversial" cleanups. They are controversial in the sense that they don't fix anything against official kernel style and they go against the author's original intention. I tend to agree that useless initializers are bad and disable GCCs uninitialized variable warnings but just because I agree with you doesn't make it official kernel style. It's slightly rude to go against the author's intention. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> May I resend a consistent patch series for the source file >> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future? > > If you were sending checkpatch.pl fixes that would be easier to deal with Does this feedback mean that you would accept any more suggestions around source code updates which are derived from recommendations of this script? > but you are sending hundreds of "controversial" cleanups. It depends on the time range you look at for my proposals. > They are controversial in the sense that they don't fix anything > against official kernel style I find that I suggested also few changes that fit to this aspect. > and they go against the author's original intention. Can it occasionally help to reconsider the "first approach"? > I tend to agree that useless initializers are bad Would any more software developers like to share their opinions on this detail? > and disable GCCs uninitialized variable warnings I hope that this software area can be also improved. > but just because I agree with you doesn't make it official kernel style. That is fine. - Will it become useful to clarify any extensions to a document like "CodingStyle"? > It's slightly rude to go against the author's intention. I just dare to propose further special changes. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dan Carpenter <dan.carpenter@oracle.com> writes:
> Please stop sending cleanup patches, Markus. Just send fixes.
Thanks for your continued but unwarranted belief in AI.
Do you mind if I remind you of https://lkml.org/lkml/2014/11/3/162 ?
I am sure there are lots and lots of other examples. There is no reason
to believe this will ever stop. He just goes into Eliza mode.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 04, 2016 at 02:17:40PM +0100, Bjørn Mork wrote: > Dan Carpenter <dan.carpenter@oracle.com> writes: > > > Please stop sending cleanup patches, Markus. Just send fixes. > > Thanks for your continued but unwarranted belief in AI. > I always tell people that I am very mechanical and you can rely on me to send predictable responses... > Do you mind if I remind you of https://lkml.org/lkml/2014/11/3/162 ? > I am sure there are lots and lots of other examples. There is no reason > to believe this will ever stop. He just goes into Eliza mode. Yup. I feel some sense of responsibility for any patches where kernel-janitors is on the CC but I'm having a hard time dealing with all of Markus's patches. Normally you just respond to the first patch and people change the later patches but, as you put it, Markus just goes into ELIZA mode. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Mon, 4 Jan 2016 12:28:57 +0300 > Please stop sending cleanup patches, Markus. Just send fixes. +1 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On Mon, Jan 4, 2016 at 11:33 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> May I resend a consistent patch series for the source file >>> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future? >> >> If you were sending checkpatch.pl fixes that would be easier to deal with > > Does this feedback mean that you would accept any more suggestions around > source code updates which are derived from recommendations of this script? A good rule of thumb here would be that if people start complaining about a particular type of change, stop sending them. Another good rule of thumb is to try to "rock the boat" on coding style and conventions as little as possible. Just because it's possible doesn't mean that people want to do it. That said, if you figure out some change that produces significant reductions in code or binary size on multiple architectures without making things more complicated, less readable or making the code or binary size larger, then by all means propose it. "This makes things smaller" carries much more weight than "I think this is better". Almost all of the changes you've proposed that have seen any discussion whatsoever fall into the latter category. Thanks,
> That said, if you figure out some change that produces significant > reductions in code or binary size on multiple architectures without > making things more complicated, less readable or making the code or > binary size larger, then by all means propose it. Are you looking also for "a proof" that such changes are worthwhile? > "This makes things smaller" carries much more weight than > "I think this is better". Can the discussed implementation of a function like "rsi_send_mgmt_pkt" become a bit smaller by the deletion of extra variable initialisations > Almost all of the changes you've proposed that have seen any > discussion whatsoever fall into the latter category. Thanks for your interesting feedback. Can a further constructive dialogue evolve from the presented information? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On Tue, Jan 5, 2016 at 7:29 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >> That said, if you figure out some change that produces significant >> reductions in code or binary size on multiple architectures without >> making things more complicated, less readable or making the code or >> binary size larger, then by all means propose it. > > Are you looking also for "a proof" that such changes are worthwhile? It'd be better than "I think doing things this way is better", which is the hallmark of most of your patch sets. (Admittedly not this one, but this one is where the discussion is now, so that's where we're discussing it.) >> "This makes things smaller" carries much more weight than >> "I think this is better". > > Can the discussed implementation of a function like "rsi_send_mgmt_pkt" > become a bit smaller by the deletion of extra variable initialisations I'm talking in general. In this case you're asking people to review a patch which requires a lot of careful review for a fairly minor improvement. I must also note that you haven't CC'd the people who wrote this driver, so it's possible that the only people who have reviewed it aren't experts in the code. The patches you sent recently which moved labels into if statements were a clear case of "I think this is better" where any actual benefit from the changes was eclipsed by the style and readability issues they introduced. >> Almost all of the changes you've proposed that have seen any >> discussion whatsoever fall into the latter category. > > Thanks for your interesting feedback. No problem. > Can a further constructive dialogue evolve from the presented information? Part of the issue here is that you don't seem to be listening to the discussion of your patches, or if you are, you're not significantly changing your approach or attitude in response. Every time you send a set of patches, there are legitimate issues which people raise, and every time they are discussed, you assert that your patches improve things and seem to ignore the concerns people raise. I've seen this same pattern of discussion here with these patches, with your patches to move labels into if statements, with the patches you sent late June last year, your patches to remove conditions before kfree() and friends, etc. You need to change you attitude: just because you can see some benefit from your patches doesn't mean others do and it doesn't mean that they're willing to accept them. Thanks,
> Every time you send a set of patches, I suggested some updates for Linux source files since October 2014. > there are legitimate issues which people raise, There was usual feedback. > and every time they are discussed, The discussion results were mixed between acceptance and usual disagreement. > you assert that your patches improve things I guess that should be the default intention of every patch, shouldn't it? > and seem to ignore the concerns people raise. I hope not. - But I can imagine that you might understand some responses from contributors in this way. Are you waiting for another clarification on a specific issue? > I've seen this same pattern of discussion here with these patches, > with your patches to move labels into if statements, with the patches > you sent late June last year, your patches to remove conditions before > kfree() and friends, etc. It seems that communication difficulties come partly from the fact that I chose search patterns from static source code analysis so far which belong to an error category that gets a lower priority. > You need to change you attitude: just because you can see some benefit > from your patches doesn't mean others do and it doesn't mean that > they're willing to accept them. I understand your advice. Further update suggestions with higher importance might follow for various software areas in the future. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c index 702593f..ee98f5b 100644 --- a/drivers/net/wireless/rsi/rsi_91x_pkt.c +++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c @@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common, struct sk_buff *skb) { struct rsi_hw *adapter = common->priv; - struct ieee80211_hdr *wh = NULL; + struct ieee80211_hdr *wh; struct ieee80211_tx_info *info; - struct ieee80211_bss_conf *bss = NULL; + struct ieee80211_bss_conf *bss; struct ieee80211_hw *hw = adapter->hw; struct ieee80211_conf *conf = &hw->conf; struct skb_info *tx_params; - int status = -E2BIG; - __le16 *msg = NULL; - u8 extnd_size = 0; + int status; + __le16 *msg; + u8 extnd_size; u8 vap_id = 0; info = IEEE80211_SKB_CB(skb);