staging:rtl8192u: Style and a question?

PATCH In review John Whitmore 3 participants 7 messages Aug 28, 2018 – Aug 29, 2018 Staging

Share & tools

John Whitmore

author

2018-08-28 18:55 UTC #0 lore raw

cover staging:rtl8192u: Style and a question?

This short series of coding style changes clears the remaining checkpatch issues in the files r8180_93cx6.h and dot11d.h. The file dot11d.h does still contain lines over the 80 character limit, but most of these lines are MACRO definitions, and I'm not sure splitting them will add anything to the style and readability of the code.

My question is with respect to the member variable 'enabled' which has been refactored in the third patch of the series. I've renamed the variable to 'dot11d_enabled', (in addition to changing it's type as suggested by checkpatch), so that it is easier to search for in code. Now that it is easier to search for 'dot11d_enabled' and it's associated macro IS_DOT11D_ENABLE(ieee) it becomes apparent that the variable is initialised to FALSE, (and elsewhere '0'), but appears never to be set.

The function dot11d_update_country_ie(), in file dot11d.c, states in its header block comment that it assumes that the value of dot11d_enabled is true.

The dot11d_info structure is allocated:

ieee->dot11d_info = kzalloc(sizeof(struct rt_dot11d_info), GFP_KERNEL);

And dot11d_enabled could be set with a memcopy or a memset, but if it is being set I've missed it completely. There may be a kernel specific mechanism which I'm not yet familiar with?

In my ignorance I'd be tempted to remove all code which relies on the variable being True, but then that function dot11d_update_country_ie() specifically assumes it's true and is an exported symbol.

John Whitmore (4):
staging:rtl8192u: Remove unused definitions - Style staging:rtl8192u: Add required SPDX-License-Identifier - Style staging:rtl8192u: Refactor member variable enabled - Style staging:rtl8192u: Rename member pDot11dInfo - Style

 drivers/staging/rtl8192u/ieee80211/dot11d.c   | 66 +++++++++----------
 drivers/staging/rtl8192u/ieee80211/dot11d.h   |  6 +-
 .../staging/rtl8192u/ieee80211/ieee80211.h    |  2 +-
 .../rtl8192u/ieee80211/ieee80211_softmac.c    |  8 +--
 drivers/staging/rtl8192u/r8180_93cx6.h        | 19 +-----
 drivers/staging/rtl8192u/r8192U_core.c        |  2 +-
 6 files changed, 43 insertions(+), 60 deletions(-)

Signature · 1 line

2.18.0

John Whitmore

author

2018-08-28 18:55 UTC #1 lore raw

PATCH 1/4 staging:rtl8192u: Remove unused definitions - Style

Remove unused definitions from the file r8180_93cx6.h.

This is a coding style change which should have no impact on runtime code execution.

Signed-off-by John Whitmore <[email protected]>

---
 drivers/staging/rtl8192u/r8180_93cx6.h | 18 ------------------
 1 file changed, 18 deletions(-)

drivers/staging/rtl8192u/r8180_93cx6.h +0 ~~-18~~

index 643d465e7105..bfbef1a16159 100644
@@ -21,22 +21,4 @@
 
 #define EPROM_DELAY 10
 
-#define EPROM_ANAPARAM_ADDRLWORD 0xd
-#define EPROM_ANAPARAM_ADDRHWORD 0xe
-
-#define EPROM_RFCHIPID 0x6
-#define EPROM_TXPW_BASE 0x05
-#define EPROM_RFCHIPID_RTL8225U 5
-#define EPROM_RF_PARAM 0x4
-#define EPROM_CONFIG2 0xc
-
-#define EPROM_VERSION 0x1E
-#define MAC_ADR 0x7
-
-#define CIS 0x18
-
-#define EPROM_TXPW0 0x16
-#define EPROM_TXPW2 0x1b
-#define EPROM_TXPW1 0x3d
-
 int eprom_read(struct net_device *dev, u32 addr); /* reads a 16 bits word */

Signature · 1 line

2.18.0

John Whitmore

author

2018-08-28 18:55 UTC #2 lore raw

PATCH 2/4 staging:rtl8192u: Add required SPDX-License-Identifier - Style

In reply to John Whitmore

