| Message ID | 2fefb2a6962f668f76db79d2f9a2b866b077e42f.1552023000.git.tsu.yubo@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: bonding: fix sparse warning | expand |
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
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
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); }
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(-)