Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460568 > unrolled thread

[PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue

Back to article view | Back to linux.kernel

Contents

  [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue Bing Sun <sunbing@redflag-linux.com> - 2016-08-11 16:20 +0200
    Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue Jes Sorensen <Jes.Sorensen@redhat.com> - 2016-08-11 17:30 +0200
      Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue sunbing <sunbing@redflag-linux.com> - 2016-08-12 01:00 +0200
        Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue Jes Sorensen <Jes.Sorensen@redhat.com> - 2016-08-12 16:40 +0200
          Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue sunbing <sunbing@redflag-linux.com> - 2016-08-13 11:30 +0200
            Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is  variable issue Joe Perches <joe@perches.com> - 2016-08-14 14:10 +0200
              Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is  variable issue Johannes Berg <johannes@sipsolutions.net> - 2016-08-14 14:20 +0200
                Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is  variable issue Joe Perches <joe@perches.com> - 2016-08-14 14:30 +0200
                  Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is  variable issue Johannes Berg <johannes@sipsolutions.net> - 2016-08-15 08:10 +0200

#1460568 — [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue

Fixed sparse parse error:
Expected constant expression in case statement.

Signed-off-by: Bing Sun <sunbing@redflag-linux.com>
---
 drivers/staging/rtl8723au/os_dep/os_intfs.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
index b8848c2..f30d5d2 100644
--- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
+++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
@@ -283,14 +283,13 @@ static u32 rtw_classify8021d(struct sk_buff *skb)
 	 */
 	if (skb->priority >= 256 && skb->priority <= 263)
 		return skb->priority - 256;
-	switch (skb->protocol) {
-	case htons(ETH_P_IP):
+
+	if (skb->protocol == htons(ETH_P_IP)) {
 		dscp = ip_hdr(skb)->tos & 0xfc;
-		break;
-	default:
-		return 0;
+		return dscp >> 5;
 	}
-	return dscp >> 5;
+
+	return 0;
 }
 
 static u16 rtw_select_queue(struct net_device *dev, struct sk_buff *skb,
-- 
2.1.0

[toc] | [next] | [standalone]

#1460612

Bing Sun <sunbing@redflag-linux.com> writes:
> Fixed sparse parse error:
> Expected constant expression in case statement.
>
> Signed-off-by: Bing Sun <sunbing@redflag-linux.com>
> ---
>  drivers/staging/rtl8723au/os_dep/os_intfs.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
> index b8848c2..f30d5d2 100644
> --- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
> +++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
> @@ -283,14 +283,13 @@ static u32 rtw_classify8021d(struct sk_buff *skb)
>  	 */
>  	if (skb->priority >= 256 && skb->priority <= 263)
>  		return skb->priority - 256;
> -	switch (skb->protocol) {
> -	case htons(ETH_P_IP):
> +
> +	if (skb->protocol == htons(ETH_P_IP)) {
>  		dscp = ip_hdr(skb)->tos & 0xfc;
> -		break;
> -	default:
> -		return 0;
> +		return dscp >> 5;
>  	}
> -	return dscp >> 5;
> +
> +	return 0;
>  }

Pardon me here, but I find it really hard to see how this change is an
improvement over the old code in any shape or form.

Jes

[toc] | [prev] | [next] | [standalone]

#1460846

On Aug 11, 2016, at 23:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:

> Bing Sun <sunbing@redflag-linux.com> writes:
>> Fixed sparse parse error:
>> Expected constant expression in case statement.
>> 
>> Signed-off-by: Bing Sun <sunbing@redflag-linux.com>
>> ---
>> drivers/staging/rtl8723au/os_dep/os_intfs.c | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
>> index b8848c2..f30d5d2 100644
>> --- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
>> +++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
>> @@ -283,14 +283,13 @@ static u32 rtw_classify8021d(struct sk_buff *skb)
>> 	 */
>> 	if (skb->priority >= 256 && skb->priority <= 263)
>> 		return skb->priority - 256;
>> -	switch (skb->protocol) {
>> -	case htons(ETH_P_IP):
>> +
>> +	if (skb->protocol == htons(ETH_P_IP)) {
>> 		dscp = ip_hdr(skb)->tos & 0xfc;
>> -		break;
>> -	default:
>> -		return 0;
>> +		return dscp >> 5;
>> 	}
>> -	return dscp >> 5;
>> +
>> +	return 0;
>> }
> 
> Pardon me here, but I find it really hard to see how this change is an
> improvement over the old code in any shape or form.
> 
> Jes


There is no functional improvement. 
But before this patch, when we do: make C=1 M=drivers/staging/rtl8723au/
An error output: 
drivers/staging/rtl8723au//os_dep/os_intfs.c:287:14: error: Expected constant expression in case statement
To avoid sparse parse error, a case statement converts to an if statement.
So we got this patch.

[toc] | [prev] | [next] | [standalone]

#1461252

sunbing <sunbing@redflag-linux.com> writes:
> On Aug 11, 2016, at 23:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>
>> Bing Sun <sunbing@redflag-linux.com> writes:
>>> Fixed sparse parse error:
>>> Expected constant expression in case statement.
>>> 
>>> Signed-off-by: Bing Sun <sunbing@redflag-linux.com>
>>> ---
>>> drivers/staging/rtl8723au/os_dep/os_intfs.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
>>> index b8848c2..f30d5d2 100644
>>> --- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
>>> +++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
>>> @@ -283,14 +283,13 @@ static u32 rtw_classify8021d(struct sk_buff *skb)
>>> 	 */
>>> 	if (skb->priority >= 256 && skb->priority <= 263)
>>> 		return skb->priority - 256;
>>> -	switch (skb->protocol) {
>>> -	case htons(ETH_P_IP):
>>> +
>>> +	if (skb->protocol == htons(ETH_P_IP)) {
>>> 		dscp = ip_hdr(skb)->tos & 0xfc;
>>> -		break;
>>> -	default:
>>> -		return 0;
>>> +		return dscp >> 5;
>>> 	}
>>> -	return dscp >> 5;
>>> +
>>> +	return 0;
>>> }
>> 
>> Pardon me here, but I find it really hard to see how this change is an
>> improvement over the old code in any shape or form.
>> 
>> Jes
>
> There is no functional improvement. 
> But before this patch, when we do: make C=1 M=drivers/staging/rtl8723au/
> An error output: 
> drivers/staging/rtl8723au//os_dep/os_intfs.c:287:14: error: Expected
> constant expression in case statement
> To avoid sparse parse error, a case statement converts to an if statement.
> So we got this patch.

Hello

I understand this part, but it seems to me we are changing the code due
to a broken test case in sparse. Does the warning go away if you use
__constant_htons() instead of htons()?

Jes

[toc] | [prev] | [next] | [standalone]

#1461610

On Aug 12, 2016, at 22:30, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:

> sunbing <sunbing@redflag-linux.com> writes:
>> On Aug 11, 2016, at 23:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>> 
>>> Bing Sun <sunbing@redflag-linux.com> writes:
>>>> Fixed sparse parse error:
>>>> Expected constant expression in case statement.
>>>> 
>>>> Signed-off-by: Bing Sun <sunbing@redflag-linux.com>
>>>> ---
>>>> drivers/staging/rtl8723au/os_dep/os_intfs.c | 11 +++++------
>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/drivers/staging/rtl8723au/os_dep/os_intfs.c b/drivers/staging/rtl8723au/os_dep/os_intfs.c
>>>> index b8848c2..f30d5d2 100644
>>>> --- a/drivers/staging/rtl8723au/os_dep/os_intfs.c
>>>> +++ b/drivers/staging/rtl8723au/os_dep/os_intfs.c
>>>> @@ -283,14 +283,13 @@ static u32 rtw_classify8021d(struct sk_buff *skb)
>>>> 	 */
>>>> 	if (skb->priority >= 256 && skb->priority <= 263)
>>>> 		return skb->priority - 256;
>>>> -	switch (skb->protocol) {
>>>> -	case htons(ETH_P_IP):
>>>> +
>>>> +	if (skb->protocol == htons(ETH_P_IP)) {
>>>> 		dscp = ip_hdr(skb)->tos & 0xfc;
>>>> -		break;
>>>> -	default:
>>>> -		return 0;
>>>> +		return dscp >> 5;
>>>> 	}
>>>> -	return dscp >> 5;
>>>> +
>>>> +	return 0;
>>>> }
>>> 
>>> Pardon me here, but I find it really hard to see how this change is an
>>> improvement over the old code in any shape or form.
>>> 
>>> Jes
>> 
>> There is no functional improvement. 
>> But before this patch, when we do: make C=1 M=drivers/staging/rtl8723au/
>> An error output: 
>> drivers/staging/rtl8723au//os_dep/os_intfs.c:287:14: error: Expected
>> constant expression in case statement
>> To avoid sparse parse error, a case statement converts to an if statement.
>> So we got this patch.
> 
> Hello
> 
> I understand this part, but it seems to me we are changing the code due
> to a broken test case in sparse. Does the warning go away if you use
> __constant_htons() instead of htons()?
> 
> Jes

Thanks for your guidance.

1. If I use __constant_htons, checkpatch.pl will warning:
    WARNING: __constant_htons should be htons

2. In os_intfs.c: rtw_classify8021d, there are only one case statement and a 
default statement. So, convert "switch case" to "if else" is more readable in my opinion.

So, I pushed this patch.

There are some patches convert use of __constant_htons to htons in kernel logs. 
Will there be a new patch convert to htons in the future if I use  __constant_htons now ?

After search through kernel code, there are 158 "case htons(...)" statements and 
2 "case __constant_htons(...)" statements. Does this mean we can ignore sparse 
error and use "case htons(...)" ?

It makes me confused. More help, please.

Regards.

[toc] | [prev] | [next] | [standalone]

#1462084 — Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue

On Sat, 2016-08-13 at 17:26 +0800, sunbing wrote:
> On Aug 12, 2016, at 22:30, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> > sunbing <sunbing@redflag-linux.com> writes:
> > > On Aug 11, 2016, at 23:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> > > > Bing Sun <sunbing@redflag-linux.com> writes:
> > > > > 
> > > > > Fixed sparse parse error:
> > > > > Expected constant expression in case statement.
[]
> > > > Pardon me here, but I find it really hard to see how this change is an
> > > > improvement over the old code in any shape or form.
> > > There is no functional improvement. 
> > > But before this patch, when we do: make C=1 M=drivers/staging/rtl8723au/
> > > An error output: 
> > > drivers/staging/rtl8723au//os_dep/os_intfs.c:287:14: error: Expected
> > > constant expression in case statement
> > > To avoid sparse parse error, a case statement converts to an if statement.
> > > So we got this patch.
> > Hello
> > 
> > I understand this part, but it seems to me we are changing the code due
> > to a broken test case in sparse. Does the warning go away if you use
> > __constant_htons() instead of htons()?
> > 
> > Jes
> Thanks for your guidance.
> 
> 1. If I use __constant_htons, checkpatch.pl will warning:
>     WARNING: __constant_htons should be htons
> 
> 2. In os_intfs.c: rtw_classify8021d, there are only one case statement and a 
> default statement. So, convert "switch case" to "if else" is more readable in my opinion.
> 
> So, I pushed this patch.
> 
> There are some patches convert use of __constant_htons to htons in kernel logs. 
> Will there be a new patch convert to htons in the future if I use  __constant_htons now ?
> 
> After search through kernel code, there are 158 "case htons(...)" statements and 
> 2 "case __constant_htons(...)" statements. Does this mean we can ignore sparse 
> error and use "case htons(...)" ?
> 
> It makes me confused. More help, please.

It's a sparse defect.

Try again after patching sparse with Jes' patch:
http://marc.info/?l=linux-sparse&m=147091200720267&w=3

[toc] | [prev] | [next] | [standalone]

#1462112 — Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue

> It's a sparse defect.
> 
> Try again after patching sparse with Jes' patch:
> http://marc.info/?l=linux-sparse&m=147091200720267&w=3

Mine, not Jes's ;-)

But there's another patch going into the kernel to fix it:

https://www.ozlabs.org/~akpm/mmotm/broken-out/byteswap-dont-use-__builtin_bswap-with-sparse.patch

johannes

[toc] | [prev] | [next] | [standalone]

#1462113 — Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue

On Sun, 2016-08-14 at 14:15 +0200, Johannes Berg wrote:
> > 
> > It's a sparse defect.
> > 
> > Try again after patching sparse with Jes' patch:
> > http://marc.info/?l=linux-sparse&m=147091200720267&w=3
> Mine, not Jes's ;-)

Right, sorry 'bout that.

> But there's another patch going into the kernel to fix it:
> 
> https://www.ozlabs.org/~akpm/mmotm/broken-out/byteswap-dont-use-__builtin_bswap-with-sparse.patch

Thanks.

Maybe this test should be sparse version checked after
sparse is updated.

[toc] | [prev] | [next] | [standalone]

#1462562 — Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue

On Sun, 2016-08-14 at 05:23 -0700, Joe Perches wrote:
> 
> Maybe this test should be sparse version checked after
> sparse is updated.

*If* sparse ever gets updated :) I don't think it's been updated much
lately.
That said, I'm not even sure how, and what version, etc. so obviously
that'd have to be done after the fact. But since nobody will ever
compile the kernel with sparse's code generator, it also doesn't matter
anyway.

johannes

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web