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

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 1c1008c793fa net: bcmgenet: add main driver file.

The bot has also determined it's probably a bug fixing patch. (score: 49.2621)

The bot has tested the following trees: v4.16, v4.15.15, v4.14.32, v4.9.92, 
v4.4.126.

v4.16: Build OK!
v4.15.15: Build OK!
v4.14.32: Build OK!
v4.9.92: Build OK!
v4.4.126: Build OK!

--
Thanks,
Sasha

[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

2da8 :
2da8:   e1d038b8ldrhr3, [r0, #136]  ; 0x88
2dac:   e92d4070push{r4, r5, r6, lr}
2db0:   e6bf3fb3rev16   r3, r3  <==
2db4:   e6ff3073uxthr3, r3
2db8:   e3530b02cmp r3, #2048   ; 0x800
2dbc:   0a20beq 2e44 
2dc0:   e30826ddmovwr2, #34525  ; 0x86dd
2dc4:   e1530002cmp r3, r2
2dc8:   1a1cbne 2e40 
2dcc:   e5906098ldr r6, [r0, #152]  ; 0x98
2dd0:   e1d028bcldrhr2, [r0, #140]  ; 0x8c
2dd4:   e0862002add r2, r6, r2
2dd8:   e5d22006ldrbr2, [r2, #6]
2ddc:   e242e011sub lr, r2, #17
2de0:   e1d0c6b4ldrhip, [r0, #100]  ; 0x64
2de4:   e16fef1eclz lr, lr
2de8:   e590509cldr r5, [r0, #156]  ; 0x9c
2dec:   e1d046b6ldrhr4, [r0, #102]  ; 0x66
2df0:   e1a0e2aelsr lr, lr, #5
2df4:   e3520006cmp r2, #6
2df8:   11a0200emovne   r2, lr
2dfc:   038e2001orreq   r2, lr, #1
2e00:   e352cmp r2, #0
2e04:   0a0bbeq 2e38 
2e08:   e0455006sub r5, r5, r6
2e0c:   e3530b02cmp r3, #2048   ; 0x800
2e10:   13a0e000movne   lr, #0
2e14:   020ee001andeq   lr, lr, #1
2e18:   e04c3005sub r3, ip, r5
2e1c:   e35ecmp lr, #0
2e20:   e2433040sub r3, r3, #64 ; 0x40
2e24:   e6ff3073uxthr3, r3
2e28:   e0844003add r4, r4, r3
2e2c:   e1842803orr r2, r4, r3, lsl #16
2e30:   e3822102orr r2, r2, #-2147483648; 0x8000
2e34:   13822902orrne   r2, r2, #32768  ; 0x8000
2e38:   e5812030str r2, [r1, #48]   ; 0x30
2e3c:   e8bd8070pop {r4, r5, r6, pc}
2e40:   e8bd8070pop {r4, r5, r6, pc}
2e44:   e5906098ldr r6, [r0, #152]  ; 0x98
2e48:   e1d028bcldrhr2, [r0, #140]  ; 0x8c
2e4c:   e0862002add r2, r6, r2
2e50:   e5d22009ldrbr2, [r2, #9]
2e54:   eae0b   2ddc 


Whereas my changes result in:

164 instructions, and no rev* since everything is resolved at compile time.

0810 :
 810:   e92d4070push{r4, r5, r6, lr}
 814:   e1d0e8b8ldrhlr, [r0, #136]  ; 0x88
 818:   e35e0008cmp lr, #8
 81c:   0a20beq 8a4 
 820:   e30d3d86movwr3, #56710  ; 0xdd86
 824:   e15e0003cmp lr, r3
 828:   1a1cbne 8a0 
 82c:   e5906098ldr r6, [r0, #152]  ; 0x98
 830:   e1d038bcldrhr3, [r0, #140]  ; 0x8c
 834:   e0863003add r3, r6, r3
 838:   e5d33006ldrbr3, [r3, #6]
 83c:   e2434011sub r4, r3, #17
 840:   e1d0c6b4ldrhip, [r0, #100]  ; 0x64
 844:   e16f4f14clz r4, r4
 848:   e590209cldr r2, [r0, #156]  ; 0x9c
 84c:   e1d056b6ldrhr5, [r0, #102]  ; 0x66
 850:   e1a042a4lsr r4, r4, #5
 854:   e3530006cmp r3, #6
 858:   11a03004movne   r3, r4
 85c:   03843001orreq   r3, r4, #1
 860:   e353cmp r3, #0
 864:   0a0bbeq 898 
 868:   e0422006sub r2, r2, r6
 86c:   

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.

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

[David Miller]

From: Al Viro 
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 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.

[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 
---
 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