Add the required SPDX-License-Identifier to the file r8180_93cx6.h, this clears a checkpatch issue.

This is a coding style change which should have no impact on runtime code execution.

Signed-off-by John Whitmore <[email protected]>

---
 drivers/staging/rtl8192u/r8180_93cx6.h | 1 +
 1 file changed, 1 insertion(+)

drivers/staging/rtl8192u/r8180_93cx6.h +1 -0

index bfbef1a16159..0cdd00a4f7b8 100644
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  *	This is part of rtl8187 OpenSource driver
  *	Copyright (C) Andrea Merello 2004-2005  <[email protected]>

Signature · 1 line

2.18.0

John Whitmore

author

2018-08-28 18:55 UTC #3 lore raw

PATCH 3/4 staging:rtl8192u: Refactor member variable enabled - Style

In reply to John Whitmore

The structure rt_dot11d_info contains a memeber variable 'enabled', which causes a checkpatch issue as it is declared as being of type bool. The type of the variable has been changed to a 'u8', to clear the issue.

Additionally to aid searching for the member variable in code it has been renamed from 'enabled' to 'dot11d_enabled'.

These are purely coding style changes which should not impact runtime code execution.

Signed-off-by John Whitmore <[email protected]>

---
 drivers/staging/rtl8192u/ieee80211/dot11d.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/dot11d.h | 4 ++--
 drivers/staging/rtl8192u/r8192U_core.c      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

drivers/staging/rtl8192u/ieee80211/dot11d.c +1 -1

