brcmsmac: Remove always false 'channel < 0' statement
diff mbox series

Message ID 20191127054358.GA59549@LGEARND20B15
State Accepted
Commit 37bc6c72f5b76f3c6ae00e3001d3187100fbeb1b
Delegated to: Kalle Valo
Headers show
Series
  • brcmsmac: Remove always false 'channel < 0' statement
Related show

Commit Message

Austin Kim Nov. 27, 2019, 5:43 a.m. UTC
As 'channel' is declared as u16, the following statement is always false.
   channel < 0

So we can remove unnecessary 'always false' statement.

Signed-off-by: Austin Kim <austindh.kim@gmail.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sergei Shtylyov Nov. 27, 2019, 10:48 a.m. UTC | #1
On 27.11.2019 8:43, Austin Kim wrote:

> As 'channel' is declared as u16, the following statement is always false.
>     channel < 0
> 
> So we can remove unnecessary 'always false' statement.

    It's an expression, not a statement.

> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
> index 3f09d89..7f2c15c 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
> @@ -5408,7 +5408,7 @@ int brcms_c_set_channel(struct brcms_c_info *wlc, u16 channel)
>   {
>   	u16 chspec = ch20mhz_chspec(channel);
>   
> -	if (channel < 0 || channel > MAXCHANNEL)
> +	if (channel > MAXCHANNEL)
>   		return -EINVAL;
>   
>   	if (!brcms_c_valid_chanspec_db(wlc->cmi, chspec))

MBR, Sergei
Austin Kim Nov. 27, 2019, 1:02 p.m. UTC | #2
2019년 11월 27일 (수) 오후 7:48, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com>님이 작성:
>
> On 27.11.2019 8:43, Austin Kim wrote:
>
> > As 'channel' is declared as u16, the following statement is always false.
> >     channel < 0
> >
> > So we can remove unnecessary 'always false' statement.
>
>     It's an expression, not a statement.
>

According to below link, it is okay to use 'statement' in above case.
https://en.wikipedia.org/wiki/Statement_(computer_science)

Why don't you show your opition about patch rather than commit message?

Thanks,
Austin Kim

> > Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> > ---
> >   drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
> > index 3f09d89..7f2c15c 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
> > @@ -5408,7 +5408,7 @@ int brcms_c_set_channel(struct brcms_c_info *wlc, u16 channel)
> >   {
> >       u16 chspec = ch20mhz_chspec(channel);
> >
> > -     if (channel < 0 || channel > MAXCHANNEL)
> > +     if (channel > MAXCHANNEL)
> >               return -EINVAL;
> >
> >       if (!brcms_c_valid_chanspec_db(wlc->cmi, chspec))
>
> MBR, Sergei
>
Kalle Valo Nov. 27, 2019, 1:35 p.m. UTC | #3
Austin Kim <austindh.kim@gmail.com> writes:

> 2019년 11월 27일 (수) 오후 7:48, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com>님이 작성:
>>
>> On 27.11.2019 8:43, Austin Kim wrote:
>>
>> > As 'channel' is declared as u16, the following statement is always false.
>> >     channel < 0
>> >
>> > So we can remove unnecessary 'always false' statement.
>>
>>     It's an expression, not a statement.
>>
>
> According to below link, it is okay to use 'statement' in above case.
> https://en.wikipedia.org/wiki/Statement_(computer_science)

I don't have time to start arguing about this, and I'm no C language
lawyer either, but all I say is that I agree with Sergei here.

> Why don't you show your opition about patch rather than commit message?

But this comment is not ok. Patch review (including commit logs) is the
core principle of upstream development so you need to have an open mind
for all comments, even the ones you don't like.
Austin Kim Nov. 27, 2019, 10:21 p.m. UTC | #4
2019년 11월 27일 (수) 오후 10:35, Kalle Valo <kvalo@codeaurora.org>님이 작성:
>
> Austin Kim <austindh.kim@gmail.com> writes:
>
> > 2019년 11월 27일 (수) 오후 7:48, Sergei Shtylyov
> > <sergei.shtylyov@cogentembedded.com>님이 작성:
> >>
> >> On 27.11.2019 8:43, Austin Kim wrote:
> >>
> >> > As 'channel' is declared as u16, the following statement is always false.
> >> >     channel < 0
> >> >
> >> > So we can remove unnecessary 'always false' statement.
> >>
> >>     It's an expression, not a statement.
> >>
> >
> > According to below link, it is okay to use 'statement' in above case.
> > https://en.wikipedia.org/wiki/Statement_(computer_science)
>
> I don't have time to start arguing about this, and I'm no C language
> lawyer either, but all I say is that I agree with Sergei here.

Thanks for your opinion.
I will use 'expression' rather than 'statement' when I upstream
similar patch later.

>
> > Why don't you show your opition about patch rather than commit message?
>
> But this comment is not ok. Patch review (including commit logs) is the
> core principle of upstream development so you need to have an open mind
> for all comments, even the ones you don't like.

Oh! I Agreed.
If I were you, I would leave similar comment.

Thanks,
Austin Kim

>
> --
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo Dec. 18, 2019, 6:28 p.m. UTC | #5
Austin Kim <austindh.kim@gmail.com> wrote:

> As 'channel' is declared as u16, the following expression is always false.
>    channel < 0
> 
> So we can remove unnecessary 'always false' statement.
> 
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>

Patch applied to wireless-drivers-next.git, thanks.

37bc6c72f5b7 brcmsmac: Remove always false 'channel < 0' statement

Patch
diff mbox series

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
index 3f09d89..7f2c15c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
@@ -5408,7 +5408,7 @@  int brcms_c_set_channel(struct brcms_c_info *wlc, u16 channel)
 {
 	u16 chspec = ch20mhz_chspec(channel);
 
-	if (channel < 0 || channel > MAXCHANNEL)
+	if (channel > MAXCHANNEL)
 		return -EINVAL;
 
 	if (!brcms_c_valid_chanspec_db(wlc->cmi, chspec))