staging: rtl8723bs: Remove redundant else branches.

PATCH In review Sevinj Aghayeva 2 participants 5 messages Mar 29, 2022 1 message not in the archive Staging

lore

Share & tools

lore

Sevinj Aghayeva

author

2022-03-29 14:09 UTC #0 lore raw

PATCH 1/1 staging: rtl8723bs: Remove redundant else branches.

This patch fixes the following checkpatch.pl warning:

WARNING: else is not generally useful after a break or return

Signed-off-by Sevinj Aghayeva <[email protected]>

---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 32 ++++++++-----------
 1 file changed, 13 insertions(+), 19 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +13 ~~-19~~

index b449be537376..27de086903e2 100644
@@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
 
 int rtw_check_network_type(unsigned char *rate, int ratelen, int channel)
 {
-	if (channel > 14) {
+	if (channel > 14)
 		return WIRELESS_INVALID;
-	} else { /*  could be pure B, pure G, or B/G */
-		if (rtw_is_cckratesonly_included(rate))
-			return WIRELESS_11B;
-		else if (rtw_is_cckrates_included(rate))
-			return	WIRELESS_11BG;
-		else
-			return WIRELESS_11G;
-	}
+	/* could be pure B, pure G, or B/G */
+	if (rtw_is_cckratesonly_included(rate))
+		return WIRELESS_11B;
+	if (rtw_is_cckrates_included(rate))
+		return WIRELESS_11BG;
+	return WIRELESS_11G;
 }
 
 u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned char *source,

@@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed int *len, signed int limit)
 		if (*p == index) {
 			*len = *(p + 1);
 			return p;
-		} else {
-			tmp = *(p + 1);
-			p += (tmp + 2);
-			i += (tmp + 2);
 		}
+		tmp = *(p + 1);
+		p += (tmp + 2);
+		i += (tmp + 2);
 		if (i >= limit)
 			break;
 	}

@@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8 *oui, u8 oui_len, u8 *ie, u
 				*ielen = in_ie[cnt+1]+2;
 
 			break;
-		} else {
-			cnt += in_ie[cnt+1]+2; /* goto next */
 		}
+		cnt += in_ie[cnt+1]+2; /* goto next */
 	}
 
 	return target_ie;

@@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen)
 			cnt += in_ie[cnt+1]+2;
 
 			break;
-		} else {
-			cnt += in_ie[cnt+1]+2; /* goto next */
 		}
+		cnt += in_ie[cnt+1]+2; /* goto next */
 	}
 
 	return wpsie_ptr;

@@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
 				*len_attr = attr_len;
 
 			break;
-		} else {
-			attr_ptr += attr_len; /* goto next */
 		}
+		attr_ptr += attr_len; /* goto next */
 	}
 
 	return target_attr_ptr;

Signature · 1 line

2.25.1

Alison Schofield

2022-03-29 19:58 UTC #1 lore raw

Hi Sevinj,

You can do this to make sure your commit msg 'looks' similar to others in the file:

$git log --pretty=oneline --abbrev-commit

d6ad258ae15d staging: rtl8723bs: Remove redundant else branches. 24e65aac9457 staging: rtl8723bs: remove rf type branching (fourth patch) 167fc30e8e51 staging: rtl8723bs: remove unused macros d7361874468f staging: rtl8723bs: fix camel case in struct wlan_bcn_info 6994aa430368 staging: rtl8723bs: fix camel case in struct ndis_802_11_ssid

That tells me the 'Remove' should not start with an uppercase and there should be no period at the end of line. Note that I look at this per patch, because different drivers and subsystems have different conventions. The point is to try to discern the prevailing style, and follow it.

Please check the get_maintainers instruction in the first patch tutorial. I only got this:

$ git show HEAD | perl scripts/get_maintainer.pl --separator , --nokeywords --nogit --nogit-fallback --norolestats
Greg Kroah-Hartman <[email protected]>,[email protected],[email protected]

