* [PATCH net 0/2] net: Broadcom drivers sparse fixes
@ 2018-04-02 22:58 Florian Fainelli
2018-04-02 22:58 ` [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() Florian Fainelli
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Florian Fainelli @ 2018-04-02 22:58 UTC (permalink / raw)
To: netdev; +Cc: Florian Fainelli, Doug Berger, open list
Hi all,
This patch series fixes the same warning reported by sparse in bcmsysport and
bcmgenet in the code that deals with inserting the TX checksum pointers:
drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: warning: cast from restricted __be16
drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: warning: incorrect type in argument 1 (different base types)
drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: expected unsigned short [unsigned] [usertype] val
drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: got restricted __be16 [usertype] protocol
This patch fixes both issues by using the same construct and not swapping
skb->protocol but instead the values we are checking against.
Florian Fainelli (2):
net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum()
net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb()
drivers/net/ethernet/broadcom/bcmsysport.c | 11 ++++++-----
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 11 ++++++-----
2 files changed, 12 insertions(+), 10 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() 2018-04-02 22:58 [PATCH net 0/2] net: Broadcom drivers sparse fixes Florian Fainelli @ 2018-04-02 22:58 ` Florian Fainelli 2018-04-03 16:29 ` Al Viro 2018-04-02 22:58 ` [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb() Florian Fainelli 2018-04-04 15:07 ` [PATCH net 0/2] net: Broadcom drivers sparse fixes David Miller 2 siblings, 1 reply; 9+ messages in thread From: Florian Fainelli @ 2018-04-02 22:58 UTC (permalink / raw) To: netdev; +Cc: Florian Fainelli, Doug Berger, open list skb->protocol is a __be16 which we would be calling htons() against, while this is not wrong per-se as it correctly results in swapping the value on LE hosts, this still upsets sparse. Adopt a similar pattern to what other drivers do and just assign ip_ver to skb->protocol, and then use htons() against the different constants such that the compiler can resolve the values at build time. Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index b1e35a9accf1..aeb329690f78 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1460,7 +1460,7 @@ static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev, struct sk_buff *new_skb; u16 offset; u8 ip_proto; - u16 ip_ver; + __be16 ip_ver; u32 tx_csum_info; if (unlikely(skb_headroom(skb) < sizeof(*status))) { @@ -1480,12 +1480,12 @@ static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev, status = (struct status_64 *)skb->data; if (skb->ip_summed == CHECKSUM_PARTIAL) { - ip_ver = htons(skb->protocol); + ip_ver = skb->protocol; switch (ip_ver) { - case ETH_P_IP: + case htons(ETH_P_IP): ip_proto = ip_hdr(skb)->protocol; break; - case ETH_P_IPV6: + case htons(ETH_P_IPV6): ip_proto = ipv6_hdr(skb)->nexthdr; break; default: @@ -1501,7 +1501,8 @@ static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev, */ if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP) { tx_csum_info |= STATUS_TX_CSUM_LV; - if (ip_proto == IPPROTO_UDP && ip_ver == ETH_P_IP) + if (ip_proto == IPPROTO_UDP && + ip_ver == htons(ETH_P_IP)) tx_csum_info |= STATUS_TX_CSUM_PROTO_UDP; } else { tx_csum_info = 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() 2018-04-02 22:58 ` [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() Florian Fainelli @ 2018-04-03 16:29 ` Al Viro 2018-04-03 16:33 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Al Viro @ 2018-04-03 16:29 UTC (permalink / raw) To: Florian Fainelli; +Cc: netdev, Doug Berger, open list On Mon, Apr 02, 2018 at 03:58:55PM -0700, Florian Fainelli wrote: > skb->protocol is a __be16 which we would be calling htons() against, > while this is not wrong per-se as it correctly results in swapping the > value on LE hosts, this still upsets sparse. Adopt a similar pattern to > what other drivers do and just assign ip_ver to skb->protocol, and then > use htons() against the different constants such that the compiler can > resolve the values at build time. Fuck that and fuck other drivers sharing that "pattern". It's not hard to remember: host-endian to net-endian short is htons host-endian to net-endian long is htonl net-endian to host-endian short is ntohs net-endian to host-endian long is ntohl That's *it*. No convoluted changes needed, just use the right primitive. You are turning trivial one-liners ("conversions between host- and net-endian are the same in both directions, so the current code works even though we are using the wrong primitive, confusing the readers. Use the right one") which are absolutely self-contained and safe into much more massive changes that are harder to follow and which are *not* self-contained at all. Don't do it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() 2018-04-03 16:29 ` Al Viro @ 2018-04-03 16:33 ` David Miller 2018-04-03 16:45 ` Al Viro 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2018-04-03 16:33 UTC (permalink / raw) To: viro; +Cc: f.fainelli, netdev, opendmb, linux-kernel From: Al Viro <viro@ZenIV.linux.org.uk> Date: Tue, 3 Apr 2018 17:29:33 +0100 > On Mon, Apr 02, 2018 at 03:58:55PM -0700, Florian Fainelli wrote: >> skb->protocol is a __be16 which we would be calling htons() against, >> while this is not wrong per-se as it correctly results in swapping the >> value on LE hosts, this still upsets sparse. Adopt a similar pattern to >> what other drivers do and just assign ip_ver to skb->protocol, and then >> use htons() against the different constants such that the compiler can >> resolve the values at build time. > > Fuck that and fuck other drivers sharing that "pattern". It's not hard > to remember: > host-endian to net-endian short is htons > host-endian to net-endian long is htonl > net-endian to host-endian short is ntohs > net-endian to host-endian long is ntohl > > That's *it*. No convoluted changes needed, just use the right primitive. > You are turning trivial one-liners ("conversions between host- and net-endian > are the same in both directions, so the current code works even though we > are using the wrong primitive, confusing the readers. Use the right one") > which are absolutely self-contained and safe into much more massive changes > that are harder to follow and which are *not* self-contained at all. > > Don't do it. Yes Al, however the pattern choosen here is probably cheaper on little endian: __beXX val = x; switch (val) { case htons(ETH_P_FOO): ... } This way only the compiler byte swaps the constants at compile time, no code is actually generated to do it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() 2018-04-03 16:33 ` David Miller @ 2018-04-03 16:45 ` Al Viro 2018-04-03 20:40 ` Florian Fainelli 0 siblings, 1 reply; 9+ messages in thread From: Al Viro @ 2018-04-03 16:45 UTC (permalink / raw) To: David Miller; +Cc: f.fainelli, netdev, opendmb, linux-kernel On Tue, Apr 03, 2018 at 12:33:05PM -0400, David Miller wrote: > Yes Al, however the pattern choosen here is probably cheaper on little endian: > > __beXX val = x; > switch (val) { > case htons(ETH_P_FOO): > ... > } > > This way only the compiler byte swaps the constants at compile time, > no code is actually generated to do it. That's not obvious, actually - depends upon how sparse the switch ends up being. You can easily lose more than a single byteswap insn on worse cascase of comparisons. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() 2018-04-03 16:45 ` Al Viro @ 2018-04-03 20:40 ` Florian Fainelli 0 siblings, 0 replies; 9+ messages in thread From: Florian Fainelli @ 2018-04-03 20:40 UTC (permalink / raw) To: Al Viro, David Miller; +Cc: netdev, opendmb, linux-kernel On 04/03/2018 09:45 AM, Al Viro wrote: > On Tue, Apr 03, 2018 at 12:33:05PM -0400, David Miller wrote: > >> Yes Al, however the pattern choosen here is probably cheaper on little endian: >> >> __beXX val = x; >> switch (val) { >> case htons(ETH_P_FOO): >> ... >> } >> >> This way only the compiler byte swaps the constants at compile time, >> no code is actually generated to do it. > > That's not obvious, actually - depends upon how sparse the switch ends > up being. You can easily lose more than a single byteswap insn on > worse cascase of comparisons. David is right though here and this is used primarily on LE hosts, the code produced with your suggested change does fix the sparse warning and is functional, but results in less efficient code with a rev16 instruction being used. If you would prefer me to use __constant_htons() to make that clearer, I don't have any objection to that. 172 instructions including a rev16 00002da8 <bcmgenet_insert_status>: 2da8: e1d038b8 ldrh r3, [r0, #136] ; 0x88 2dac: e92d4070 push {r4, r5, r6, lr} 2db0: e6bf3fb3 rev16 r3, r3 <============== 2db4: e6ff3073 uxth r3, r3 2db8: e3530b02 cmp r3, #2048 ; 0x800 2dbc: 0a000020 beq 2e44 <bcmgenet_insert_status+0x9c> 2dc0: e30826dd movw r2, #34525 ; 0x86dd 2dc4: e1530002 cmp r3, r2 2dc8: 1a00001c bne 2e40 <bcmgenet_insert_status+0x98> 2dcc: e5906098 ldr r6, [r0, #152] ; 0x98 2dd0: e1d028bc ldrh r2, [r0, #140] ; 0x8c 2dd4: e0862002 add r2, r6, r2 2dd8: e5d22006 ldrb r2, [r2, #6] 2ddc: e242e011 sub lr, r2, #17 2de0: e1d0c6b4 ldrh ip, [r0, #100] ; 0x64 2de4: e16fef1e clz lr, lr 2de8: e590509c ldr r5, [r0, #156] ; 0x9c 2dec: e1d046b6 ldrh r4, [r0, #102] ; 0x66 2df0: e1a0e2ae lsr lr, lr, #5 2df4: e3520006 cmp r2, #6 2df8: 11a0200e movne r2, lr 2dfc: 038e2001 orreq r2, lr, #1 2e00: e3520000 cmp r2, #0 2e04: 0a00000b beq 2e38 <bcmgenet_insert_status+0x90> 2e08: e0455006 sub r5, r5, r6 2e0c: e3530b02 cmp r3, #2048 ; 0x800 2e10: 13a0e000 movne lr, #0 2e14: 020ee001 andeq lr, lr, #1 2e18: e04c3005 sub r3, ip, r5 2e1c: e35e0000 cmp lr, #0 2e20: e2433040 sub r3, r3, #64 ; 0x40 2e24: e6ff3073 uxth r3, r3 2e28: e0844003 add r4, r4, r3 2e2c: e1842803 orr r2, r4, r3, lsl #16 2e30: e3822102 orr r2, r2, #-2147483648 ; 0x80000000 2e34: 13822902 orrne r2, r2, #32768 ; 0x8000 2e38: e5812030 str r2, [r1, #48] ; 0x30 2e3c: e8bd8070 pop {r4, r5, r6, pc} 2e40: e8bd8070 pop {r4, r5, r6, pc} 2e44: e5906098 ldr r6, [r0, #152] ; 0x98 2e48: e1d028bc ldrh r2, [r0, #140] ; 0x8c 2e4c: e0862002 add r2, r6, r2 2e50: e5d22009 ldrb r2, [r2, #9] 2e54: eaffffe0 b 2ddc <bcmgenet_insert_status+0x34> Whereas my changes result in: 164 instructions, and no rev* since everything is resolved at compile time. 00000810 <bcmgenet_insert_status>: 810: e92d4070 push {r4, r5, r6, lr} 814: e1d0e8b8 ldrh lr, [r0, #136] ; 0x88 818: e35e0008 cmp lr, #8 81c: 0a000020 beq 8a4 <bcmgenet_insert_status+0x94> 820: e30d3d86 movw r3, #56710 ; 0xdd86 824: e15e0003 cmp lr, r3 828: 1a00001c bne 8a0 <bcmgenet_insert_status+0x90> 82c: e5906098 ldr r6, [r0, #152] ; 0x98 830: e1d038bc ldrh r3, [r0, #140] ; 0x8c 834: e0863003 add r3, r6, r3 838: e5d33006 ldrb r3, [r3, #6] 83c: e2434011 sub r4, r3, #17 840: e1d0c6b4 ldrh ip, [r0, #100] ; 0x64 844: e16f4f14 clz r4, r4 848: e590209c ldr r2, [r0, #156] ; 0x9c 84c: e1d056b6 ldrh r5, [r0, #102] ; 0x66 850: e1a042a4 lsr r4, r4, #5 854: e3530006 cmp r3, #6 858: 11a03004 movne r3, r4 85c: 03843001 orreq r3, r4, #1 860: e3530000 cmp r3, #0 864: 0a00000b beq 898 <bcmgenet_insert_status+0x88> 868: e0422006 sub r2, r2, r6 86c: e35e0008 cmp lr, #8 870: 13a0e000 movne lr, #0 874: 0204e001 andeq lr, r4, #1 878: e04c2002 sub r2, ip, r2 87c: e35e0000 cmp lr, #0 880: e2422040 sub r2, r2, #64 ; 0x40 884: e6ff2072 uxth r2, r2 888: e0855002 add r5, r5, r2 88c: e1853802 orr r3, r5, r2, lsl #16 890: e3833102 orr r3, r3, #-2147483648 ; 0x80000000 894: 13833902 orrne r3, r3, #32768 ; 0x8000 898: e5813030 str r3, [r1, #48] ; 0x30 89c: e8bd8070 pop {r4, r5, r6, pc} 8a0: e8bd8070 pop {r4, r5, r6, pc} 8a4: e5906098 ldr r6, [r0, #152] ; 0x98 8a8: e1d038bc ldrh r3, [r0, #140] ; 0x8c 8ac: e0863003 add r3, r6, r3 8b0: e5d33009 ldrb r3, [r3, #9] 8b4: eaffffe0 b 83c <bcmgenet_insert_status+0x2c> -- Florian ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb() 2018-04-02 22:58 [PATCH net 0/2] net: Broadcom drivers sparse fixes Florian Fainelli 2018-04-02 22:58 ` [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() Florian Fainelli @ 2018-04-02 22:58 ` Florian Fainelli 2018-04-03 16:23 ` Al Viro 2018-04-04 15:07 ` [PATCH net 0/2] net: Broadcom drivers sparse fixes David Miller 2 siblings, 1 reply; 9+ messages in thread From: Florian Fainelli @ 2018-04-02 22:58 UTC (permalink / raw) To: netdev; +Cc: Florian Fainelli, Doug Berger, open list skb->protocol is a __be16 which we would be calling htons() against, while this is not wrong per-se as it correctly results in swapping the value on LE hosts, this still upsets sparse. Adopt a similar pattern to what other drivers do and just assign ip_ver to skb->protocol, and then use htons() against the different constants such that the compiler can resolve the values at build time. Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/ethernet/broadcom/bcmsysport.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index 3fc549b88c43..879db1c700bd 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -1133,7 +1133,7 @@ static struct sk_buff *bcm_sysport_insert_tsb(struct sk_buff *skb, u32 csum_info; u8 ip_proto; u16 csum_start; - u16 ip_ver; + __be16 ip_ver; /* Re-allocate SKB if needed */ if (unlikely(skb_headroom(skb) < sizeof(*tsb))) { @@ -1152,12 +1152,12 @@ static struct sk_buff *bcm_sysport_insert_tsb(struct sk_buff *skb, memset(tsb, 0, sizeof(*tsb)); if (skb->ip_summed == CHECKSUM_PARTIAL) { - ip_ver = htons(skb->protocol); + ip_ver = skb->protocol; switch (ip_ver) { - case ETH_P_IP: + case htons(ETH_P_IP): ip_proto = ip_hdr(skb)->protocol; break; - case ETH_P_IPV6: + case htons(ETH_P_IPV6): ip_proto = ipv6_hdr(skb)->nexthdr; break; default: @@ -1171,7 +1171,8 @@ static struct sk_buff *bcm_sysport_insert_tsb(struct sk_buff *skb, if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP) { csum_info |= L4_LENGTH_VALID; - if (ip_proto == IPPROTO_UDP && ip_ver == ETH_P_IP) + if (ip_proto == IPPROTO_UDP && + ip_ver == htons(ETH_P_IP)) csum_info |= L4_UDP; } else { csum_info = 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb() 2018-04-02 22:58 ` [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb() Florian Fainelli @ 2018-04-03 16:23 ` Al Viro 0 siblings, 0 replies; 9+ messages in thread From: Al Viro @ 2018-04-03 16:23 UTC (permalink / raw) To: Florian Fainelli; +Cc: netdev, Doug Berger, open list On Mon, Apr 02, 2018 at 03:58:56PM -0700, Florian Fainelli wrote: > skb->protocol is a __be16 which we would be calling htons() against, > while this is not wrong per-se as it correctly results in swapping the > value on LE hosts, this still upsets sparse. Adopt a similar pattern to > what other drivers do and just assign ip_ver to skb->protocol, and then > use htons() against the different constants such that the compiler can > resolve the values at build time. This is completely bogus. What sparse is complaining about is htons used to convert from big-endian to host-endian. Which is what ntohs is for. IOW, instead of all that crap just - ip_ver = htons(skb->protocol); + ip_ver = ntohs(skb->protocol); and be done with that. Same in other drivers with the same problem. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 0/2] net: Broadcom drivers sparse fixes 2018-04-02 22:58 [PATCH net 0/2] net: Broadcom drivers sparse fixes Florian Fainelli 2018-04-02 22:58 ` [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() Florian Fainelli 2018-04-02 22:58 ` [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb() Florian Fainelli @ 2018-04-04 15:07 ` David Miller 2 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2018-04-04 15:07 UTC (permalink / raw) To: f.fainelli; +Cc: netdev, opendmb, linux-kernel From: Florian Fainelli <f.fainelli@gmail.com> Date: Mon, 2 Apr 2018 15:58:54 -0700 > This patch series fixes the same warning reported by sparse in bcmsysport and > bcmgenet in the code that deals with inserting the TX checksum pointers: > > drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: warning: cast from restricted __be16 > drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: warning: incorrect type in argument 1 (different base types) > drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: expected unsigned short [unsigned] [usertype] val > drivers/net/ethernet/broadcom/bcmsysport.c:1155:26: got restricted __be16 [usertype] protocol > > This patch fixes both issues by using the same construct and not swapping > skb->protocol but instead the values we are checking against. Series applied, thanks Florian. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-04-04 15:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-02 22:58 [PATCH net 0/2] net: Broadcom drivers sparse fixes Florian Fainelli 2018-04-02 22:58 ` [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum() Florian Fainelli 2018-04-03 16:29 ` Al Viro 2018-04-03 16:33 ` David Miller 2018-04-03 16:45 ` Al Viro 2018-04-03 20:40 ` Florian Fainelli 2018-04-02 22:58 ` [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb() Florian Fainelli 2018-04-03 16:23 ` Al Viro 2018-04-04 15:07 ` [PATCH net 0/2] net: Broadcom drivers sparse fixes David Miller
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).