| Message ID | 20170131190131.3347-1-cera@cera.cz |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Feb 1, 2017 at 12:31 AM, 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. > > v2 - Corrected the comment and condition check suggested by Suresh & Harsha > > 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 | 33 ++++++++++++++++++++++++----- > 1 file changed, 28 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..cd49a54c538d 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,27 @@ 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 the old programmed MAC as we successfully programmed > + * a new MAC > + */ > + 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 +4571,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 > Thanks Ivan. Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> Date: Wed, 1 Feb 2017 11:21:29 +0530 > On Wed, Feb 1, 2017 at 12:31 AM, 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. >> >> v2 - Corrected the comment and condition check suggested by Suresh & Harsha >> >> 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> ... > Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> Applied, thanks everyone.
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index 1a7f8ad7b9c6..cd49a54c538d 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,27 @@ 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 the old programmed MAC as we successfully programmed + * a new MAC + */ + 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 +4571,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. v2 - Corrected the comment and condition check suggested by Suresh & Harsha 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 | 33 ++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-)