From: Dan Carpenter <dan.carpenter@oracle.com>
To: kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 1/9] Staging:wlan-ng:cfg80211.c,p80211conv.h,p80211metastruct.h,p80211netdev.h:
Date: Wed, 24 Jul 2013 15:04:26 +0000 [thread overview]
Message-ID: <20130724150425.GV5585@mwanda> (raw)
In-Reply-To: <1374673972-10041-1-git-send-email-kumargauravgupta3@gmail.com>
Everyone's first patch is rejected, and this is a newbie friendly
list so don't worry that this patch is rejected.
On Wed, Jul 24, 2013 at 07:22:52PM +0530, Kumar Gaurav wrote:
> Fixed coding style issues
> ---
It doesn't apply. Send it to yourself first and figure that bit
out. It has to apply with `cat email.txt | patch -p1` or better
yet `cat email.txt | git am`.
It needs a signed off line.
This patch does too many things at once and needs to be split into
multiple patches. One patch to rename the defines, one to rework
the includes,
> linux-3.11-rc2/drivers/staging/wlan-ng/cfg80211.c | 99 +++++++++++++-------
> .../drivers/staging/wlan-ng/p80211conv.h | 1 +
> .../drivers/staging/wlan-ng/p80211metastruct.h | 2 +
> .../drivers/staging/wlan-ng/p80211netdev.h | 9 +-
> 4 files changed, 75 insertions(+), 36 deletions(-)
>
> diff --git a/linux-3.11-rc2/drivers/staging/wlan-ng/cfg80211.c b/linux-3.11-rc2/drivers/staging/wlan-ng/cfg80211.c
> index f1bce18..a3f54f4 100644
> --- a/linux-3.11-rc2/drivers/staging/wlan-ng/cfg80211.c
> +++ b/linux-3.11-rc2/drivers/staging/wlan-ng/cfg80211.c
> @@ -2,7 +2,20 @@
>
>
> /* Prism2 channel/frequency/bitrate declarations */
> -static const struct ieee80211_channel prism2_channels[] = {
> +/*define statement for making varnames more understandable and short*/
> +#define WEPDEFKEY0 DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey0
> +#define WEPDEFKEY1 DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey1
> +#define WEPDEFKEY2 DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey2
> +#define WEPDEFKEY3 DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey3
> +#define RTSTHREASHOLD DIDmib_dot11mac_dot11OperationTable_dot11RTSThreshold
Spelling mistake. Should be THRESHOLD.
> +#define FRMTHLD DIDmib_dot11mac_dot11OperationTable_dot11FragmentationThreshold
> +#define CRRCHNL DIDmib_dot11phy_dot11PhyDSSSTable_dot11CurrentChannel
> +#define DEFWEPKEYID DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID
> +#define PRVCINVKD DIDmib_dot11smt_dot11PrivacyTable_dot11PrivacyInvoked
> +#define EXCLUNENC DIDmib_dot11smt_dot11PrivacyTable_dot11ExcludeUnencrypted
Instead of having a reasonable name and a way too loooooonng,
nonsense name, this should only have a reasonable name. The long
name should be deleted.
These names are a bit too generic, they should have a prefix to
show which driver they are associated with. They are hard to read
without underscores to separate the words.
P80211DID_DEF_KEY0
P80211DID_RTS_THRESHOLD
P80211DID_FRM_THRESHOLD
P80211DID_CUR_CHAN
P80211DID_DEF_KEYID
P80211DID_PRV_INVKD
P80211DID_EXCL_UNENC
> +
> +
Don't put two blank lines in a row.
> +static const struct ieee80211_channel prism2_channels[14] = {
> { .center_freq = 2412 },
> { .center_freq = 2417 },
> { .center_freq = 2422 },
> @@ -73,7 +86,8 @@ static int prism2_result2err(int prism2_result)
> static int prism2_domibset_uint32(wlandevice_t *wlandev, u32 did, u32 data)
> {
> struct p80211msg_dot11req_mibset msg;
> - p80211item_uint32_t *mibitem = (p80211item_uint32_t *) &msg.mibattribute.data;
> + p80211item_uint32_t *mibitem
> + = (p80211item_uint32_t *) &msg.mibattribute.data;
>
Just do these like this:
p80211item_uint32_t *mibitem;
mibitem = (p80211item_uint32_t *)&msg.mibattribute.data;
> msg.msgcode = DIDmsg_dot11req_mibset;
> mibitem->did = did;
> @@ -86,7 +100,8 @@ static int prism2_domibset_pstr32(wlandevice_t *wlandev,
> u32 did, u8 len, u8 *data)
> {
> struct p80211msg_dot11req_mibset msg;
> - p80211item_pstr32_t *mibitem = (p80211item_pstr32_t *) &msg.mibattribute.data;
> + p80211item_pstr32_t *mibitem
> + = (p80211item_pstr32_t *) &msg.mibattribute.data;
>
> msg.msgcode = DIDmsg_dot11req_mibset;
> mibitem->did = did;
> @@ -122,7 +137,7 @@ int prism2_change_virtual_intf(struct wiphy *wiphy,
> data = 1;
> break;
> default:
> - printk(KERN_WARNING "Operation mode: %d not support\n", type);
> + pr_warn(KERN_WARNING "Operation mode: %d not support\n", type);
This isn't how this works. I'm surprised this compiles actually.
I can't test compiling this because it doesn't apply.
> return -EOPNOTSUPP;
> }
>
> @@ -154,7 +169,7 @@ int prism2_add_key(struct wiphy *wiphy, struct net_device *dev,
> case WLAN_CIPHER_SUITE_WEP40:
> case WLAN_CIPHER_SUITE_WEP104:
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
> + DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
The original is better even though it goes over the 80 character
limit.
> key_index);
> if (result)
> goto exit;
> @@ -162,19 +177,19 @@ int prism2_add_key(struct wiphy *wiphy, struct net_device *dev,
> /* send key to driver */
> switch (key_index) {
> case 0:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey0;
> + did = WEPDEFKEY0;
> break;
>
> case 1:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey1;
> + did = WEPDEFKEY1;
> break;
>
> case 2:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey2;
> + did = WEPDEFKEY2;
> break;
>
> case 3:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey3;
> + did = WEPDEFKEY3;
> break;
>
> default:
> @@ -182,7 +197,8 @@ int prism2_add_key(struct wiphy *wiphy, struct net_device *dev,
> goto exit;
> }
>
> - result = prism2_domibset_pstr32(wlandev, did, params->key_len, params->key);
> + result = prism2_domibset_pstr32(wlandev, did, params->key_len,
> + params->key);
> if (result)
> goto exit;
> break;
> @@ -200,7 +216,8 @@ exit:
> }
>
> int prism2_get_key(struct wiphy *wiphy, struct net_device *dev,
> - u8 key_index, bool pairwise, const u8 *mac_addr, void *cookie,
> + u8 key_index, bool pairwise, const u8 *mac_addr,
> + void *cookie,
> void (*callback)(void *cookie, struct key_params*))
> {
> wlandevice_t *wlandev = dev->ml_priv;
> @@ -242,22 +259,22 @@ int prism2_del_key(struct wiphy *wiphy, struct net_device *dev,
> switch (key_index) {
> case 0:
> did > - DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey0;
> + WEPDEFKEY0;
> break;
>
> case 1:
> did > - DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey1;
> + WEPDEFKEY1;
> break;
>
> case 2:
> did > - DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey2;
> + WEPDEFKEY2;
> break;
>
> case 3:
> did > - DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey3;
> + WEPDEFKEY3;
> break;
>
> default:
> @@ -352,7 +369,7 @@ int prism2_scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
> return -EBUSY;
>
> if (wlandev->macmode = WLAN_MACMODE_ESS_AP) {
> - printk(KERN_ERR "Can't scan in AP mode\n");
> + pr_err(KERN_ERR "Can't scan in AP mode\n");
> return -EOPNOTSUPP;
> }
>
> @@ -379,7 +396,9 @@ int prism2_scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
> (i < request->n_channels) && i < ARRAY_SIZE(prism2_channels);
> i++)
> msg1.channellist.data.data[i] > - ieee80211_frequency_to_channel(request->channels[i]->center_freq);
> + ieee80211_frequency_to_channel(
> + request->channels[i]->center_freq
> + );
The original was easier to read.
> msg1.channellist.data.len = request->n_channels;
>
> msg1.maxchanneltime.data = 250;
> @@ -409,13 +428,17 @@ int prism2_scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
> ie_len = ie_buf[1] + 2;
> memcpy(&ie_buf[2], &(msg2.ssid.data.data), msg2.ssid.data.len);
> bss = cfg80211_inform_bss(wiphy,
> - ieee80211_get_channel(wiphy, ieee80211_dsss_chan_to_freq(msg2.dschannel.data)),
> + ieee80211_get_channel(wiphy,
> + ieee80211_dsss_chan_to_freq(
> + msg2.dschannel.data
> + )),
The original was better.
> (const u8 *) &(msg2.bssid.data.data),
> msg2.timestamp.data, msg2.capinfo.data,
> msg2.beaconperiod.data,
> ie_buf,
> ie_len,
> - (msg2.signal.data - 65536) * 100, /* Conversion to signed type */
> + /* Next line is for Conversion to signed type */
> + (msg2.signal.data - 65536) * 100,
> GFP_KERNEL
> );
>
> @@ -451,7 +474,7 @@ int prism2_set_wiphy_params(struct wiphy *wiphy, u32 changed)
> data = wiphy->rts_threshold;
>
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11mac_dot11OperationTable_dot11RTSThreshold,
> + RTSTHREASHOLD,
> data);
> if (result) {
> err = -EFAULT;
> @@ -466,7 +489,8 @@ int prism2_set_wiphy_params(struct wiphy *wiphy, u32 changed)
> data = wiphy->frag_threshold;
>
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11mac_dot11OperationTable_dot11FragmentationThreshold,
> + /*FRMTHLD is macro*/
> + FRMTHLD,
This is aligned wierdly. This comment is not needed, actually.
> data);
> if (result) {
> err = -EFAULT;
> @@ -487,8 +511,9 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> u32 did;
> int length = sme->ssid_len;
> int chan = -1;
> - int is_wep = (sme->crypto.cipher_group = WLAN_CIPHER_SUITE_WEP40) ||
> - (sme->crypto.cipher_group = WLAN_CIPHER_SUITE_WEP104);
> + int is_wep = (sme->crypto.cipher_group = WLAN_CIPHER_SUITE_WEP40)
> + ||
> + (sme->crypto.cipher_group = WLAN_CIPHER_SUITE_WEP104);
The original was ugly as can be but still better.
> int result;
> int err = 0;
>
> @@ -496,7 +521,8 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> if (channel) {
> chan = ieee80211_frequency_to_channel(channel->center_freq);
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11phy_dot11PhyDSSSTable_dot11CurrentChannel,
> + /*CRRCHNL is macro */
> + CRRCHNL,
> chan);
> if (result)
> goto exit;
> @@ -510,7 +536,7 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> ((sme->auth_type = NL80211_AUTHTYPE_AUTOMATIC) && is_wep))
> msg_join.authtype.data = P80211ENUM_authalg_sharedkey;
> else
> - printk(KERN_WARNING
> + pr_warn(KERN_WARNING
> "Unhandled authorisation type for connect (%d)\n",
> sme->auth_type);
>
> @@ -518,7 +544,8 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> if (is_wep) {
> if (sme->key) {
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
> + /*DEFWEPKEYID is macro*/
> + DEFWEPKEYID,
> sme->key_idx);
> if (result)
> goto exit;
> @@ -526,19 +553,19 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> /* send key to driver */
> switch (sme->key_idx) {
> case 0:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey0;
> + did = WEPDEFKEY0;
> break;
>
> case 1:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey1;
> + did = WEPDEFKEY1;
> break;
>
> case 2:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey2;
> + did = WEPDEFKEY2;
> break;
>
> case 3:
> - did = DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey3;
> + did = WEPDEFKEY3;
> break;
>
> default:
> @@ -558,13 +585,15 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> We could possibly use sme->privacy here, but the assumption
> seems reasonable anyway */
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11smt_dot11PrivacyTable_dot11PrivacyInvoked,
> + /*PRVCINVKD is macro */
> + PRVCINVKD,
> P80211ENUM_truth_true);
> if (result)
> goto exit;
>
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11smt_dot11PrivacyTable_dot11ExcludeUnencrypted,
> + /*EXCLUNENC is macro*/
These sorts of obvious comments are not needed (it's just noise).
> + EXCLUNENC,
> P80211ENUM_truth_true);
> if (result)
> goto exit;
> @@ -573,13 +602,15 @@ int prism2_connect(struct wiphy *wiphy, struct net_device *dev,
> /* Assume we should unset privacy invoked
> and exclude unencrypted */
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11smt_dot11PrivacyTable_dot11PrivacyInvoked,
> + /*PRVCINVKD is macro*/
> + PRVCINVKD,
> P80211ENUM_truth_false);
> if (result)
> goto exit;
>
> result = prism2_domibset_uint32(wlandev,
> - DIDmib_dot11smt_dot11PrivacyTable_dot11ExcludeUnencrypted,
> + /*EXCLUNENC is macro*/
> + EXCLUNENC,
> P80211ENUM_truth_false);
> if (result)
> goto exit;
> diff --git a/linux-3.11-rc2/drivers/staging/wlan-ng/p80211conv.h b/linux-3.11-rc2/drivers/staging/wlan-ng/p80211conv.h
> index e031a74..a4f4add 100644
> --- a/linux-3.11-rc2/drivers/staging/wlan-ng/p80211conv.h
> +++ b/linux-3.11-rc2/drivers/staging/wlan-ng/p80211conv.h
> @@ -53,6 +53,7 @@
> #ifndef _LINUX_P80211CONV_H
> #define _LINUX_P80211CONV_H
>
> +#include "p80211hdr.h"
> #define WLAN_ETHADDR_LEN 6
> #define WLAN_IEEE_OUI_LEN 3
>
> diff --git a/linux-3.11-rc2/drivers/staging/wlan-ng/p80211metastruct.h b/linux-3.11-rc2/drivers/staging/wlan-ng/p80211metastruct.h
> index c501162..e49ab2c 100644
> --- a/linux-3.11-rc2/drivers/staging/wlan-ng/p80211metastruct.h
> +++ b/linux-3.11-rc2/drivers/staging/wlan-ng/p80211metastruct.h
> @@ -46,6 +46,8 @@
>
> #ifndef _P80211MKMETASTRUCT_H
> #define _P80211MKMETASTRUCT_H
> +#include "p80211msg.h"
> +
>
> struct p80211msg_dot11req_mibget {
> u32 msgcode;
> diff --git a/linux-3.11-rc2/drivers/staging/wlan-ng/p80211netdev.h b/linux-3.11-rc2/drivers/staging/wlan-ng/p80211netdev.h
> index 2fecca2..3b4a309 100644
> --- a/linux-3.11-rc2/drivers/staging/wlan-ng/p80211netdev.h
> +++ b/linux-3.11-rc2/drivers/staging/wlan-ng/p80211netdev.h
> @@ -56,6 +56,10 @@
> #include <linux/interrupt.h>
> #include <linux/wireless.h>
> #include <linux/netdevice.h>
> +#include "p80211msg.h"
> +#include "p80211conv.h"
> +#include "p80211hdr.h"
> +#include "p80211types.h"
>
This stuff wasn't described in the changelog.
> #undef netdevice_t
> typedef struct net_device netdevice_t;
> @@ -141,7 +145,7 @@ typedef struct p80211_frmrx_t {
> struct iw_statistics *p80211wext_get_wireless_stats(netdevice_t * dev);
> /* wireless extensions' ioctls */
> extern struct iw_handler_def p80211wext_handler_def;
> -int p80211wext_event_associated(struct wlandevice *wlandev, int assoc);
> +
>
> /* WEP stuff */
> #define NUM_WEPKEYS 4
> @@ -226,7 +230,8 @@ typedef struct wlandevice {
> char spy_address[IW_MAX_SPY][ETH_ALEN];
> struct iw_quality spy_stat[IW_MAX_SPY];
> } wlandevice_t;
> -
> +//Reposition below definition to aviod warning
This comment is not wanted. The warning should be pasted in the
changelog.
> +int p80211wext_event_associated(struct wlandevice *wlandev, int assoc);
> /* WEP stuff */
> int wep_change_key(wlandevice_t *wlandev, int keynum, u8 *key, int keylen);
> int wep_decrypt(wlandevice_t *wlandev, u8 *buf, u32 len, int key_override,
regards,
dan carpenter
next prev parent reply other threads:[~2013-07-24 15:04 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-24 14:04 [PATCH 1/9] Staging:wlan-ng:cfg80211.c,p80211conv.h,p80211metastruct.h,p80211netdev.h: Fixed code st Kumar Gaurav
2013-07-24 15:04 ` Dan Carpenter [this message]
2013-07-24 15:37 ` [PATCH 1/9] Staging:wlan-ng:cfg80211.c,p80211conv.h,p80211metastruct.h,p80211netdev.h: Fixed cod Kumar Gaurav
2013-07-24 16:01 ` [PATCH 1/9] Staging:wlan-ng:cfg80211.c,p80211conv.h,p80211metastruct.h,p80211netdev.h: Dan Carpenter
2013-07-24 16:10 ` Dan Carpenter
2013-07-24 16:19 ` [PATCH 1/9] Staging:wlan-ng:cfg80211.c,p80211conv.h,p80211metastruct.h,p80211netdev.h: Fixed cod Kumar Gaurav
2013-07-25 14:23 ` [PATCH 1/9] Staging:wlan-ng:cfg80211.c,p80211conv.h,p80211metastruct.h,p80211netdev.h: Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130724150425.GV5585@mwanda \
--to=dan.carpenter@oracle.com \
--cc=kernel-janitors@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox