[PATCH net 0/2] net: Broadcom drivers sparse fixes

[PATCH net 0/2] net: Broadcom drivers sparse fixes

[Florian Fainelli]

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

[PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum()

[Florian Fainelli]

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

[PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb()

[Florian Fainelli]

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

Re: [PATCH net 2/2] net: systemport: Fix sparse warnings in bcm_sysport_insert_tsb()

[Al Viro]

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.

Re: [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum()

[Al Viro]

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.

Re: [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum()

[David Miller]

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.

Re: [PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum()

[Al Viro]

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.

[PATCH net 1/2] net: bcmgenet: Fix sparse warnings in bcmgenet_put_tx_csum()

[Florian Fainelli]

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

Re: [PATCH net 0/2] net: Broadcom drivers sparse fixes

[David Miller]

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.