> -----Original Message----- > From: Brett Creeley <[email protected]> > Sent: Wednesday, May 15, 2024 9:53 AM > To: Karthik Sundaravel <[email protected]>; Brandeburg, Jesse > <[email protected]>; Drewek, Wojciech <[email protected]>; > [email protected]; Keller, Jacob E <[email protected]>; Nguyen, > Anthony L <[email protected]>; [email protected]; > [email protected]; [email protected]; [email protected]; intel-wired- > [email protected]; [email protected]; [email protected]; > [email protected] > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected] > Subject: Re: [PATCH iwl-next v9] ice: Add get/set hw address for VFs using > devlink > commands > > > > On 5/15/2024 7:22 AM, Karthik Sundaravel wrote: > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > > > > Changing the MAC address of the VFs is currently unsupported via devlink. > > Add the function handlers to set and get the HW address for the VFs. > > > > Signed-off-by: Karthik Sundaravel <[email protected]> > > --- > > .../ethernet/intel/ice/devlink/devlink_port.c | 59 +++++++++++++++++- > > drivers/net/ethernet/intel/ice/ice_sriov.c | 62 +++++++++++++++++++ > > drivers/net/ethernet/intel/ice/ice_sriov.h | 8 +++ > > 3 files changed, 128 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > > index c9fbeebf7fb9..6ff7cba3f630 100644 > > --- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > > @@ -372,6 +372,62 @@ void ice_devlink_destroy_pf_port(struct ice_pf *pf) > > devl_port_unregister(&pf->devlink_port); > > } > > > > +/** > > + * ice_devlink_port_get_vf_fn_mac - .port_fn_hw_addr_get devlink handler > > + * @port: devlink port structure > > + * @hw_addr: MAC address of the port > > + * @hw_addr_len: length of MAC address > > + * @extack: extended netdev ack structure > > + * > > + * Callback for the devlink .port_fn_hw_addr_get operation > > + * Return: zero on success or an error code on failure. > > + */ > > +static int ice_devlink_port_get_vf_fn_mac(struct devlink_port *port, > > + u8 *hw_addr, int *hw_addr_len, > > + struct netlink_ext_ack *extack) > > +{ > > + struct ice_vf *vf = container_of(port, struct ice_vf, devlink_port); > > + > > + ether_addr_copy(hw_addr, vf->dev_lan_addr); > > + *hw_addr_len = ETH_ALEN; > > + > > + return 0; > > +} > > + > > +/** > > + * ice_devlink_port_set_vf_fn_mac - .port_fn_hw_addr_set devlink handler > > + * @port: devlink port structure > > + * @hw_addr: MAC address of the port > > + * @hw_addr_len: length of MAC address > > + * @extack: extended netdev ack structure > > + * > > + * Callback for the devlink .port_fn_hw_addr_set operation > > + * Return: zero on success or an error code on failure. > > + */ > > +static int ice_devlink_port_set_vf_fn_mac(struct devlink_port *port, > > + const u8 *hw_addr, > > + int hw_addr_len, > > + struct netlink_ext_ack *extack) > > + > > +{ > > + struct devlink_port_attrs *attrs = &port->attrs; > > + struct devlink_port_pci_vf_attrs *pci_vf; > > + struct devlink *devlink = port->devlink; > > + struct ice_pf *pf; > > + u16 vf_id; > > + > > + pf = devlink_priv(devlink); > > + pci_vf = &attrs->pci_vf; > > + vf_id = pci_vf->vf; > > + > > + return ice_set_vf_fn_mac(pf, vf_id, hw_addr); > > Instead of creating a duplicate function, it seems like you can do the > following instead: > > pf = devlink_priv(devlink); > vsi = ice_get_main_vsi(pf); > if (!vsi) > return -ENODEV; > > [...] > > return ice_set_vf_mac(vsi->netdev, vf_id, hw_addr); > > > +} > > + > > +static const struct devlink_port_ops ice_devlink_vf_port_ops = { > > + .port_fn_hw_addr_get = ice_devlink_port_get_vf_fn_mac, > > + .port_fn_hw_addr_set = ice_devlink_port_set_vf_fn_mac, > > +}; > > + > > /** > > * ice_devlink_create_vf_port - Create a devlink port for this VF > > * @vf: the VF to create a port for > > @@ -407,7 +463,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf) > > devlink_port_attrs_set(devlink_port, &attrs); > > devlink = priv_to_devlink(pf); > > > > - err = devl_port_register(devlink, devlink_port, vsi->idx); > > + err = devl_port_register_with_ops(devlink, devlink_port, vsi->idx, > > + &ice_devlink_vf_port_ops); > > if (err) { > > dev_err(dev, "Failed to create devlink port for VF %d, > > error %d\n", > > vf->vf_id, err); > > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c > b/drivers/net/ethernet/intel/ice/ice_sriov.c > > index 067712f4923f..dc40be741be0 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_sriov.c > > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c > > @@ -1415,6 +1415,68 @@ ice_get_vf_cfg(struct net_device *netdev, int vf_id, > struct ifla_vf_info *ivi) > > return ret; > > } > > > > +/** > > + * ice_set_vf_fn_mac > > + * @pf: PF to be configure > > + * @vf_id: VF identifier > > + * @mac: MAC address > > + * > > + * program VF MAC address > > + */ > > +int ice_set_vf_fn_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac) > > +{ > > + struct device *dev; > > + struct ice_vf *vf; > > + int ret; > > + > > + dev = ice_pf_to_dev(pf); > > + if (is_multicast_ether_addr(mac)) { > > + dev_err(dev, "%pM not a valid unicast address\n", mac); > > + return -EINVAL; > > + } > > + > > + vf = ice_get_vf_by_id(pf, vf_id); > > + if (!vf) > > + return -EINVAL; > > + > > + /* nothing left to do, unicast MAC already set */ > > + if (ether_addr_equal(vf->dev_lan_addr, mac) && > > + ether_addr_equal(vf->hw_lan_addr, mac)) { > > + ret = 0; > > + goto out_put_vf; > > + } > > + > > + ret = ice_check_vf_ready_for_cfg(vf); > > + if (ret) > > + goto out_put_vf; > > + > > + mutex_lock(&vf->cfg_lock); > > + > > + /* VF is notified of its new MAC via the PF's response to the > > + * VIRTCHNL_OP_GET_VF_RESOURCES message after the VF has been reset > > + */ > > + ether_addr_copy(vf->dev_lan_addr, mac); > > + ether_addr_copy(vf->hw_lan_addr, mac); > > + if (is_zero_ether_addr(mac)) { > > + /* VF will send VIRTCHNL_OP_ADD_ETH_ADDR message with its > > MAC */ > > + vf->pf_set_mac = false; > > + dev_info(dev, "Removing MAC on VF %d. VF driver will be > reinitialized\n", > > + vf->vf_id); > > + } else { > > + /* PF will add MAC rule for the VF */ > > + vf->pf_set_mac = true; > > + dev_info(dev, "Setting MAC %pM on VF %d. VF driver will be > reinitialized\n", > > + mac, vf_id); > > + } > > + > > + ice_reset_vf(vf, ICE_VF_RESET_NOTIFY); > > + mutex_unlock(&vf->cfg_lock); > > + > > +out_put_vf: > > + ice_put_vf(vf); > > + return ret; > > +} > > This is almost an exact copy of ice_set_vf_mac(). The only difference > being the function arguments. > > Was there any reason to not use ice_set_vf_mac()? Why can't you pass the > PF's netdev? >
Or, refactor the set_vf_mac() into a function into a common one which takes a common set of arguments that can be derived in both the netdev op and devlink flow?
