| Message ID | 20170126102801.18914-1-cera@cera.cz |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Ivan, This patch is a bit involved. We need some time to review and test this to make sure it works with other (non-BE3/VF - Skyhawk, Lancer) devices. Also, IIRC we shouldn't see this issue with the latest FW. Thanks, -Harsha On Thu, Jan 26, 2017 at 3:58 PM, Ivan Vecera <cera@cera.cz> wrote: > Recent commit 34393529163a ("be2net: fix MAC addr setting on privileged > BE3 VFs") allows privileged BE3 VFs to set its MAC address during > initialization. Although the initial MAC for such VFs is already > programmed by parent PF the subsequent setting performed by VF is OK, > but in certain cases (after fresh boot) this command in VF can fail. > > The MAC should be initialized only when: > 1) no MAC is programmed (always except BE3 VFs during first init) > 2) programmed MAC is different from requested (e.g. MAC is set when > interface is down). In this case the initial MAC programmed by PF > needs to be deleted. > > The adapter->dev_mac contains MAC address currently programmed in HW so > it should be zeroed when the MAC is deleted from HW and should not be > filled when MAC is set when interface is down in be_mac_addr_set() as > no programming is performed in this case. > > Example of failure without the fix (immediately after fresh boot): > > # ip link set eth0 up <- eth0 is BE3 PF > be2net 0000:01:00.0 eth0: Link is Up > > # echo 1 > /sys/class/net/eth0/device/sriov_numvfs <- Create 1 VF > ... > be2net 0000:01:04.0: Emulex OneConnect(be3): VF port 0 > > # ip link set eth8 up <- eth8 is created privileged VF > be2net 0000:01:04.0: opcode 59-1 failed:status 1-76 > RTNETLINK answers: Input/output error > > # echo 0 > /sys/class/net/eth0/device/sriov_numvfs <- Delete VF > iommu: Removing device 0000:01:04.0 from group 33 > ... > > # echo 1 > /sys/class/net/eth0/device/sriov_numvfs <- Create it again > iommu: Removing device 0000:01:04.0 from group 33 > ... > > # ip link set eth8 up > be2net 0000:01:04.0 eth8: Link is Up > > Initialization is now OK. > > Fixes: 34393529163a ("be2net: fix MAC addr setting on privileged BE3 VFs") > Cc: Sathya Perla <sathya.perla@broadcom.com> > Cc: Ajit Khaparde <ajit.khaparde@broadcom.com> > Cc: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > Cc: Somnath Kotur <somnath.kotur@broadcom.com> > Signed-off-by: Ivan Vecera <cera@cera.cz> > --- > drivers/net/ethernet/emulex/benet/be_main.c | 31 ++++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c > index 1a7f8ad7b9c6..cf13b99b8551 100644 > --- a/drivers/net/ethernet/emulex/benet/be_main.c > +++ b/drivers/net/ethernet/emulex/benet/be_main.c > @@ -362,8 +362,10 @@ static int be_mac_addr_set(struct net_device *netdev, void *p) > status = -EPERM; > goto err; > } > -done: > + > + /* Remember currently programmed MAC */ > ether_addr_copy(adapter->dev_mac, addr->sa_data); > +done: > ether_addr_copy(netdev->dev_addr, addr->sa_data); > dev_info(dev, "MAC address changed to %pM\n", addr->sa_data); > return 0; > @@ -3618,8 +3620,10 @@ static void be_disable_if_filters(struct be_adapter *adapter) > { > /* Don't delete MAC on BE3 VFs without FILTMGMT privilege */ > if (!BEx_chip(adapter) || !be_virtfn(adapter) || > - check_privilege(adapter, BE_PRIV_FILTMGMT)) > + check_privilege(adapter, BE_PRIV_FILTMGMT)) { > be_dev_mac_del(adapter, adapter->pmac_id[0]); > + eth_zero_addr(adapter->dev_mac); > + } > > be_clear_uc_list(adapter); > be_clear_mc_list(adapter); > @@ -3773,12 +3777,25 @@ static int be_enable_if_filters(struct be_adapter *adapter) > if (status) > return status; > > - /* Don't add MAC on BE3 VFs without FILTMGMT privilege */ > - if (!BEx_chip(adapter) || !be_virtfn(adapter) || > - check_privilege(adapter, BE_PRIV_FILTMGMT)) { > + /* Normally this condition usually true as the ->dev_mac is zeroed. > + * But on BE3 VFs the initial MAC is pre-programmed by PF and > + * subsequent be_dev_mac_add() can fail (after fresh boot) > + */ > + if (!ether_addr_equal(adapter->dev_mac, adapter->netdev->dev_addr)) { > + int old_pmac_id = -1; > + > + /* Remember old programmed MAC if any - can happen on BE3 VF */ > + if (!is_zero_ether_addr(adapter->dev_mac)) > + old_pmac_id = adapter->pmac_id[0]; > + > status = be_dev_mac_add(adapter, adapter->netdev->dev_addr); > if (status) > return status; > + > + /* Delete old programmed MAC if necessary */ > + if (old_pmac_id > 0 && old_pmac_id != adapter->pmac_id[0]) > + be_dev_mac_del(adapter, old_pmac_id); > + > ether_addr_copy(adapter->dev_mac, adapter->netdev->dev_addr); > } > > @@ -4552,6 +4569,10 @@ static int be_mac_setup(struct be_adapter *adapter) > > memcpy(adapter->netdev->dev_addr, mac, ETH_ALEN); > memcpy(adapter->netdev->perm_addr, mac, ETH_ALEN); > + > + /* Initial MAC for BE3 VFs is already programmed by PF */ > + if (BEx_chip(adapter) && be_virtfn(adapter)) > + memcpy(adapter->dev_mac, mac, ETH_ALEN); > } > > return 0; > -- > 2.10.2 >
2017-01-27 13:38 GMT+01:00 Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>: > Hi Ivan, > > This patch is a bit involved. We need some time to review and test > this to make sure it works with other (non-BE3/VF - Skyhawk, Lancer) > devices. Also, IIRC we shouldn't see this issue with the latest FW. > I have done tests on BE3 PF & both privileged and non-privileged as well as on Skyhawk PF and VF. No problems found. Regarding firmware, I'm using version 11.1.215.0 on my devices and this version is affected on BE3. Older versions are affected as well. Ivan > > On Thu, Jan 26, 2017 at 3:58 PM, Ivan Vecera <cera@cera.cz> wrote: >> Recent commit 34393529163a ("be2net: fix MAC addr setting on privileged >> BE3 VFs") allows privileged BE3 VFs to set its MAC address during >> initialization. Although the initial MAC for such VFs is already >> programmed by parent PF the subsequent setting performed by VF is OK, >> but in certain cases (after fresh boot) this command in VF can fail. >> >> The MAC should be initialized only when: >> 1) no MAC is programmed (always except BE3 VFs during first init) >> 2) programmed MAC is different from requested (e.g. MAC is set when >> interface is down). In this case the initial MAC programmed by PF >> needs to be deleted. >> >> The adapter->dev_mac contains MAC address currently programmed in HW so >> it should be zeroed when the MAC is deleted from HW and should not be >> filled when MAC is set when interface is down in be_mac_addr_set() as >> no programming is performed in this case. >> >> Example of failure without the fix (immediately after fresh boot): >> >> # ip link set eth0 up <- eth0 is BE3 PF >> be2net 0000:01:00.0 eth0: Link is Up >> >> # echo 1 > /sys/class/net/eth0/device/sriov_numvfs <- Create 1 VF >> ... >> be2net 0000:01:04.0: Emulex OneConnect(be3): VF port 0 >> >> # ip link set eth8 up <- eth8 is created privileged VF >> be2net 0000:01:04.0: opcode 59-1 failed:status 1-76 >> RTNETLINK answers: Input/output error >> >> # echo 0 > /sys/class/net/eth0/device/sriov_numvfs <- Delete VF >> iommu: Removing device 0000:01:04.0 from group 33 >> ... >> >> # echo 1 > /sys/class/net/eth0/device/sriov_numvfs <- Create it again >> iommu: Removing device 0000:01:04.0 from group 33 >> ... >> >> # ip link set eth8 up >> be2net 0000:01:04.0 eth8: Link is Up >> >> Initialization is now OK. >> >> Fixes: 34393529163a ("be2net: fix MAC addr setting on privileged BE3 VFs") >> Cc: Sathya Perla <sathya.perla@broadcom.com> >> Cc: Ajit Khaparde <ajit.khaparde@broadcom.com> >> Cc: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> >> Cc: Somnath Kotur <somnath.kotur@broadcom.com> >> Signed-off-by: Ivan Vecera <cera@cera.cz> >> --- >> drivers/net/ethernet/emulex/benet/be_main.c | 31 ++++++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c >> index 1a7f8ad7b9c6..cf13b99b8551 100644 >> --- a/drivers/net/ethernet/emulex/benet/be_main.c >> +++ b/drivers/net/ethernet/emulex/benet/be_main.c >> @@ -362,8 +362,10 @@ static int be_mac_addr_set(struct net_device *netdev, void *p) >> status = -EPERM; >> goto err; >> } >> -done: >> + >> + /* Remember currently programmed MAC */ >> ether_addr_copy(adapter->dev_mac, addr->sa_data); >> +done: >> ether_addr_copy(netdev->dev_addr, addr->sa_data); >> dev_info(dev, "MAC address changed to %pM\n", addr->sa_data); >> return 0; >> @@ -3618,8 +3620,10 @@ static void be_disable_if_filters(struct be_adapter *adapter) >> { >> /* Don't delete MAC on BE3 VFs without FILTMGMT privilege */ >> if (!BEx_chip(adapter) || !be_virtfn(adapter) || >> - check_privilege(adapter, BE_PRIV_FILTMGMT)) >> + check_privilege(adapter, BE_PRIV_FILTMGMT)) { >> be_dev_mac_del(adapter, adapter->pmac_id[0]); >> + eth_zero_addr(adapter->dev_mac); >> + } >> >> be_clear_uc_list(adapter); >> be_clear_mc_list(adapter); >> @@ -3773,12 +3777,25 @@ static int be_enable_if_filters(struct be_adapter *adapter) >> if (status) >> return status; >> >> - /* Don't add MAC on BE3 VFs without FILTMGMT privilege */ >> - if (!BEx_chip(adapter) || !be_virtfn(adapter) || >> - check_privilege(adapter, BE_PRIV_FILTMGMT)) { >> + /* Normally this condition usually true as the ->dev_mac is zeroed. >> + * But on BE3 VFs the initial MAC is pre-programmed by PF and >> + * subsequent be_dev_mac_add() can fail (after fresh boot) >> + */ >> + if (!ether_addr_equal(adapter->dev_mac, adapter->netdev->dev_addr)) { >> + int old_pmac_id = -1; >> + >> + /* Remember old programmed MAC if any - can happen on BE3 VF */ >> + if (!is_zero_ether_addr(adapter->dev_mac)) >> + old_pmac_id = adapter->pmac_id[0]; >> + >> status = be_dev_mac_add(adapter, adapter->netdev->dev_addr); >> if (status) >> return status; >> + >> + /* Delete old programmed MAC if necessary */ >> + if (old_pmac_id > 0 && old_pmac_id != adapter->pmac_id[0]) >> + be_dev_mac_del(adapter, old_pmac_id); >> + >> ether_addr_copy(adapter->dev_mac, adapter->netdev->dev_addr); >> } >> >> @@ -4552,6 +4569,10 @@ static int be_mac_setup(struct be_adapter *adapter) >> >> memcpy(adapter->netdev->dev_addr, mac, ETH_ALEN); >> memcpy(adapter->netdev->perm_addr, mac, ETH_ALEN); >> + >> + /* Initial MAC for BE3 VFs is already programmed by PF */ >> + if (BEx_chip(adapter) && be_virtfn(adapter)) >> + memcpy(adapter->dev_mac, mac, ETH_ALEN); >> } >> >> return 0; >> -- >> 2.10.2 >>
2017-01-27 13:38 GMT+01:00 Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>: > Hi Ivan, > > This patch is a bit involved. We need some time to review and test > this to make sure it works with other (non-BE3/VF - Skyhawk, Lancer) > devices. Also, IIRC we shouldn't see this issue with the latest FW. > > Thanks, > -Harsha Btw. FW 11.1.215.0 is the latest available... As I described the problem is reproducible after fresh boot. 1) After _FRESH_ boot we have enp1s0f0 interface that is BE3 PF with SR-IOV enabled [root@sm-02 ~]# uname -r 4.10.0-rc4-00189-ga47b70e [root@sm-02 ~]# ethtool -i enp1s0f0 | egrep '(firm|bus)' firmware-version: 11.1.215.0 bus-info: 0000:01:00.0 [root@sm-02 ~]# lspci -vv -s 0000:01:00.0 | egrep '(Name:|VFs)' Product Name: OCe11102-FM 2P 10GbE Tomcat Enterprise CNA, NIC PF Initial VFs: 32, Total VFs: 32, Number of VFs: 0, Function Dependency Link: 00 2) Now we create 1 VF [root@sm-02 ~]# ip link set enp1s0f1 up be2net 0000:01:00.1 enp1s0f1: Link is Up [root@sm-02 ~]# echo 1 > /sys/class/net/enp1s0f1/device/sriov_numvfs ... be2net 0000:01:00.1: VF0 has FILTMGMT privilege ... be2net 0000:01:08.0: Emulex OneConnect(be3): VF port 1 be2net 0000:01:08.0 enp1s8f0: renamed from eth0 3) Now we have 1 privileged VF named enp1s8f0 that is down [root@sm-02 ~]# ip link show dev enp1s8f0 12: enp1s8f0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop portid 0200000000000000000000333630384254 state DOWN mode DEFAULT qlen 1000 link/ether 00:00:c9:9c:5c:60 brd ff:ff:ff:ff:ff:ff 4) Now we set the interface up (this fails) [root@sm-02 ~]# ip link set enp1s8f0 up be2net 0000:01:08.0: opcode 59-1 failed:status 1-76 RTNETLINK answers: Input/output error 5) Delete VF and create 2 new VFs [root@sm-02 ~]# echo 0 > /sys/class/net/enp1s0f1/device/sriov_numvfs ... [root@sm-02 ~]# echo 2 > /sys/class/net/enp1s0f1/device/sriov_numvfs ... be2net 0000:01:08.0 enp1s8f0: renamed from eth0 ... be2net 0000:01:08.1 enp1s8f1: renamed from eth0 6) We have 2 privileged VFs named enp1s8f0 and enp1s8f1, both down [root@sm-02 ~]# ip link show dev enp1s8f0 13: enp1s8f0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop portid 0200000000000000000000333630384254 state DOWN mode DEFAULT qlen 1000 link/ether 00:00:c9:9c:5c:60 brd ff:ff:ff:ff:ff:ff [root@sm-02 ~]# ip link show dev enp1s8f1 14: enp1s8f1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop portid 0200000000000000000000333630384254 state DOWN mode DEFAULT qlen 1000 link/ether 00:00:c9:9c:5c:61 brd ff:ff:ff:ff:ff:ff 7) Now set them up [root@sm-02 ~]# ip link set enp1s8f0 up be2net 0000:01:08.0 enp1s8f0: Link is Up [root@sm-02 ~]# ip link set enp1s8f1 up be2net 0000:01:08.1: opcode 59-1 failed:status 1-76 RTNETLINK answers: Input/output error As you can see the re-used interface enp1s8f0 is OK but not-yet-used interface enp1s8f1 fails. 8) Try to set enp1s8f1 up again [root@sm-02 ~]# ip link set enp1s8f1 up be2net 0000:01:08.1 enp1s8f1: Link is Up Now it is OK... This is because be_close() is called when be_enable_if_filters() fails. be_close() calls be_disable_if_filters() and there the MAC pre-programmed by PF is deleted by be_dev_mac_del(). So when you try to set the interface up again then the pre-programmed MAC is away and be_dev_mac_add() succeeds. Regards, Ivan
Sriharsha and other Broadcom folks, you must follow-up on Ivan's explanations and proof of testing. It is not acceptable for you to leave this patch's review state in limbo for days, no matter how complicated or big the patch is. You must at the very least say what you are doing, and how long it will take for you to do that before you will be able to fully respond. Thank you.
2017-01-31 19:01 GMT+01:00 David Miller <davem@davemloft.net>: > > > Sriharsha and other Broadcom folks, you must follow-up on Ivan's > explanations and proof of testing. > > It is not acceptable for you to leave this patch's review state in > limbo for days, no matter how complicated or big the patch is. You > must at the very least say what you are doing, and how long it will > take for you to do that before you will be able to fully respond. > > Thank you. Hi Dave, I have some review notes from them but they have not cc-ed netdev mailing list. I'm including their comments: Reply 1: ">> > + >> > + /* Delete old programmed MAC if necessary */ >> > + if (old_pmac_id > 0 && old_pmac_id != >> > adapter->pmac_id[0]) >> > + be_dev_mac_del(adapter, old_pmac_id); >> >> I'm trying to understand why you added the above call to be_dev_mac_del() >> here. >> Since be_close() --> be_disable_if_filters() already does this. >> > It is necessary, see: > > 1) Lets say, we have created BE3 VF... it has programmed MAC1 by PF > 2) This VF is initially down > 3) Lets change its MAC address to MAC2. Because the interface is down then > no programming is done and MAC1 is still active in HW > -> adapter->dev_mac = MAC1 and adapter->netdev->dev_addr = MAC2 > 4) Now we set the interface up and be_enable_if_filters() is executed > -> dev_mac and dev_addr is different. so MAC2 is programmed by > be_dev_mac_add() > 5) Interface is up and has MAC2 as an active MAC > 6) Lets change active MAC to MAC1 > -> this will fail if you didn't delete inital MAC1 at step 4 Ok, so this takes care of the case where we do another set-mac without an intervening interface-down right ? Can you please change the comment to something like this (drop the "if necessary" since I feel it is a distraction). "Delete the old programmed MAC as we successfully programmed a new MAC" Thanks for the fix, we will update you as soon as we are done with all our tests tomorrow." Reply 2: "I am seeing the collision error with the steps mentioned in your previous mail. MAC address will not be deleted when old_mac_id is 0. ZERO (0) is the valid pmac id. So the above check should be >=. Can you please verify again and fix it. Regards, Suresh." So I will submit v2 with corrected comment and condition check. Btw. Suresh, Harsha, next time please always cc netdev mailing list. It is not my responsibility to replicate your emails. Thanks, Ivan
My apologies for my previous replies... Gmail sent them as HTML emails that are not acceptable for ML... Thanks, Ivan
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index 1a7f8ad7b9c6..cf13b99b8551 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c @@ -362,8 +362,10 @@ static int be_mac_addr_set(struct net_device *netdev, void *p) status = -EPERM; goto err; } -done: + + /* Remember currently programmed MAC */ ether_addr_copy(adapter->dev_mac, addr->sa_data); +done: ether_addr_copy(netdev->dev_addr, addr->sa_data); dev_info(dev, "MAC address changed to %pM\n", addr->sa_data); return 0; @@ -3618,8 +3620,10 @@ static void be_disable_if_filters(struct be_adapter *adapter) { /* Don't delete MAC on BE3 VFs without FILTMGMT privilege */ if (!BEx_chip(adapter) || !be_virtfn(adapter) || - check_privilege(adapter, BE_PRIV_FILTMGMT)) + check_privilege(adapter, BE_PRIV_FILTMGMT)) { be_dev_mac_del(adapter, adapter->pmac_id[0]); + eth_zero_addr(adapter->dev_mac); + } be_clear_uc_list(adapter); be_clear_mc_list(adapter); @@ -3773,12 +3777,25 @@ static int be_enable_if_filters(struct be_adapter *adapter) if (status) return status; - /* Don't add MAC on BE3 VFs without FILTMGMT privilege */ - if (!BEx_chip(adapter) || !be_virtfn(adapter) || - check_privilege(adapter, BE_PRIV_FILTMGMT)) { + /* Normally this condition usually true as the ->dev_mac is zeroed. + * But on BE3 VFs the initial MAC is pre-programmed by PF and + * subsequent be_dev_mac_add() can fail (after fresh boot) + */ + if (!ether_addr_equal(adapter->dev_mac, adapter->netdev->dev_addr)) { + int old_pmac_id = -1; + + /* Remember old programmed MAC if any - can happen on BE3 VF */ + if (!is_zero_ether_addr(adapter->dev_mac)) + old_pmac_id = adapter->pmac_id[0]; + status = be_dev_mac_add(adapter, adapter->netdev->dev_addr); if (status) return status; + + /* Delete old programmed MAC if necessary */ + if (old_pmac_id > 0 && old_pmac_id != adapter->pmac_id[0]) + be_dev_mac_del(adapter, old_pmac_id); + ether_addr_copy(adapter->dev_mac, adapter->netdev->dev_addr); } @@ -4552,6 +4569,10 @@ static int be_mac_setup(struct be_adapter *adapter) memcpy(adapter->netdev->dev_addr, mac, ETH_ALEN); memcpy(adapter->netdev->perm_addr, mac, ETH_ALEN); + + /* Initial MAC for BE3 VFs is already programmed by PF */ + if (BEx_chip(adapter) && be_virtfn(adapter)) + memcpy(adapter->dev_mac, mac, ETH_ALEN); } return 0;
Recent commit 34393529163a ("be2net: fix MAC addr setting on privileged BE3 VFs") allows privileged BE3 VFs to set its MAC address during initialization. Although the initial MAC for such VFs is already programmed by parent PF the subsequent setting performed by VF is OK, but in certain cases (after fresh boot) this command in VF can fail. The MAC should be initialized only when: 1) no MAC is programmed (always except BE3 VFs during first init) 2) programmed MAC is different from requested (e.g. MAC is set when interface is down). In this case the initial MAC programmed by PF needs to be deleted. The adapter->dev_mac contains MAC address currently programmed in HW so it should be zeroed when the MAC is deleted from HW and should not be filled when MAC is set when interface is down in be_mac_addr_set() as no programming is performed in this case. Example of failure without the fix (immediately after fresh boot): # ip link set eth0 up <- eth0 is BE3 PF be2net 0000:01:00.0 eth0: Link is Up # echo 1 > /sys/class/net/eth0/device/sriov_numvfs <- Create 1 VF ... be2net 0000:01:04.0: Emulex OneConnect(be3): VF port 0 # ip link set eth8 up <- eth8 is created privileged VF be2net 0000:01:04.0: opcode 59-1 failed:status 1-76 RTNETLINK answers: Input/output error # echo 0 > /sys/class/net/eth0/device/sriov_numvfs <- Delete VF iommu: Removing device 0000:01:04.0 from group 33 ... # echo 1 > /sys/class/net/eth0/device/sriov_numvfs <- Create it again iommu: Removing device 0000:01:04.0 from group 33 ... # ip link set eth8 up be2net 0000:01:04.0 eth8: Link is Up Initialization is now OK. Fixes: 34393529163a ("be2net: fix MAC addr setting on privileged BE3 VFs") Cc: Sathya Perla <sathya.perla@broadcom.com> Cc: Ajit Khaparde <ajit.khaparde@broadcom.com> Cc: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> Cc: Somnath Kotur <somnath.kotur@broadcom.com> Signed-off-by: Ivan Vecera <cera@cera.cz> --- drivers/net/ethernet/emulex/benet/be_main.c | 31 ++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)