* [PATCH 0/2] net: bonding: fix sparse warning @ 2019-03-08 5:33 Bo YU 2019-03-08 5:33 ` [PATCH 1/2] net: bonding: fix restricted __be16 degrades to integer Bo YU 2019-03-08 5:34 ` [PATCH 2/2] net: bonding: fix incorrect type in assignment Bo YU 0 siblings, 2 replies; 6+ messages in thread From: Bo YU @ 2019-03-08 5:33 UTC (permalink / raw) To: j.vosburgh, vfalico, andy, davem; +Cc: Bo YU, netdev, linux-kernel, yuzibode Bo YU (2): net: bonding: fix restricted __be16 degrades to integer net: bonding: fix incorrect type in assignment drivers/net/bonding/bond_main.c | 8 ++++---- drivers/net/bonding/bond_options.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] net: bonding: fix restricted __be16 degrades to integer 2019-03-08 5:33 [PATCH 0/2] net: bonding: fix sparse warning Bo YU @ 2019-03-08 5:33 ` Bo YU 2019-03-08 20:54 ` Jay Vosburgh 2019-03-08 5:34 ` [PATCH 2/2] net: bonding: fix incorrect type in assignment Bo YU 1 sibling, 1 reply; 6+ messages in thread From: Bo YU @ 2019-03-08 5:33 UTC (permalink / raw) To: j.vosburgh, vfalico, andy, davem; +Cc: Bo YU, netdev, linux-kernel, yuzibode There are some warning when: sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/ drivers/net/bonding/bond_main.c:2385:26: warning: restricted __be16 degrades to integer drivers/net/bonding/bond_main.c:2391:20: warning: restricted __be16 degrades to integer ... drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer So fix it. Signed-off-by: Bo YU <tsu.yubo@gmail.com> --- drivers/net/bonding/bond_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index b59708c35faf..135fec28daa9 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2382,13 +2382,13 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, return; } - if (!tags || tags->vlan_proto == VLAN_N_VID) + if (!tags || be16_to_cpu(tags->vlan_proto) == VLAN_N_VID) goto xmit; tags++; /* Go through all the tags backwards and add them to the packet */ - while (tags->vlan_proto != VLAN_N_VID) { + while (be16_to_cpu(tags->vlan_proto) != VLAN_N_VID) { if (!tags->vlan_id) { tags++; continue; @@ -3238,7 +3238,7 @@ static inline u32 bond_eth_hash(struct sk_buff *skb) ep = skb_header_pointer(skb, 0, sizeof(hdr_tmp), &hdr_tmp); if (ep) - return ep->h_dest[5] ^ ep->h_source[5] ^ ep->h_proto; + return ep->h_dest[5] ^ ep->h_source[5] ^ be16_to_cpu(ep->h_proto); return 0; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: bonding: fix restricted __be16 degrades to integer 2019-03-08 5:33 ` [PATCH 1/2] net: bonding: fix restricted __be16 degrades to integer Bo YU @ 2019-03-08 20:54 ` Jay Vosburgh 0 siblings, 0 replies; 6+ messages in thread From: Jay Vosburgh @ 2019-03-08 20:54 UTC (permalink / raw) To: Bo YU; +Cc: vfalico, andy, davem, netdev, linux-kernel, yuzibode Bo YU <tsu.yubo@gmail.com> wrote: >There are some warning when: > >sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/ > >drivers/net/bonding/bond_main.c:2385:26: warning: restricted __be16 degrades to integer >drivers/net/bonding/bond_main.c:2391:20: warning: restricted __be16 degrades to integer >... >drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer >drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer > >So fix it. > >Signed-off-by: Bo YU <tsu.yubo@gmail.com> >--- > drivers/net/bonding/bond_main.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index b59708c35faf..135fec28daa9 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2382,13 +2382,13 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, > return; > } > >- if (!tags || tags->vlan_proto == VLAN_N_VID) >+ if (!tags || be16_to_cpu(tags->vlan_proto) == VLAN_N_VID) > goto xmit; > > tags++; > > /* Go through all the tags backwards and add them to the packet */ >- while (tags->vlan_proto != VLAN_N_VID) { >+ while (be16_to_cpu(tags->vlan_proto) != VLAN_N_VID) { I believe both of the above are incorrect, as vlan_proto is set explicitly to VLAN_N_VID (in host byte order) by bond_verify_device_path as a sentinel value. Byte swapping the tags->vlan_proto value would cause the test or loop to miss the sentinel. > if (!tags->vlan_id) { > tags++; > continue; >@@ -3238,7 +3238,7 @@ static inline u32 bond_eth_hash(struct sk_buff *skb) > > ep = skb_header_pointer(skb, 0, sizeof(hdr_tmp), &hdr_tmp); > if (ep) >- return ep->h_dest[5] ^ ep->h_source[5] ^ ep->h_proto; >+ return ep->h_dest[5] ^ ep->h_source[5] ^ be16_to_cpu(ep->h_proto); This is probably harmless, other than adding work to the transmit path. -J --- -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] net: bonding: fix incorrect type in assignment 2019-03-08 5:33 [PATCH 0/2] net: bonding: fix sparse warning Bo YU 2019-03-08 5:33 ` [PATCH 1/2] net: bonding: fix restricted __be16 degrades to integer Bo YU @ 2019-03-08 5:34 ` Bo YU 2019-03-08 6:51 ` Jay Vosburgh 1 sibling, 1 reply; 6+ messages in thread From: Bo YU @ 2019-03-08 5:34 UTC (permalink / raw) To: j.vosburgh, vfalico, andy, davem; +Cc: Bo YU, netdev, linux-kernel, yuzibode There are some warning when: sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/ drivers/net/bonding/bond_main.c:2438:40: warning: incorrect type in assignment (different base types) drivers/net/bonding/bond_main.c:2438:40: expected restricted __be16 [usertype] vlan_proto drivers/net/bonding/bond_main.c:2438:40: ... rivers/net/bonding/bond_options.c:1089:24: warning: incorrect type in assignment (different base types) drivers/net/bonding/bond_options.c:1089:24: expected restricted __be32 [addressable] [usertype] target drivers/net/bonding/bond_options.c:1089:24: got unsigned long long const [usertype] value So fix it Signed-off-by: Bo YU <tsu.yubo@gmail.com> --- drivers/net/bonding/bond_main.c | 2 +- drivers/net/bonding/bond_options.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 135fec28daa9..07e52d863e91 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2435,7 +2435,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); if (!tags) return ERR_PTR(-ENOMEM); - tags[level].vlan_proto = VLAN_N_VID; + tags[level].vlan_proto = cpu_to_be16(VLAN_N_VID); return tags; } diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index da1fc17295d9..3a196999bd1b 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -1086,7 +1086,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond, else netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n"); } else { - target = newval->value; + target = cpu_to_be32(newval->value); ret = bond_option_arp_ip_target_add(bond, target); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] net: bonding: fix incorrect type in assignment 2019-03-08 5:34 ` [PATCH 2/2] net: bonding: fix incorrect type in assignment Bo YU @ 2019-03-08 6:51 ` Jay Vosburgh 2019-03-08 8:28 ` Bo YU 0 siblings, 1 reply; 6+ messages in thread From: Jay Vosburgh @ 2019-03-08 6:51 UTC (permalink / raw) To: Bo YU; +Cc: vfalico, andy, davem, netdev, linux-kernel, yuzibode Bo YU <tsu.yubo@gmail.com> wrote: >There are some warning when: > >sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/ > >drivers/net/bonding/bond_main.c:2438:40: warning: incorrect type in assignment (different base types) >drivers/net/bonding/bond_main.c:2438:40: expected restricted __be16 [usertype] vlan_proto >drivers/net/bonding/bond_main.c:2438:40: >... >rivers/net/bonding/bond_options.c:1089:24: warning: incorrect type in assignment (different base types) >drivers/net/bonding/bond_options.c:1089:24: expected restricted __be32 [addressable] [usertype] target >drivers/net/bonding/bond_options.c:1089:24: got unsigned long long const [usertype] value > >So fix it > >Signed-off-by: Bo YU <tsu.yubo@gmail.com> >--- > drivers/net/bonding/bond_main.c | 2 +- > drivers/net/bonding/bond_options.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 135fec28daa9..07e52d863e91 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2435,7 +2435,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, > tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); > if (!tags) > return ERR_PTR(-ENOMEM); >- tags[level].vlan_proto = VLAN_N_VID; >+ tags[level].vlan_proto = cpu_to_be16(VLAN_N_VID); > return tags; > } > >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >index da1fc17295d9..3a196999bd1b 100644 >--- a/drivers/net/bonding/bond_options.c >+++ b/drivers/net/bonding/bond_options.c >@@ -1086,7 +1086,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond, > else > netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n"); > } else { >- target = newval->value; >+ target = cpu_to_be32(newval->value); > ret = bond_option_arp_ip_target_add(bond, target); I'm not sure this is correct; if I'm reading the call path correctly, bond_changelink will if (data[IFLA_BOND_ARP_IP_TARGET]) { [...] __be32 target; [...] target = nla_get_be32(attr); bond_opt_initval(&newval, (__force u64)target); err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS, &newval); thus, newval.value is initially be32, but stored in a u64. __bond_opt_set will call bond_opt_parse, which in turn will call bond_option_arp_ip_targets_set (via .set), and the change above would swap the newval.value back to host order (on little endian architectures for which cpu_to_be32 is not a no-op). Am I misunderstanding? Did you test this change on an x86 or other little endian system? -J --- -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] net: bonding: fix incorrect type in assignment 2019-03-08 6:51 ` Jay Vosburgh @ 2019-03-08 8:28 ` Bo YU 0 siblings, 0 replies; 6+ messages in thread From: Bo YU @ 2019-03-08 8:28 UTC (permalink / raw) To: Jay Vosburgh Cc: vfalico, Andy Gospodarek, David Miller, Netdev, open list, 于波 On Fri, Mar 8, 2019 at 2:51 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > > Bo YU <tsu.yubo@gmail.com> wrote: > > >There are some warning when: > > > >sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/ > > > >drivers/net/bonding/bond_main.c:2438:40: warning: incorrect type in assignment (different base types) > >drivers/net/bonding/bond_main.c:2438:40: expected restricted __be16 [usertype] vlan_proto > >drivers/net/bonding/bond_main.c:2438:40: > >... > >rivers/net/bonding/bond_options.c:1089:24: warning: incorrect type in assignment (different base types) > >drivers/net/bonding/bond_options.c:1089:24: expected restricted __be32 [addressable] [usertype] target > >drivers/net/bonding/bond_options.c:1089:24: got unsigned long long const [usertype] value > > > >So fix it > > > >Signed-off-by: Bo YU <tsu.yubo@gmail.com> > >--- > > drivers/net/bonding/bond_main.c | 2 +- > > drivers/net/bonding/bond_options.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > >index 135fec28daa9..07e52d863e91 100644 > >--- a/drivers/net/bonding/bond_main.c > >+++ b/drivers/net/bonding/bond_main.c > >@@ -2435,7 +2435,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, > > tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); > > if (!tags) > > return ERR_PTR(-ENOMEM); > >- tags[level].vlan_proto = VLAN_N_VID; > >+ tags[level].vlan_proto = cpu_to_be16(VLAN_N_VID); > > return tags; > > } > > > >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > >index da1fc17295d9..3a196999bd1b 100644 > >--- a/drivers/net/bonding/bond_options.c > >+++ b/drivers/net/bonding/bond_options.c > >@@ -1086,7 +1086,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond, > > else > > netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n"); > > } else { > >- target = newval->value; > >+ target = cpu_to_be32(newval->value); > > ret = bond_option_arp_ip_target_add(bond, target); > > I'm not sure this is correct; if I'm reading the call path > correctly, bond_changelink will > > if (data[IFLA_BOND_ARP_IP_TARGET]) { > [...] > __be32 target; > [...] > target = nla_get_be32(attr); > > bond_opt_initval(&newval, (__force u64)target); > err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS, > &newval); > > thus, newval.value is initially be32, but stored in a u64. > __bond_opt_set will call bond_opt_parse, which in turn will call > bond_option_arp_ip_targets_set (via .set), and the change above would > swap the newval.value back to host order (on little endian architectures > for which cpu_to_be32 is not a no-op). > > Am I misunderstanding? Did you test this change on an x86 or > other little endian system? Hi, After above patch: I have enable BONDING config in .config and dmesg: [ 0.247417] Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011) I don't have the HW, so just add printfk in bond_option_arp_ip_targets_set However, i don't get what i want to print. I do the test in qemu-system-x86_64. I do not know how to hit the driver maybe. So, not confirmed :( > > -J > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-08 20:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-08 5:33 [PATCH 0/2] net: bonding: fix sparse warning Bo YU 2019-03-08 5:33 ` [PATCH 1/2] net: bonding: fix restricted __be16 degrades to integer Bo YU 2019-03-08 20:54 ` Jay Vosburgh 2019-03-08 5:34 ` [PATCH 2/2] net: bonding: fix incorrect type in assignment Bo YU 2019-03-08 6:51 ` Jay Vosburgh 2019-03-08 8:28 ` Bo YU
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).