On Tue, Mar 29, 2022 at 07:09:04AM -0700, Sevinj Aghayeva wrote:

Please state the 'why' of this patch. ie something like:
Adhere to Linux kernel coding style.

Quoted 1 line

This patch fixes the following checkpatch.pl warning:

The phrases 'This patch' is frowned upon as unecessarily wordy. Perhaps simply:
Reported by checkpatch:

You have another patch in flight with similar things to update. Please post a v2 of that one that addresses the feedback given here.

I thought both patches had good scope in that they addressed multiple instances of the same issue in a single file.

Thanks!
Alison

Quoted 88 lines

WARNING: else is not generally useful after a break or return

Signed-off-by Sevinj Aghayeva <[email protected]>

---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 32 ++++++++-----------
 1 file changed, 13 insertions(+), 19 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +13 ~~-19~~

index b449be537376..27de086903e2 100644
@@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
 
 int rtw_check_network_type(unsigned char *rate, int ratelen, int channel)
 {
-	if (channel > 14) {
+	if (channel > 14)
 		return WIRELESS_INVALID;
-	} else { /*  could be pure B, pure G, or B/G */
-		if (rtw_is_cckratesonly_included(rate))
-			return WIRELESS_11B;
-		else if (rtw_is_cckrates_included(rate))
-			return	WIRELESS_11BG;
-		else
-			return WIRELESS_11G;
-	}
+	/* could be pure B, pure G, or B/G */
+	if (rtw_is_cckratesonly_included(rate))
+		return WIRELESS_11B;
+	if (rtw_is_cckrates_included(rate))
+		return WIRELESS_11BG;
+	return WIRELESS_11G;
 }
 
 u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned char *source,

@@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed int *len, signed int limit)
 		if (*p == index) {
 			*len = *(p + 1);
 			return p;
-		} else {
-			tmp = *(p + 1);
-			p += (tmp + 2);
-			i += (tmp + 2);
 		}
+		tmp = *(p + 1);
+		p += (tmp + 2);
+		i += (tmp + 2);
 		if (i >= limit)
 			break;
 	}

@@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8 *oui, u8 oui_len, u8 *ie, u
 				*ielen = in_ie[cnt+1]+2;
 
 			break;
-		} else {
-			cnt += in_ie[cnt+1]+2; /* goto next */
 		}
+		cnt += in_ie[cnt+1]+2; /* goto next */
 	}
 
 	return target_ie;

@@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen)
 			cnt += in_ie[cnt+1]+2;
 
 			break;
-		} else {
-			cnt += in_ie[cnt+1]+2; /* goto next */
 		}
+		cnt += in_ie[cnt+1]+2; /* goto next */
 	}
 
 	return wpsie_ptr;

@@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
 				*len_attr = attr_len;
 
 			break;
-		} else {
-			attr_ptr += attr_len; /* goto next */
 		}
+		attr_ptr += attr_len; /* goto next */
 	}
 
 	return target_attr_ptr;

Signature · 1 line

2.25.1

A message that is not in the archive (sent off-list or not yet received): CAMWRUK4ti6QxA4JWbG_XwZHwQwWUKYu+HvVHvEBX-k82oaEP6g@mail.gmail.com

Alison Schofield

2022-03-29 21:22 UTC #3 lore raw

Quoted 53 lines · Sevinj Aghayeva

