[PATCH 2/2] net: bonding: fix incorrect type in assignment

2019-03-07 Thread Bo YU 
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 
---
 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 + or -\n");
} else {
-   target = newval->value;
+   target = cpu_to_be32(newval->value);
ret = bond_option_arp_ip_target_add(bond, target);
}
 
-- 
2.11.0

Re: [PATCH 2/2] net: bonding: fix incorrect type in assignment

2019-03-07 Thread Jay Vosburgh 
Bo YU  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 
>---
> 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 + or -\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.vosbu...@canonical.com

Re: [PATCH 2/2] net: bonding: fix incorrect type in assignment

2019-03-08 Thread Bo YU 
On Fri, Mar 8, 2019 at 2:51 PM Jay Vosburgh  wrote:
>
> Bo YU  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 
> >---
> > 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 + or -\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.vosbu...@canonical.com