Fixed sparse parse error:
Expected constant expression in case statement.
Signed-off-by: Bing Sun <[email protected]>
---
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
On Aug 11, 2016, at 23:25, Jes Sorensen <[email protected]> wrote:
> Bing Sun <[email protected]> writes:
>> Fixed sparse parse error:
>> Expected constant expression in case statement.
>>
>> Signed-off-by: Bing Sun <[email protected]>
>> ---
>> 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.
On Sat, 2016-08-13 at 17:26 +0800, sunbing wrote:
> On Aug 12, 2016, at 22:30, Jes Sorensen <[email protected]> wrote:
> > sunbing <[email protected]> writes:
> > > On Aug 11, 2016, at 23:25, Jes Sorensen <[email protected]> wrote:
> > > > Bing Sun <[email protected]> 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
On Aug 12, 2016, at 22:30, Jes Sorensen <[email protected]> wrote:
> sunbing <[email protected]> writes:
>> On Aug 11, 2016, at 23:25, Jes Sorensen <[email protected]> wrote:
>>
>>> Bing Sun <[email protected]> writes:
>>>> Fixed sparse parse error:
>>>> Expected constant expression in case statement.
>>>>
>>>> Signed-off-by: Bing Sun <[email protected]>
>>>> ---
>>>> 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.
Bing Sun <[email protected]> writes:
> Fixed sparse parse error:
> Expected constant expression in case statement.
>
> Signed-off-by: Bing Sun <[email protected]>
> ---
> 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
sunbing <[email protected]> writes:
> On Aug 11, 2016, at 23:25, Jes Sorensen <[email protected]> wrote:
>
>> Bing Sun <[email protected]> writes:
>>> Fixed sparse parse error:
>>> Expected constant expression in case statement.
>>>
>>> Signed-off-by: Bing Sun <[email protected]>
>>> ---
>>> 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
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
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.
> 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