| Message ID | 1456388374-1440-2-git-send-email-michael.chan@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Michael Chan <michael.chan@broadcom.com> Date: Thu, 25 Feb 2016 03:19:25 -0500 > From: Jeffrey Huang <huangjw@broadcom.com> > > Allow the VF to setup its own MAC address if the PF has not administratively > set it for the VF. If the perm_mac_address returned by firmware is all > zeros, that means the PF has not set up the MAC address for the VF and > we should store it. This will allow the VF to change the MAC address using > ndo_set_mac_address() when it sees that the stored permanent MAC address > is all zeros. > > Signed-off-by: Jeffrey Huang <huangjw@broadcom.com> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> What triggers that ->ndo_set_mac_address() call in the VF? You cannot register an ethernet netdev with an invalid ethernet address, you must use a random one if you don't have a valid one available.
On Thu, Feb 25, 2016 at 8:31 AM, David Miller <davem@davemloft.net> wrote: > From: Michael Chan <michael.chan@broadcom.com> > Date: Thu, 25 Feb 2016 03:19:25 -0500 > >> From: Jeffrey Huang <huangjw@broadcom.com> >> >> Allow the VF to setup its own MAC address if the PF has not administratively >> set it for the VF. If the perm_mac_address returned by firmware is all >> zeros, that means the PF has not set up the MAC address for the VF and >> we should store it. This will allow the VF to change the MAC address using >> ndo_set_mac_address() when it sees that the stored permanent MAC address >> is all zeros. >> >> Signed-off-by: Jeffrey Huang <huangjw@broadcom.com> >> Signed-off-by: Michael Chan <michael.chan@broadcom.com> > > What triggers that ->ndo_set_mac_address() call in the VF? ip link set eth0 address ... done on the VF. > > You cannot register an ethernet netdev with an invalid ethernet address, > you must use a random one if you don't have a valid one available. We are not registering an invalid MAC address. We are just storing it in the driver's VF data structure. There are 2 cases: 1. VF comes up and the MAC address from firmware is 0. The VF will generate random MAC. The stored MAC address in the VF datastructure is 0 so that ip set link eth0 address is allowed on the VF. 2. PF sets the MAC address for the VF using ip link set eth0 vf mac .... The firmware will now provide the PF administered MAC address to the VF and random MAC is no longer generated. This valid MAC is now stored and ip link set eth0 address ... on the VF is no longer allowed. This patch is to make case 2 work properly. I will change the commit message to make it more clear and I will fix the parameter formatting in the other patch. Thanks.
From: Michael Chan <michael.chan@broadcom.com> Date: Thu, 25 Feb 2016 08:57:01 -0800 > We are not registering an invalid MAC address. We are just storing it in the > driver's VF data structure. There are 2 cases: > > 1. VF comes up and the MAC address from firmware is 0. The VF will > generate random MAC. The stored MAC address in the VF datastructure is > 0 so that ip set link eth0 address is allowed on the VF. Who looks at this 0 MAC address in the "VF datastructure", the driver? Why does there need to be a 0 MAC address there to allow ->ndo_set_mac_address() to succeed on the VF at all? This MAC address management between VFs and PFs looks unnecessarily convoluted and complicated. I'd hate to have to actually be a user configuring this stuff.
On Thu, Feb 25, 2016 at 9:09 AM, David Miller <davem@davemloft.net> wrote: > From: Michael Chan <michael.chan@broadcom.com> > Date: Thu, 25 Feb 2016 08:57:01 -0800 > >> We are not registering an invalid MAC address. We are just storing it in the >> driver's VF data structure. There are 2 cases: >> >> 1. VF comes up and the MAC address from firmware is 0. The VF will >> generate random MAC. The stored MAC address in the VF datastructure is >> 0 so that ip set link eth0 address is allowed on the VF. > > Who looks at this 0 MAC address in the "VF datastructure", the driver? the VF driver. > > Why does there need to be a 0 MAC address there to allow > ->ndo_set_mac_address() to succeed on the VF at all? 0 means that the PF has not set it. If the PF had set it, the datastructure would contain the MAC address set by the PF. The idea is that if the PF has administered a MAC address, we won't allow the VF to change it using ndo. > > This MAC address management between VFs and PFs looks unnecessarily > convoluted and complicated. I'd hate to have to actually be a user > configuring this stuff. > I agree it is complicated. The default, if nobody does anything, is random MAC. But random MAC has disadvantages, so we allow some options for the PF or VF users to change it.
From: Michael Chan <michael.chan@broadcom.com> Date: Thu, 25 Feb 2016 09:15:39 -0800 > I agree it is complicated. The default, if nobody does anything, is random > MAC. But random MAC has disadvantages, so we allow some options > for the PF or VF users to change it. Ok, thanks for explaining.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c index c1cc83d..bb2b376 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c @@ -809,13 +809,12 @@ void bnxt_update_vf_mac(struct bnxt *bp) if (_hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT)) goto update_vf_mac_exit; - if (!is_valid_ether_addr(resp->perm_mac_address)) - goto update_vf_mac_exit; - if (!ether_addr_equal(resp->perm_mac_address, bp->vf.mac_addr)) memcpy(bp->vf.mac_addr, resp->perm_mac_address, ETH_ALEN); - /* overwrite netdev dev_adr with admin VF MAC */ - memcpy(bp->dev->dev_addr, bp->vf.mac_addr, ETH_ALEN); + + /* overwrite netdev dev_addr with admin VF MAC */ + if (is_valid_ether_addr(bp->vf.mac_addr)) + memcpy(bp->dev->dev_addr, bp->vf.mac_addr, ETH_ALEN); update_vf_mac_exit: mutex_unlock(&bp->hwrm_cmd_lock); }