Hi Alison,
Thanks for the feedback! Please see inline.
Quoted 13 lines · Alison Schofield
Hi Sevinj,
You can do this to make sure your commit msg 'looks' similar to others in the file:
$git log --pretty=oneline --abbrev-commit
d6ad258ae15d staging: rtl8723bs: Remove redundant else branches. 24e65aac9457 staging: rtl8723bs: remove rf type branching (fourth patch) 167fc30e8e51 staging: rtl8723bs: remove unused macros d7361874468f staging: rtl8723bs: fix camel case in struct wlan_bcn_info 6994aa430368 staging: rtl8723bs: fix camel case in struct ndis_802_11_ssid
Quoted 6 lines
That tells me the 'Remove' should not start with an uppercase and there should be no period at the end of line. Note that I look at this per patch, because different drivers and subsystems have different conventions. The point is to try to discern the prevailing style, and follow it.
Got it. The few examples that I saw in other places were using grammatically formatted sentences, but I'll take the subsystem conventions into account.
Quoted 7 lines
Please check the get_maintainers instruction in the first patch tutorial. I only got this:
$ git show HEAD | perl scripts/get_maintainer.pl --separator ,
--nokeywords --nogit --nogit-fallback --norolestats
Greg Kroah-Hartman <[email protected]>,
[email protected],[email protected]
My bad! I ran get_maintainer.pl without those options and got those a lot more email addresses.
Quoted 8 lines · Sevinj Aghayeva
Please state the 'why' of this patch. ie something like:
Adhere to Linux kernel coding style.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
This patch fixes the following checkpatch.pl warning:
The phrases 'This patch' is frowned upon as unecessarily wordy. Perhaps simply:
Reported by checkpatch:
Okay. I think we should update the https://kernelnewbies.org/PatchPhilosophy
because I took "This patch..." from there.

Yeah, there's a lot of 'This patch...' and 'Fixes...' and some other stuff that has fallen out of favor.
I just made a small edit getting rid of a 'Fixes' subject line.

How about if you update this example:
Subject: [OPW kernel] [PATCH] Staging: rtl8192e: Fix printk() warning style with your patch right here..once it is done!

Thanks!
Alison

Quoted 9 lines

Quoted 7 lines
You have another patch in flight with similar things to update. Please post a v2 of that one that addresses the feedback given here.
That one has already landed. Is it okay if I send v2 just for this patch?

Did you see it applied already?

Quoted 115 lines

Thanks!

Quoted 103 lines

I thought both patches had good scope in that they addressed multiple instances of the same issue in a single file.

Thanks!
Alison

Review

Quoted 9 lines

WARNING: else is not generally useful after a break or return
Signed-off-by Sevinj Aghayeva <[email protected]>
---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 32 ++++++++-----------
 1 file changed, 13 insertions(+), 19 deletions(-)
(quoted diff) +0 -0

b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c

drivers/staging/rtl8723bs/core/rtw_ieee80211.c bool rtw_is_cckratesonly_included(u8 *rate) · line 95

Quoted 6 lines

index b449be537376..27de086903e2 100644
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +0 -0
@@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
 
 int rtw_check_network_type(unsigned char *rate, int ratelen, int

channel)

Quoted 11 lines

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -8

 {
-     if (channel > 14) {
+     if (channel > 14)
              return WIRELESS_INVALID;
-     } else { /*  could be pure B, pure G, or B/G */
-             if (rtw_is_cckratesonly_included(rate))
-                     return WIRELESS_11B;
-             else if (rtw_is_cckrates_included(rate))
-                     return  WIRELESS_11BG;
-             else
-                     return WIRELESS_11G;

drivers/staging/rtl8723bs/core/rtw_ieee80211.c bool rtw_is_cckratesonly_included(u8 *rate)

Quoted 10 lines

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +6 -1

-     }
+     /* could be pure B, pure G, or B/G */
+     if (rtw_is_cckratesonly_included(rate))
+             return WIRELESS_11B;
+     if (rtw_is_cckrates_included(rate))
+             return WIRELESS_11BG;
+     return WIRELESS_11G;
 }
 
 u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned

char *source,

drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_ie(u8 *pbuf, signed int index, signed

Quoted 1 line

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +0 -0
@@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed

int *len, signed int limit)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8

Quoted 15 lines

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +3 -4

              if (*p == index) {
                      *len = *(p + 1);
                      return p;
-             } else {
-                     tmp = *(p + 1);
-                     p += (tmp + 2);
-                     i += (tmp + 2);
              }
+             tmp = *(p + 1);
+             p += (tmp + 2);
+             i += (tmp + 2);
              if (i >= limit)
                      break;
      }

@@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8

