On 06/01/2013 01:11 AM, Dan Carpenter wrote:
This one will need to be redone. It introduces new GCC warnings:drivers/staging/rtl8192u/r8192U_core.c:1958:9: warning: mixing declarations and code drivers/staging/rtl8192u/r8192U_core.c:2698:25: warning: mixing declarations and code drivers/staging/rtl8192u/r8192U_core.c:2809:9: warning: mixing declarations and code drivers/staging/rtl8192u/r8192U_core.c:2895:9: warning: mixing declarations and code drivers/staging/rtl8192u/r8192U_core.c:3007:17: warning: mixing declarations and code drivers/staging/rtl8192u/r8192U_core.c:3019:9: warning: mixing declarations and code People use this trick: { int my_local_var; blah; blah; blah; } You will need to move the declaration to start of the function when you delete the block. Often the code does this: #ifdef DEAD_CODE { int my_local_var; blah; blah; blah; } #endif My suggestion is to go through and delete the dead code first in a separate patch. Also really this patch is pretty huge. With all the indent changes and everything it's a bit hard to review, and I knew you were going to have to redo it anyway so I didn't review until the end. One way to split this up would be: [patch 1/3] remove ifdefed out dead code [patch 2/3] move braces around but don't add or delete any brace or change indent levels [patch 3/3] remove unneeded block statements and pull things in an indent level The subjects are bad but you get the idea.
Ok i see. I will try this.
On Fri, May 31, 2013 at 08:10:49PM +0300, Xenia Ragiadakou wrote:if (priv->ieee80211->iw_mode == IW_MODE_MONITOR || \You didn't add this, but these '\' characters are pointless. This isn't a macro.
I have removed these line continuations in RFC 4/5.
- dev->flags& IFF_PROMISC){ + dev->flags& IFF_PROMISC) rxconf = rxconf | RCR_AAP; - } else{ + else {You can fix this in a follow on patch, but this isn't right. The rule is that if either side of the if else statement has curly braces then both sides get them.
I did not know that. Thx I will fix it.
rxconf = rxconf | RCR_APM; rxconf = rxconf | RCR_CBSSID; } @@ -1441,13 +1429,10 @@ void rtl8192_update_cap(struct net_device *dev, u16 cap) tmp |= BRSR_AckShortPmb; write_nic_dword(dev, RRSR, tmp); - if (net->mode& (IEEE_G|IEEE_N_24G)) - { + if (net->mode& (IEEE_G|IEEE_N_24G)) { u8 slot_time = 0; - if ((cap& WLAN_CAPABILITY_SHORT_SLOT)&&(!priv->ieee80211->pHTInfo->bCurrentRT2RTLongSlotTime)) - {//short slot time + if ((cap& WLAN_CAPABILITY_SHORT_SLOT)&&(!priv->ieee80211->pHTInfo->bCurrentRT2RTLongSlotTime)) //short slot time^^^^^^^^^^^^^^^ This is a pointless comment. Delete.
I will delete all pointless comments in a following patch.
slot_time = SHORT_SLOT_TIME; - } else //long slot time slot_time = NON_SHORT_SLOT_TIME; priv->slot_time = slot_time; @@ -2563,15 +2503,12 @@ static void rtl8192_init_priv_variable(struct net_device *dev) skb_queue_head_init(&priv->skb_queue); /* Tx related queue */ - for (i = 0; i< MAX_QUEUE_SIZE; i++) { + for (i = 0; i< MAX_QUEUE_SIZE; i++) skb_queue_head_init(&priv->ieee80211->skb_waitQ [i]); - } - for (i = 0; i< MAX_QUEUE_SIZE; i++) { + for (i = 0; i< MAX_QUEUE_SIZE; i++) skb_queue_head_init(&priv->ieee80211->skb_aggQ [i]); - } - for (i = 0; i< MAX_QUEUE_SIZE; i++) { + for (i = 0; i< MAX_QUEUE_SIZE; i++) skb_queue_head_init(&priv->ieee80211->skb_drv_aggQ [i]); - }In a later patch you can change this to: for (i = 0; i< MAX_QUEUE_SIZE; i++) { skb_queue_head_init(&priv->ieee80211->skb_waitQ[i]); skb_queue_head_init(&priv->ieee80211->skb_aggQ[i]); skb_queue_head_init(&priv->ieee80211->skb_drv_aggQ[i]); } And the 'Q' on the end is bad CamelCase and it's bad kernel style. "skb_waitQ" is a horrible name. Neither "skb" nor "waitQ/wait_queue" actually tell you any information at all about what it's for. :/
Yes, I have noticed it. I will fix it in a following patch.
regards, dan carpenter
Thanks again for your suggestions. I will update it and resend it. Ksenia _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
