* [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue @ 2016-08-11 14:11 Bing Sun 2016-08-11 15:25 ` Jes Sorensen 0 siblings, 1 reply; 9+ messages in thread From: Bing Sun @ 2016-08-11 14:11 UTC (permalink / raw) To: Larry.Finger, Jes.Sorensen Cc: gregkh, linux-wireless, devel, linux-kernel, sunbing.linux, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-11 14:11 [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue Bing Sun @ 2016-08-11 15:25 ` Jes Sorensen 2016-08-11 22:50 ` sunbing 0 siblings, 1 reply; 9+ messages in thread From: Jes Sorensen @ 2016-08-11 15:25 UTC (permalink / raw) To: Bing Sun Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-11 15:25 ` Jes Sorensen @ 2016-08-11 22:50 ` sunbing 2016-08-12 14:30 ` Jes Sorensen 0 siblings, 1 reply; 9+ messages in thread From: sunbing @ 2016-08-11 22:50 UTC (permalink / raw) To: Jes Sorensen Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-11 22:50 ` sunbing @ 2016-08-12 14:30 ` Jes Sorensen 2016-08-13 9:26 ` sunbing 0 siblings, 1 reply; 9+ messages in thread From: Jes Sorensen @ 2016-08-12 14:30 UTC (permalink / raw) To: sunbing Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-12 14:30 ` Jes Sorensen @ 2016-08-13 9:26 ` sunbing 2016-08-14 12:07 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: sunbing @ 2016-08-13 9:26 UTC (permalink / raw) To: Jes Sorensen Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-13 9:26 ` sunbing @ 2016-08-14 12:07 ` Joe Perches 2016-08-14 12:15 ` Johannes Berg 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2016-08-14 12:07 UTC (permalink / raw) To: sunbing, Jes Sorensen Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux, Johannes Berg 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-14 12:07 ` Joe Perches @ 2016-08-14 12:15 ` Johannes Berg 2016-08-14 12:23 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Johannes Berg @ 2016-08-14 12:15 UTC (permalink / raw) To: Joe Perches, sunbing, Jes Sorensen Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-14 12:15 ` Johannes Berg @ 2016-08-14 12:23 ` Joe Perches 2016-08-15 6:07 ` Johannes Berg 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2016-08-14 12:23 UTC (permalink / raw) To: Johannes Berg, sunbing, Jes Sorensen Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue 2016-08-14 12:23 ` Joe Perches @ 2016-08-15 6:07 ` Johannes Berg 0 siblings, 0 replies; 9+ messages in thread From: Johannes Berg @ 2016-08-15 6:07 UTC (permalink / raw) To: Joe Perches, sunbing, Jes Sorensen Cc: Larry.Finger, gregkh, linux-wireless, devel, linux-kernel, sunbing.linux 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-08-15 6:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-11 14:11 [PATCH] Staging: rtl8723au: os_intfs: fixed case statement is variable issue Bing Sun 2016-08-11 15:25 ` Jes Sorensen 2016-08-11 22:50 ` sunbing 2016-08-12 14:30 ` Jes Sorensen 2016-08-13 9:26 ` sunbing 2016-08-14 12:07 ` Joe Perches 2016-08-14 12:15 ` Johannes Berg 2016-08-14 12:23 ` Joe Perches 2016-08-15 6:07 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).