*oui, u8 oui_len, u8 *ie, u

drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8

Quoted 11 lines

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -2

                              *ielen = in_ie[cnt+1]+2;
 
                      break;
-             } else {
-                     cnt += in_ie[cnt+1]+2; /* goto next */
              }
+             cnt += in_ie[cnt+1]+2; /* goto next */
      }
 
      return target_ie;

@@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8

*wps_ie, uint *wps_ielen)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16

Quoted 11 lines

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -2

                      cnt += in_ie[cnt+1]+2;
 
                      break;
-             } else {
-                     cnt += in_ie[cnt+1]+2; /* goto next */
              }
+             cnt += in_ie[cnt+1]+2; /* goto next */
      }
 
      return wpsie_ptr;

@@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16

target_attr_id, u8 *buf_att

Quoted 14 lines

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -3

                              *len_attr = attr_len;
 
                      break;
-             } else {
-                     attr_ptr += attr_len; /* goto next */
              }
+             attr_ptr += attr_len; /* goto next */
      }
 
      return target_attr_ptr;
--

2.25.1

Signature · 1 line

Sevinj.Aghayeva

Alison Schofield

2022-03-29 21:33 UTC #4 lore raw

snip

Quoted 6 lines · Alison Schofield

Quoted 2 lines · Sevinj Aghayeva
That one has already landed. Is it okay if I send v2 just for this patch?
Did you see it applied already?

I see Greg took a couple that I wanted to send for rework. BTW - if it was really something *wrong* I would ask you to send another patch, but since it's just style, let's just apply the learning to the next one.

Thanks!
Alison

Sevinj Aghayeva

author

2022-03-29 21:42 UTC #5 lore raw

In reply to Alison Schofield

Quoted 64 lines · Alison Schofield

Quoted 53 lines · Sevinj Aghayeva
Hi Alison,
Thanks for the feedback! Please see inline.
Quoted 13 lines · Alison Schofield
Hi Sevinj,
You can do this to make sure your commit msg 'looks' similar to others in the file:
$git log --pretty=oneline --abbrev-commit
d6ad258ae15d staging: rtl8723bs: Remove redundant else branches. 24e65aac9457 staging: rtl8723bs: remove rf type branching (fourth patch) 167fc30e8e51 staging: rtl8723bs: remove unused macros d7361874468f staging: rtl8723bs: fix camel case in struct wlan_bcn_info 6994aa430368 staging: rtl8723bs: fix camel case in struct ndis_802_11_ssid
Quoted 6 lines
That tells me the 'Remove' should not start with an uppercase and there should be no period at the end of line. Note that I look at this per patch, because different drivers and subsystems have different conventions. The point is to try to discern the prevailing style, and follow it.
Got it. The few examples that I saw in other places were using grammatically formatted sentences, but I'll take the subsystem conventions into account.
Quoted 7 lines
Please check the get_maintainers instruction in the first patch tutorial. I only got this:
$ git show HEAD | perl scripts/get_maintainer.pl --separator ,
--nokeywords --nogit --nogit-fallback --norolestats
Greg Kroah-Hartman <[email protected]>,
[email protected],[email protected]
My bad! I ran get_maintainer.pl without those options and got those a lot more email addresses.
Quoted 8 lines · Sevinj Aghayeva
Please state the 'why' of this patch. ie something like:
Adhere to Linux kernel coding style.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
This patch fixes the following checkpatch.pl warning:
The phrases 'This patch' is frowned upon as unecessarily wordy. Perhaps simply:
Reported by checkpatch:
Okay. I think we should update the https://kernelnewbies.org/PatchPhilosophy
because I took "This patch..." from there.

Quoted 7 lines

Yeah, there's a lot of 'This patch...' and 'Fixes...' and some other stuff that has fallen out of favor.
I just made a small edit getting rid of a 'Fixes' subject line.
How about if you update this example:
Subject: [OPW kernel] [PATCH] Staging: rtl8192e: Fix printk() warning style with your patch right here..once it is done!

