* [PATCH 0/5]
@ 2015-07-24 4:38 Joshua Clayton
2015-07-24 4:38 ` [PATCH 1/5] staging: rtl8712: fix buggy size calculation Joshua Clayton
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Joshua Clayton @ 2015-07-24 4:38 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman
Cc: Nitin Kuppelur, Sudip Mukherjee, Joshua Clayton,
Tapasweni Pathak, Vaishali Thakkar, devel, linux-kernel
The main goal of this series is to get rid of a needless and ugly typedef
in the rtl8712 wlan driver.
In the course of fixing that, I found a bug thati will can might (at least in theory)
lead to a overrun during a memcpy, as well as a duplicate struct.
Finally after cleaning up the typedef, I could not bring myself to leave
a variable called SupportedRates in the kernel with my name on it.
I have tested this on amd64. cwthe module loads and doesn't explode
Joshua Clayton (5):
staging: rtl8712: fix buggy size calculation
staging: rtl8712: simplify size calculation
staging: rtl8712: remove duplicate struct
staging: rtl8712: remove typedefs
staging: rtl8712: style fix:
drivers/staging/rtl8712/ieee80211.c | 22 ++++++-------
drivers/staging/rtl8712/rtl871x_cmd.c | 28 +++++-----------
drivers/staging/rtl8712/rtl871x_cmd.h | 4 +--
drivers/staging/rtl8712/rtl871x_event.h | 2 +-
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 33 +++++++++----------
drivers/staging/rtl8712/rtl871x_mlme.c | 47 ++++++++++-----------------
drivers/staging/rtl8712/rtl871x_mlme.h | 2 +-
drivers/staging/rtl8712/rtl871x_mp_ioctl.c | 6 ++--
drivers/staging/rtl8712/wlan_bssdef.h | 32 +++---------------
9 files changed, 63 insertions(+), 113 deletions(-)
--
2.4.6
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/5] staging: rtl8712: fix buggy size calculation
2015-07-24 4:38 [PATCH 0/5] Joshua Clayton
@ 2015-07-24 4:38 ` Joshua Clayton
2015-07-24 10:52 ` Dan Carpenter
2015-07-24 4:38 ` [PATCH 2/5] staging: rtl8712: simplify " Joshua Clayton
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Joshua Clayton @ 2015-07-24 4:38 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman
Cc: Nitin Kuppelur, Sudip Mukherjee, Joshua Clayton,
Tapasweni Pathak, Vaishali Thakkar, devel, linux-kernel
r8712_get_ndis_wlan_bssid_ex_sz has a "6 * sizeof(unsigned long)"
where the underlying struct has a 6 * unsigned char.
Simplify the calculation by just subtracting the variable part from
the size of the struct.
This also gets rid of a use of typedef NDIS_802_11_RATES_EX
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
drivers/staging/rtl8712/rtl871x_mlme.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c b/drivers/staging/rtl8712/rtl871x_mlme.c
index c044b0e..6b3451f 100644
--- a/drivers/staging/rtl8712/rtl871x_mlme.c
+++ b/drivers/staging/rtl8712/rtl871x_mlme.c
@@ -210,17 +210,7 @@ void r8712_generate_random_ibss(u8 *pibss)
uint r8712_get_ndis_wlan_bssid_ex_sz(struct ndis_wlan_bssid_ex *bss)
{
- uint t_len;
-
- t_len = sizeof(u32) + 6 * sizeof(unsigned long) + 2 +
- sizeof(struct ndis_802_11_ssid) + sizeof(u32) +
- sizeof(s32) +
- sizeof(enum NDIS_802_11_NETWORK_TYPE) +
- sizeof(struct NDIS_802_11_CONFIGURATION) +
- sizeof(enum NDIS_802_11_NETWORK_INFRASTRUCTURE) +
- sizeof(NDIS_802_11_RATES_EX) +
- sizeof(u32) + bss->IELength;
- return t_len;
+ return sizeof(*bss) + bss->IELength - MAX_IE_SZ;
}
u8 *r8712_get_capability_from_ie(u8 *ie)
--
2.4.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/5] staging: rtl8712: simplify size calculation
2015-07-24 4:38 [PATCH 0/5] Joshua Clayton
2015-07-24 4:38 ` [PATCH 1/5] staging: rtl8712: fix buggy size calculation Joshua Clayton
@ 2015-07-24 4:38 ` Joshua Clayton
2015-07-24 4:52 ` [PATCH 4/5] staging: rtl8712: remove typedefs Joshua Clayton
2015-07-24 4:53 ` [PATCH 5/5] staging: rtl8712: style fix: Joshua Clayton
3 siblings, 0 replies; 8+ messages in thread
From: Joshua Clayton @ 2015-07-24 4:38 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman
Cc: Nitin Kuppelur, Sudip Mukherjee, Joshua Clayton,
Tapasweni Pathak, Vaishali Thakkar, devel, linux-kernel
replace item-by-item size calculation of a struct
with the size of the struct.
This gets rid of a use of typedef NDIS_802_11_RATES_EX
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
drivers/staging/rtl8712/rtl871x_cmd.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
index e35854d..f07050d 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -471,7 +471,6 @@ u8 r8712_createbss_cmd(struct _adapter *padapter)
u8 r8712_joinbss_cmd(struct _adapter *padapter, struct wlan_network *pnetwork)
{
- uint t_len = 0;
struct ndis_wlan_bssid_ex *psecnetwork;
struct cmd_obj *pcmd;
struct cmd_priv *pcmdpriv = &padapter->cmdpriv;
@@ -486,14 +485,6 @@ u8 r8712_joinbss_cmd(struct _adapter *padapter, struct wlan_network *pnetwork)
pcmd = kmalloc(sizeof(*pcmd), GFP_ATOMIC);
if (pcmd == NULL)
return _FAIL;
- t_len = sizeof(u32) + 6 * sizeof(unsigned char) + 2 +
- sizeof(struct ndis_802_11_ssid) + sizeof(u32) +
- sizeof(s32) +
- sizeof(enum NDIS_802_11_NETWORK_TYPE) +
- sizeof(struct NDIS_802_11_CONFIGURATION) +
- sizeof(enum NDIS_802_11_NETWORK_INFRASTRUCTURE) +
- sizeof(NDIS_802_11_RATES_EX) +
- sizeof(u32) + MAX_IE_SZ;
/* for hidden ap to set fw_state here */
if (check_fwstate(pmlmepriv, WIFI_STATION_STATE|WIFI_ADHOC_STATE) !=
@@ -516,7 +507,7 @@ u8 r8712_joinbss_cmd(struct _adapter *padapter, struct wlan_network *pnetwork)
kfree(pcmd);
return _FAIL;
}
- memcpy(psecnetwork, &pnetwork->network, t_len);
+ memcpy(psecnetwork, &pnetwork->network, sizeof(*psecnetwork));
psecuritypriv->authenticator_ie[0] = (unsigned char)
psecnetwork->IELength;
if ((psecnetwork->IELength-12) < (256 - 1))
--
2.4.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/5] staging: rtl8712: remove typedefs
2015-07-24 4:38 [PATCH 0/5] Joshua Clayton
2015-07-24 4:38 ` [PATCH 1/5] staging: rtl8712: fix buggy size calculation Joshua Clayton
2015-07-24 4:38 ` [PATCH 2/5] staging: rtl8712: simplify " Joshua Clayton
@ 2015-07-24 4:52 ` Joshua Clayton
2015-07-24 4:53 ` [PATCH 5/5] staging: rtl8712: style fix: Joshua Clayton
3 siblings, 0 replies; 8+ messages in thread
From: Joshua Clayton @ 2015-07-24 4:52 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman
Cc: Nitin Kuppelur, Sudip Mukherjee, Joshua Clayton,
Tapasweni Pathak, Vaishali Thakkar, devel, linux-kernel
Coding style fix.
Get rid of typedefs NDIS_802_11_RATES and NDIS_802_11_RATES_EX
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
drivers/staging/rtl8712/wlan_bssdef.h | 7 +------
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 4f5f69c..087fbf1 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -635,7 +635,7 @@ static int r8711_wx_get_name(struct net_device *dev,
u8 ht_cap = false;
struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
struct wlan_bssid_ex *pcur_bss = &pmlmepriv->cur_network.network;
- NDIS_802_11_RATES_EX *prates = NULL;
+ u8 *prates;
if (check_fwstate(pmlmepriv, _FW_LINKED|WIFI_ADHOC_MASTER_STATE) ==
true) {
@@ -644,7 +644,7 @@ static int r8711_wx_get_name(struct net_device *dev,
&ht_ielen, pcur_bss->IELength - 12);
if (p && ht_ielen > 0)
ht_cap = true;
- prates = &pcur_bss->SupportedRates;
+ prates = pcur_bss->SupportedRates;
if (r8712_is_cckratesonly_included((u8 *)prates) == true) {
if (ht_cap == true)
snprintf(wrqu->name, IFNAMSIZ,
diff --git a/drivers/staging/rtl8712/wlan_bssdef.h b/drivers/staging/rtl8712/wlan_bssdef.h
index 7d769e8..7a410b5 100644
--- a/drivers/staging/rtl8712/wlan_bssdef.h
+++ b/drivers/staging/rtl8712/wlan_bssdef.h
@@ -32,11 +32,6 @@
#define NDIS_802_11_LENGTH_RATES 8
#define NDIS_802_11_LENGTH_RATES_EX 16
-/* Set of 8 data rates*/
-typedef unsigned char NDIS_802_11_RATES[NDIS_802_11_LENGTH_RATES];
-/* Set of 16 data rates */
-typedef unsigned char NDIS_802_11_RATES_EX[NDIS_802_11_LENGTH_RATES_EX];
-
struct ndis_802_11_ssid {
u32 SsidLength;
u8 Ssid[32];
@@ -104,7 +99,7 @@ struct wlan_bssid_ex {
enum NDIS_802_11_NETWORK_TYPE NetworkTypeInUse;
struct NDIS_802_11_CONFIGURATION Configuration;
enum NDIS_802_11_NETWORK_INFRASTRUCTURE InfrastructureMode;
- NDIS_802_11_RATES_EX SupportedRates;
+ u8 SupportedRates[NDIS_802_11_LENGTH_RATES_EX];
u32 IELength;
/*(timestamp, beacon interval, and capability information) */
u8 IEs[MAX_IE_SZ];
--
2.4.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/5] staging: rtl8712: style fix:
2015-07-24 4:38 [PATCH 0/5] Joshua Clayton
` (2 preceding siblings ...)
2015-07-24 4:52 ` [PATCH 4/5] staging: rtl8712: remove typedefs Joshua Clayton
@ 2015-07-24 4:53 ` Joshua Clayton
2015-07-24 10:52 ` Dan Carpenter
3 siblings, 1 reply; 8+ messages in thread
From: Joshua Clayton @ 2015-07-24 4:53 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman
Cc: Nitin Kuppelur, Sudip Mukherjee, Joshua Clayton,
Tapasweni Pathak, Vaishali Thakkar, devel, linux-kernel
change instances SupportedRates to compliant and sane "rates"
This change in no way harms readability, and brings several lines
under the 80 character limit.
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
drivers/staging/rtl8712/ieee80211.c | 22 +++++++++++-----------
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 21 +++++++++------------
drivers/staging/rtl8712/wlan_bssdef.h | 4 ++--
3 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/rtl8712/ieee80211.c b/drivers/staging/rtl8712/ieee80211.c
index 5786808..ca9dcaf 100644
--- a/drivers/staging/rtl8712/ieee80211.c
+++ b/drivers/staging/rtl8712/ieee80211.c
@@ -134,22 +134,22 @@ u8 *r8712_get_ie(u8 *pbuf, sint index, sint *len, sint limit)
return NULL;
}
-static void set_supported_rate(u8 *SupportedRates, uint mode)
+static void set_supported_rate(u8 *rates, uint mode)
{
- memset(SupportedRates, 0, NDIS_802_11_LENGTH_RATES_EX);
+ memset(rates, 0, NDIS_802_11_LENGTH_RATES_EX);
switch (mode) {
case WIRELESS_11B:
- memcpy(SupportedRates, WIFI_CCKRATES,
+ memcpy(rates, WIFI_CCKRATES,
IEEE80211_CCK_RATE_LEN);
break;
case WIRELESS_11G:
case WIRELESS_11A:
- memcpy(SupportedRates, WIFI_OFDMRATES,
+ memcpy(rates, WIFI_OFDMRATES,
IEEE80211_NUM_OFDM_RATESLEN);
break;
case WIRELESS_11BG:
- memcpy(SupportedRates, WIFI_CCKRATES, IEEE80211_CCK_RATE_LEN);
- memcpy(SupportedRates + IEEE80211_CCK_RATE_LEN, WIFI_OFDMRATES,
+ memcpy(rates, WIFI_CCKRATES, IEEE80211_CCK_RATE_LEN);
+ memcpy(rates + IEEE80211_CCK_RATE_LEN, WIFI_OFDMRATES,
IEEE80211_NUM_OFDM_RATESLEN);
break;
}
@@ -195,17 +195,17 @@ int r8712_generate_ie(struct registry_priv *pregistrypriv)
ie = r8712_set_ie(ie, _SSID_IE_, pdev_network->Ssid.SsidLength,
pdev_network->Ssid.Ssid, &sz);
/*supported rates*/
- set_supported_rate(pdev_network->SupportedRates,
+ set_supported_rate(pdev_network->rates,
pregistrypriv->wireless_mode);
- rateLen = r8712_get_rateset_len(pdev_network->SupportedRates);
+ rateLen = r8712_get_rateset_len(pdev_network->rates);
if (rateLen > 8) {
ie = r8712_set_ie(ie, _SUPPORTEDRATES_IE_, 8,
- pdev_network->SupportedRates, &sz);
+ pdev_network->rates, &sz);
ie = r8712_set_ie(ie, _EXT_SUPPORTEDRATES_IE_, (rateLen - 8),
- (pdev_network->SupportedRates + 8), &sz);
+ (pdev_network->rates + 8), &sz);
} else
ie = r8712_set_ie(ie, _SUPPORTEDRATES_IE_,
- rateLen, pdev_network->SupportedRates, &sz);
+ rateLen, pdev_network->rates, &sz);
/*DS parameter set*/
ie = r8712_set_ie(ie, _DSSET_IE_, 1,
(u8 *)&(pdev_network->Configuration.DSConfig), &sz);
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 087fbf1..219239c 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -203,14 +203,12 @@ static inline char *translate_scan(struct _adapter *padapter,
}
/* Add the protocol name */
iwe.cmd = SIOCGIWNAME;
- if ((r8712_is_cckratesonly_included((u8 *)&pnetwork->network.
- SupportedRates)) == true) {
+ if (r8712_is_cckratesonly_included(pnetwork->network.rates)) {
if (ht_cap == true)
snprintf(iwe.u.name, IFNAMSIZ, "IEEE 802.11bn");
else
snprintf(iwe.u.name, IFNAMSIZ, "IEEE 802.11b");
- } else if ((r8712_is_cckrates_included((u8 *)&pnetwork->network.
- SupportedRates)) == true) {
+ } else if (r8712_is_cckrates_included(pnetwork->network.rates)) {
if (ht_cap == true)
snprintf(iwe.u.name, IFNAMSIZ, "IEEE 802.11bgn");
else
@@ -270,9 +268,9 @@ static inline char *translate_scan(struct _adapter *padapter,
iwe.u.bitrate.disabled = 0;
iwe.u.bitrate.value = 0;
i = 0;
- while (pnetwork->network.SupportedRates[i] != 0) {
+ while (pnetwork->network.rates[i] != 0) {
/* Bit rate given in 500 kb/s units */
- iwe.u.bitrate.value = (pnetwork->network.SupportedRates[i++] &
+ iwe.u.bitrate.value = (pnetwork->network.rates[i++] &
0x7F) * 500000;
current_val = iwe_stream_add_value(info, start, current_val,
stop, &iwe, IW_EV_PARAM_LEN);
@@ -644,15 +642,15 @@ static int r8711_wx_get_name(struct net_device *dev,
&ht_ielen, pcur_bss->IELength - 12);
if (p && ht_ielen > 0)
ht_cap = true;
- prates = pcur_bss->SupportedRates;
- if (r8712_is_cckratesonly_included((u8 *)prates) == true) {
+ prates = pcur_bss->rates;
+ if (r8712_is_cckratesonly_included(prates) == true) {
if (ht_cap == true)
snprintf(wrqu->name, IFNAMSIZ,
"IEEE 802.11bn");
else
snprintf(wrqu->name, IFNAMSIZ,
"IEEE 802.11b");
- } else if ((r8712_is_cckrates_included((u8 *)prates)) == true) {
+ } else if ((r8712_is_cckrates_included(prates)) == true) {
if (ht_cap == true)
snprintf(wrqu->name, IFNAMSIZ,
"IEEE 802.11bgn");
@@ -1444,9 +1442,8 @@ static int r8711_wx_get_rate(struct net_device *dev,
(IEEE80211_HT_CAP_SGI_20 |
IEEE80211_HT_CAP_SGI_40)) ? 1 : 0;
}
- while ((pcur_bss->SupportedRates[i] != 0) &&
- (pcur_bss->SupportedRates[i] != 0xFF)) {
- rate = pcur_bss->SupportedRates[i] & 0x7F;
+ while (pcur_bss->rates[i] && (pcur_bss->rates[i] != 0xFF)) {
+ rate = pcur_bss->rates[i] & 0x7F;
if (rate > max_rate)
max_rate = rate;
wrqu->bitrate.fixed = 0; /* no auto select */
diff --git a/drivers/staging/rtl8712/wlan_bssdef.h b/drivers/staging/rtl8712/wlan_bssdef.h
index 7a410b5..8e2d798 100644
--- a/drivers/staging/rtl8712/wlan_bssdef.h
+++ b/drivers/staging/rtl8712/wlan_bssdef.h
@@ -83,7 +83,7 @@ struct NDIS_802_11_FIXED_IEs {
* 6 * sizeof (unsigned char) + 2 + sizeof (ndis_802_11_ssid) + sizeof (u32)
* + sizeof (s32) + sizeof (NDIS_802_11_NETWORK_TYPE)
* + sizeof (struct NDIS_802_11_CONFIGURATION)
- * + sizeof (NDIS_802_11_RATES_EX) + IELength
+ * + sizeof (rates) + IELength
* Except the IELength, all other fields are fixed length. Therefore, we can
* define a macro to present the partial sum.
@@ -99,7 +99,7 @@ struct wlan_bssid_ex {
enum NDIS_802_11_NETWORK_TYPE NetworkTypeInUse;
struct NDIS_802_11_CONFIGURATION Configuration;
enum NDIS_802_11_NETWORK_INFRASTRUCTURE InfrastructureMode;
- u8 SupportedRates[NDIS_802_11_LENGTH_RATES_EX];
+ u8 rates[NDIS_802_11_LENGTH_RATES_EX];
u32 IELength;
/*(timestamp, beacon interval, and capability information) */
u8 IEs[MAX_IE_SZ];
--
2.4.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 5/5] staging: rtl8712: style fix:
2015-07-24 4:53 ` [PATCH 5/5] staging: rtl8712: style fix: Joshua Clayton
@ 2015-07-24 10:52 ` Dan Carpenter
2015-07-24 13:06 ` Joshua Clayton
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2015-07-24 10:52 UTC (permalink / raw)
To: Joshua Clayton
Cc: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman, devel,
Tapasweni Pathak, Vaishali Thakkar, Nitin Kuppelur, linux-kernel,
Sudip Mukherjee
Write a better subject line.
On Thu, Jul 23, 2015 at 09:53:18PM -0700, Joshua Clayton wrote:
> change instances SupportedRates to compliant and sane "rates"
> This change in no way harms readability, and brings several lines
> under the 80 character limit.
>
Yeah, but it does a some other stuff as well like removing casts.
> - while ((pcur_bss->SupportedRates[i] != 0) &&
> - (pcur_bss->SupportedRates[i] != 0xFF)) {
> - rate = pcur_bss->SupportedRates[i] & 0x7F;
> + while (pcur_bss->rates[i] && (pcur_bss->rates[i] != 0xFF)) {
> + rate = pcur_bss->rates[i] & 0x7F;
> if (rate > max_rate)
> max_rate = rate;
> wrqu->bitrate.fixed = 0; /* no auto select */
I actually like the != 0 here because we're talking about the number
zero. It should look like this:
while (pcur_bss->rates[i] != 0 &&
pcur_bss->rates[i] != 0xFF) {
But removing the parens is something for a different patch. I use a
script to help review these so when you mix different changes together
it means there is more manual review work for me.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] staging: rtl8712: fix buggy size calculation
2015-07-24 4:38 ` [PATCH 1/5] staging: rtl8712: fix buggy size calculation Joshua Clayton
@ 2015-07-24 10:52 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-07-24 10:52 UTC (permalink / raw)
To: Joshua Clayton
Cc: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman, devel,
Tapasweni Pathak, Vaishali Thakkar, Nitin Kuppelur, linux-kernel,
Sudip Mukherjee
On Thu, Jul 23, 2015 at 09:38:33PM -0700, Joshua Clayton wrote:
> r8712_get_ndis_wlan_bssid_ex_sz has a "6 * sizeof(unsigned long)"
> where the underlying struct has a 6 * unsigned char.
> Simplify the calculation by just subtracting the variable part from
> the size of the struct.
>
> This also gets rid of a use of typedef NDIS_802_11_RATES_EX
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Looks like you are right.
Reviewed-by: Dan Carpenter
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 5/5] staging: rtl8712: style fix:
2015-07-24 10:52 ` Dan Carpenter
@ 2015-07-24 13:06 ` Joshua Clayton
0 siblings, 0 replies; 8+ messages in thread
From: Joshua Clayton @ 2015-07-24 13:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman, devel,
Tapasweni Pathak, Vaishali Thakkar, Nitin Kuppelur, linux-kernel,
Sudip Mukherjee
Dan,
On Friday, July 24, 2015 01:52:27 PM Dan Carpenter wrote:
> Write a better subject line.
>
> On Thu, Jul 23, 2015 at 09:53:18PM -0700, Joshua Clayton wrote:
> > change instances SupportedRates to compliant and sane "rates"
> > This change in no way harms readability, and brings several lines
> > under the 80 character limit.
>
> Yeah, but it does a some other stuff as well like removing casts.
I apologize. I thought it would make sense to double up (while I'm in there
I'll just...).
>
> > - while ((pcur_bss->SupportedRates[i] != 0) &&
> > - (pcur_bss->SupportedRates[i] != 0xFF)) {
> > - rate = pcur_bss->SupportedRates[i] & 0x7F;
> > + while (pcur_bss->rates[i] && (pcur_bss->rates[i] != 0xFF)) {
> > + rate = pcur_bss->rates[i] & 0x7F;
> >
> > if (rate > max_rate)
> >
> > max_rate = rate;
> >
> > wrqu->bitrate.fixed = 0; /* no auto select */
>
> I actually like the != 0 here because we're talking about the number
> zero. It should look like this:
>
> while (pcur_bss->rates[i] != 0 &&
> pcur_bss->rates[i] != 0xFF) {
OK.
>
> But removing the parens is something for a different patch. I use a
> script to help review these so when you mix different changes together
> it means there is more manual review work for me.
Sorry about that.
I'll split or drop it in the next version
>
> regards,
> dan carpenter
Joshua
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-07-24 13:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-24 4:38 [PATCH 0/5] Joshua Clayton
2015-07-24 4:38 ` [PATCH 1/5] staging: rtl8712: fix buggy size calculation Joshua Clayton
2015-07-24 10:52 ` Dan Carpenter
2015-07-24 4:38 ` [PATCH 2/5] staging: rtl8712: simplify " Joshua Clayton
2015-07-24 4:52 ` [PATCH 4/5] staging: rtl8712: remove typedefs Joshua Clayton
2015-07-24 4:53 ` [PATCH 5/5] staging: rtl8712: style fix: Joshua Clayton
2015-07-24 10:52 ` Dan Carpenter
2015-07-24 13:06 ` Joshua Clayton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).