index d3b13f0cfc09..f3aa60cb3227 100644
@@ -7,7 +7,7 @@ void dot11d_init(struct ieee80211_device *ieee)
 {
 	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(ieee);
 
-	pDot11dInfo->enabled = false;
+	pDot11dInfo->dot11d_enabled = false;
 
 	pDot11dInfo->state = DOT11D_STATE_NONE;
 	pDot11dInfo->country_ie_len = 0;

drivers/staging/rtl8192u/ieee80211/dot11d.h +2 -2

index be59f720ae20..df37678b806d 100644
@@ -28,12 +28,12 @@ struct rt_dot11d_info {
 	u8  max_tx_pwr_dbm_list[MAX_CHANNEL_NUMBER + 1];
 
 	enum dot11d_state state;
-	bool enabled; /* dot11MultiDomainCapabilityEnabled */
+	u8  dot11d_enabled; /* dot11MultiDomainCapabilityEnabled */
 };
 
 #define GET_DOT11D_INFO(ieee_dev) ((struct rt_dot11d_info *)((ieee_dev)->pDot11dInfo))
 
-#define IS_DOT11D_ENABLE(ieee_dev) (GET_DOT11D_INFO(ieee_dev)->enabled)
+#define IS_DOT11D_ENABLE(ieee_dev) (GET_DOT11D_INFO(ieee_dev)->dot11d_enabled)
 #define IS_COUNTRY_IE_VALID(ieee_dev) (GET_DOT11D_INFO(ieee_dev)->country_ie_len > 0)
 
 #define IS_EQUAL_CIE_SRC(ieee_dev, addr) ether_addr_equal(GET_DOT11D_INFO(ieee_dev)->country_ie_src_addr, addr)

drivers/staging/rtl8192u/r8192U_core.c +1 -1

index 6b22aaa40ff9..34ea47366b72 100644
@@ -211,7 +211,7 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
 		/* this flag enabled to follow 11d country IE setting,
 		 * otherwise, it shall follow global domain settings.
 		 */
-		GET_DOT11D_INFO(ieee)->enabled = 0;
+		GET_DOT11D_INFO(ieee)->dot11d_enabled = 0;
 		dot11d_reset(ieee);
 		ieee->bGlobalDomain = true;
 		break;

Signature · 1 line

2.18.0

Dan Carpenter

2018-08-29 11:20 UTC #4 lore raw

Re: [PATCH 3/4] staging:rtl8192u: Refactor member variable enabled - Style

Like you mentioned in the 0/4 email, the right thing to do here is just to delete all references to ->enabled.

regards,
dan carpenter

John Whitmore

2018-08-29 11:30 UTC #5 lore raw

Re: [PATCH 3/4] staging:rtl8192u: Refactor member variable enabled - Style

Quoted 3 lines · Dan Carpenter

Like you mentioned in the 0/4 email, the right thing to do here is just to delete all references to ->enabled.

Thank you for your response, I'll do that in a future series of patches.

jwhitmore

John Whitmore

author

2018-08-28 18:55 UTC #6 lore raw

PATCH 4/4 staging:rtl8192u: Rename member pDot11dInfo - Style

In reply to John Whitmore

Rename 'pDot11dInfo', this member variable of the structure ieee80211_device causes a checkpatch issue, CamelCase naming. The member has been renamed 'dot11d_info' to clear this issue.

This is a coding style change which should have no impact on runtime code execution.

Signed-off-by John Whitmore <[email protected]>

---
 drivers/staging/rtl8192u/ieee80211/dot11d.c   | 66 +++++++++----------
 drivers/staging/rtl8192u/ieee80211/dot11d.h   |  2 +-
 .../staging/rtl8192u/ieee80211/ieee80211.h    |  2 +-
 .../rtl8192u/ieee80211/ieee80211_softmac.c    |  8 +--
 4 files changed, 39 insertions(+), 39 deletions(-)

drivers/staging/rtl8192u/ieee80211/dot11d.c +33 ~~-33~~

index f3aa60cb3227..e152528c9409 100644
@@ -5,14 +5,14 @@
 
 void dot11d_init(struct ieee80211_device *ieee)
 {
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(ieee);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(ieee);
 
-	pDot11dInfo->dot11d_enabled = false;
+	dot11d_info->dot11d_enabled = false;
 
-	pDot11dInfo->state = DOT11D_STATE_NONE;
-	pDot11dInfo->country_ie_len = 0;
-	memset(pDot11dInfo->channel_map, 0, MAX_CHANNEL_NUMBER + 1);
-	memset(pDot11dInfo->max_tx_pwr_dbm_list, 0xFF, MAX_CHANNEL_NUMBER+1);
+	dot11d_info->state = DOT11D_STATE_NONE;
+	dot11d_info->country_ie_len = 0;
+	memset(dot11d_info->channel_map, 0, MAX_CHANNEL_NUMBER + 1);
+	memset(dot11d_info->max_tx_pwr_dbm_list, 0xFF, MAX_CHANNEL_NUMBER+1);
 	RESET_CIE_WATCHDOG(ieee);
 
 	netdev_info(ieee->dev, "dot11d_init()\n");

@@ -23,19 +23,19 @@ EXPORT_SYMBOL(dot11d_init);
 void dot11d_reset(struct ieee80211_device *ieee)
 {
 	u32 i;
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(ieee);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(ieee);
 	/* Clear old channel map */
-	memset(pDot11dInfo->channel_map, 0, MAX_CHANNEL_NUMBER+1);
-	memset(pDot11dInfo->max_tx_pwr_dbm_list, 0xFF, MAX_CHANNEL_NUMBER+1);
+	memset(dot11d_info->channel_map, 0, MAX_CHANNEL_NUMBER+1);
+	memset(dot11d_info->max_tx_pwr_dbm_list, 0xFF, MAX_CHANNEL_NUMBER+1);
 	/* Set new channel map */
 	for (i = 1; i <= 11; i++)
-		(pDot11dInfo->channel_map)[i] = 1;
+		(dot11d_info->channel_map)[i] = 1;
 
 	for (i = 12; i <= 14; i++)
-		(pDot11dInfo->channel_map)[i] = 2;
+		(dot11d_info->channel_map)[i] = 2;
 
-	pDot11dInfo->state = DOT11D_STATE_NONE;
-	pDot11dInfo->country_ie_len = 0;
+	dot11d_info->state = DOT11D_STATE_NONE;
+	dot11d_info->country_ie_len = 0;
 	RESET_CIE_WATCHDOG(ieee);
 }
 EXPORT_SYMBOL(dot11d_reset);

@@ -52,12 +52,12 @@ EXPORT_SYMBOL(dot11d_reset);
 void dot11d_update_country_ie(struct ieee80211_device *dev, u8 *pTaddr,
 			    u16 CoutryIeLen, u8 *pCoutryIe)
 {
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(dev);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(dev);
 	u8 i, j, NumTriples, MaxChnlNum;
 	struct chnl_txpower_triple *pTriple;
 
-	memset(pDot11dInfo->channel_map, 0, MAX_CHANNEL_NUMBER+1);
-	memset(pDot11dInfo->max_tx_pwr_dbm_list, 0xFF, MAX_CHANNEL_NUMBER+1);
+	memset(dot11d_info->channel_map, 0, MAX_CHANNEL_NUMBER+1);
+	memset(dot11d_info->max_tx_pwr_dbm_list, 0xFF, MAX_CHANNEL_NUMBER+1);
 	MaxChnlNum = 0;
 	NumTriples = (CoutryIeLen - 3) / 3; /* skip 3-byte country string. */
 	pTriple = (struct chnl_txpower_triple *)(pCoutryIe + 3);

@@ -78,8 +78,8 @@ void dot11d_update_country_ie(struct ieee80211_device *dev, u8 *pTaddr,
 		}
 
 		for (j = 0; j < pTriple->num_channels; j++) {
-			pDot11dInfo->channel_map[pTriple->first_channel + j] = 1;
-			pDot11dInfo->max_tx_pwr_dbm_list[pTriple->first_channel + j] = pTriple->max_tx_pwr_dbm;
+			dot11d_info->channel_map[pTriple->first_channel + j] = 1;
+			dot11d_info->max_tx_pwr_dbm_list[pTriple->first_channel + j] = pTriple->max_tx_pwr_dbm;
 			MaxChnlNum = pTriple->first_channel + j;
 		}

@@ -87,29 +87,29 @@ void dot11d_update_country_ie(struct ieee80211_device *dev, u8 *pTaddr,
 	}
 	netdev_info(dev->dev, "Channel List:");
 	for (i = 1; i <= MAX_CHANNEL_NUMBER; i++)
-		if (pDot11dInfo->channel_map[i] > 0)
+		if (dot11d_info->channel_map[i] > 0)
 			netdev_info(dev->dev, " %d", i);
 	netdev_info(dev->dev, "\n");
 
 	UPDATE_CIE_SRC(dev, pTaddr);
 
-	pDot11dInfo->country_ie_len = CoutryIeLen;
-	memcpy(pDot11dInfo->country_ie_buf, pCoutryIe, CoutryIeLen);
-	pDot11dInfo->state = DOT11D_STATE_LEARNED;
+	dot11d_info->country_ie_len = CoutryIeLen;
+	memcpy(dot11d_info->country_ie_buf, pCoutryIe, CoutryIeLen);
+	dot11d_info->state = DOT11D_STATE_LEARNED;
 }
 EXPORT_SYMBOL(dot11d_update_country_ie);
 
 u8 dot11d_get_max_tx_pwr_in_dbm(struct ieee80211_device *dev, u8 Channel)
 {
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(dev);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(dev);
 	u8 MaxTxPwrInDbm = 255;
 
 	if (Channel > MAX_CHANNEL_NUMBER) {
 		netdev_err(dev->dev, "dot11d_get_max_tx_pwr_in_dbm(): Invalid Channel\n");
 		return MaxTxPwrInDbm;
 	}
-	if (pDot11dInfo->channel_map[Channel])
-		MaxTxPwrInDbm = pDot11dInfo->max_tx_pwr_dbm_list[Channel];
+	if (dot11d_info->channel_map[Channel])
+		MaxTxPwrInDbm = dot11d_info->max_tx_pwr_dbm_list[Channel];
 
 	return MaxTxPwrInDbm;
 }

@@ -117,11 +117,11 @@ EXPORT_SYMBOL(dot11d_get_max_tx_pwr_in_dbm);
 
 void dot11d_scan_complete(struct ieee80211_device *dev)
 {
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(dev);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(dev);
 
-	switch (pDot11dInfo->state) {
+	switch (dot11d_info->state) {
 	case DOT11D_STATE_LEARNED:
-		pDot11dInfo->state = DOT11D_STATE_DONE;
+		dot11d_info->state = DOT11D_STATE_DONE;
 		break;
 
 	case DOT11D_STATE_DONE:

@@ -138,13 +138,13 @@ EXPORT_SYMBOL(dot11d_scan_complete);
 
 int is_legal_channel(struct ieee80211_device *dev, u8 channel)
 {
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(dev);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(dev);
 
 	if (channel > MAX_CHANNEL_NUMBER) {
 		netdev_err(dev->dev, "is_legal_channel(): Invalid Channel\n");
 		return 0;
 	}
-	if (pDot11dInfo->channel_map[channel] > 0)
+	if (dot11d_info->channel_map[channel] > 0)
 		return 1;
 	return 0;
 }

@@ -152,12 +152,12 @@ EXPORT_SYMBOL(is_legal_channel);
 
 int to_legal_channel(struct ieee80211_device *dev, u8 channel)
 {
-	struct rt_dot11d_info *pDot11dInfo = GET_DOT11D_INFO(dev);
+	struct rt_dot11d_info *dot11d_info = GET_DOT11D_INFO(dev);
 	u8 default_chn = 0;
 	u32 i = 0;
 
 	for (i = 1; i <= MAX_CHANNEL_NUMBER; i++) {
-		if (pDot11dInfo->channel_map[i] > 0) {
+		if (dot11d_info->channel_map[i] > 0) {
 			default_chn = i;
 			break;
 		}

@@ -168,7 +168,7 @@ int to_legal_channel(struct ieee80211_device *dev, u8 channel)
 		return default_chn;
 	}
 
-	if (pDot11dInfo->channel_map[channel] > 0)
+	if (dot11d_info->channel_map[channel] > 0)
 		return channel;
 
 	return default_chn;

drivers/staging/rtl8192u/ieee80211/dot11d.h +1 -1

index df37678b806d..414b74abfb74 100644
@@ -31,7 +31,7 @@ struct rt_dot11d_info {
 	u8  dot11d_enabled; /* dot11MultiDomainCapabilityEnabled */
 };
 
-#define GET_DOT11D_INFO(ieee_dev) ((struct rt_dot11d_info *)((ieee_dev)->pDot11dInfo))
+#define GET_DOT11D_INFO(ieee_dev) ((struct rt_dot11d_info *)((ieee_dev)->dot11d_info))
 
 #define IS_DOT11D_ENABLE(ieee_dev) (GET_DOT11D_INFO(ieee_dev)->dot11d_enabled)
 #define IS_COUNTRY_IE_VALID(ieee_dev) (GET_DOT11D_INFO(ieee_dev)->country_ie_len > 0)

drivers/staging/rtl8192u/ieee80211/ieee80211.h +1 -1

index e2e503d24c4c..b4e7ae60ac1c 100644
@@ -1772,7 +1772,7 @@ struct ieee80211_device {
 
 	/* map of allowed channels. 0 is dummy */
 	// FIXME: remember to default to a basic channel plan depending of the PHY type
-	void *pDot11dInfo;
+	void *dot11d_info;
 	bool bGlobalDomain;
 	int rate;       /* current rate */
 	int basic_rate;

drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +4 -4

index a20c79401dab..8635faf84316 100644
@@ -2542,8 +2542,8 @@ void ieee80211_softmac_init(struct ieee80211_device *ieee)
 	for (i = 0; i < 5; i++)
 		ieee->seq_ctrl[i] = 0;
 
-	ieee->pDot11dInfo = kzalloc(sizeof(struct rt_dot11d_info), GFP_KERNEL);
-	if (!ieee->pDot11dInfo)
+	ieee->dot11d_info = kzalloc(sizeof(struct rt_dot11d_info), GFP_KERNEL);
+	if (!ieee->dot11d_info)
 		IEEE80211_DEBUG(IEEE80211_DL_ERR, "can't alloc memory for DOT11D\n");
 	//added for  AP roaming
 	ieee->LinkDetectInfo.SlotNum = 2;

@@ -2603,8 +2603,8 @@ void ieee80211_softmac_init(struct ieee80211_device *ieee)
 void ieee80211_softmac_free(struct ieee80211_device *ieee)
 {
 	mutex_lock(&ieee->wx_mutex);
-	kfree(ieee->pDot11dInfo);
-	ieee->pDot11dInfo = NULL;
+	kfree(ieee->dot11d_info);
+	ieee->dot11d_info = NULL;
 	del_timer_sync(&ieee->associate_timer);
 
 	cancel_delayed_work(&ieee->associate_retry_wq);

Signature · 1 line

2.18.0