Sounds good! I think I will have to send a request for editing pages like you explained in a previous email...

(Removed non-maintainers' email address from this thread. It's baffling that a script called get_maintainers would return email addresses of people who are not maintainers :-).)

Quoted 131 lines

Thanks!
Alison
Quoted 9 lines
Quoted 7 lines
You have another patch in flight with similar things to update. Please post a v2 of that one that addresses the feedback given here.
That one has already landed. Is it okay if I send v2 just for this patch?
Did you see it applied already?
Quoted 115 lines
Thanks!
Quoted 85 lines
I thought both patches had good scope in that they addressed multiple instances of the same issue in a single file.
Thanks!
Alison
Review
Quoted 9 lines
WARNING: else is not generally useful after a break or return
Signed-off-by Sevinj Aghayeva <[email protected]>
---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 32 ++++++++-----------
 1 file changed, 13 insertions(+), 19 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
drivers/staging/rtl8723bs/core/rtw_ieee80211.c bool rtw_is_cckratesonly_included(u8 *rate) · line 95
Quoted 6 lines
index b449be537376..27de086903e2 100644
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +0 -0
@@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
 
 int rtw_check_network_type(unsigned char *rate, int ratelen, int
channel)
Quoted 11 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -8
 {
-     if (channel > 14) {
+     if (channel > 14)
              return WIRELESS_INVALID;
-     } else { /*  could be pure B, pure G, or B/G */
-             if (rtw_is_cckratesonly_included(rate))
-                     return WIRELESS_11B;
-             else if (rtw_is_cckrates_included(rate))
-                     return  WIRELESS_11BG;
-             else
-                     return WIRELESS_11G;
drivers/staging/rtl8723bs/core/rtw_ieee80211.c bool rtw_is_cckratesonly_included(u8 *rate)
Quoted 10 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +6 -1
-     }
+     /* could be pure B, pure G, or B/G */
+     if (rtw_is_cckratesonly_included(rate))
+             return WIRELESS_11B;
+     if (rtw_is_cckrates_included(rate))
+             return WIRELESS_11BG;
+     return WIRELESS_11G;
 }
 
 u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned
char *source,
drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_ie(u8 *pbuf, signed int index, signed
Quoted 1 line
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +0 -0
@@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed
int *len, signed int limit)
drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8
Quoted 15 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +3 -4
              if (*p == index) {
                      *len = *(p + 1);
                      return p;
-             } else {
-                     tmp = *(p + 1);
-                     p += (tmp + 2);
-                     i += (tmp + 2);
              }
+             tmp = *(p + 1);
+             p += (tmp + 2);
+             i += (tmp + 2);
              if (i >= limit)
                      break;
      }
@@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8
*oui, u8 oui_len, u8 *ie, u
drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8
Quoted 11 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -2
                              *ielen = in_ie[cnt+1]+2;
 
                      break;
-             } else {
-                     cnt += in_ie[cnt+1]+2; /* goto next */
              }
+             cnt += in_ie[cnt+1]+2; /* goto next */
      }
 
      return target_ie;
@@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8
*wps_ie, uint *wps_ielen)
Quoted 9 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -2
                      cnt += in_ie[cnt+1]+2;
 
                      break;
-             } else {
-                     cnt += in_ie[cnt+1]+2; /* goto next */
              }
+             cnt += in_ie[cnt+1]+2; /* goto next */
      }
 
Quoted 18 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16
Quoted 2 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +0 -0
      return wpsie_ptr;
@@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16
target_attr_id, u8 *buf_att
Quoted 14 lines
drivers/staging/rtl8723bs/core/rtw_ieee80211.c +1 -3
                              *len_attr = attr_len;
 
                      break;
-             } else {
-                     attr_ptr += attr_len; /* goto next */
              }
+             attr_ptr += attr_len; /* goto next */
      }
 
      return target_attr_ptr;
--
2.25.1
--
Sevinj.Aghayeva

Signature · 1 line

Sevinj.Aghayeva