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

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

[Bing Sun]

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

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

[Jes Sorensen]

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

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

[sunbing]

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.

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

[Jes Sorensen]

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

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

[sunbing]

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.

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

[Joe Perches]

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

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

[Johannes Berg]

> 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

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

[Joe Perches]

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.

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

[Johannes Berg]

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