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(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index b449be537376..27de086903e2 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -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;
--
2.25.1
On Tue, Mar 29, 2022 at 01:59:52PM -0700, Sevinj Aghayeva wrote:
> Hi Alison,
>
> Thanks for the feedback! Please see inline.
>
> On Tue, Mar 29, 2022 at 12:58 PM Alison Schofield <
> [email protected]> wrote:
>
> > 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.
> >
>
> 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.
>
>
> > 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.
>
>
> >
> >
> >
> > 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.
> >
> > > 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
>
>
> >
> > 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?
>
> Thanks!
>
>
> >
> > I thought both patches had good scope in that they addressed multiple
> > instances of the same issue in a single file.
> >
> > Thanks!
> > Alison
> >
> > >
> > > 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(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > > index b449be537376..27de086903e2 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > > @@ -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;
> > > --
> > > 2.25.1
> > >
> > >
> >
>
>
> --
>
> Sevinj.Aghayeva
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.
> 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
>
> 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(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index b449be537376..27de086903e2 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -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;
> --
> 2.25.1
>
>
snip
On Tue, Mar 29, 2022 at 02:24:26PM -0700, Alison Schofield wrote:
> On Tue, Mar 29, 2022 at 01:59:52PM -0700, Sevinj Aghayeva wrote:
> >
> > 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