* [Intel-wired-lan] [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands
@ 2024-05-15 14:22 ` Karthik Sundaravel
0 siblings, 0 replies; 10+ messages in thread
From: Karthik Sundaravel @ 2024-05-15 14:22 UTC (permalink / raw)
To: jesse.brandeburg, wojciech.drewek, sumang, jacob.e.keller,
anthony.l.nguyen, davem, edumazet, kuba, pabeni, intel-wired-lan,
netdev, linux-kernel, horms
Cc: pmenzel, aharivel, jiri, cfontain, vchundur, ksundara,
michal.swiatkowski, rjarry
Dear Maintainers,
Thanks for the review and suggestions for my patch.
v8 -> v9
--------
- Rebasing against dev-queue branch of next-queue tree
v7 -> v8
--------
- Added const keyword for the parameter ``mac`` in ice_set_vf_fn_mac()
v6 -> v7
--------
- Addressed Smatch and checkpatch issues
v5 -> v6
--------
- Changed data type of vf_id to u16
- Used container_of(port, struct ice_vf, devlink_port) to
get the vf instead of ice_get_vf_by_id()/ice_put_vf()
v4 -> v5
--------
- Cloned ice_set_vf_mac() to ice_set_vf_fn_mac() so that the
parameter ice_pf is used instead of net_device of vf
- removed redundant error handling
v3 -> v4
--------
- Released the vf device by calling ice_put_vf()
v2 -> v3
--------
- Fill the extack message instead of dev_err()
v1 -> v2
--------
- called ice_set_vf_mac() directly from the devlink port function
handlers.
RFC -> v1
---------
- Add the function handlers to set and get the HW address for the
VF representor ports.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands @ 2024-05-15 14:22 ` Karthik Sundaravel 0 siblings, 0 replies; 10+ messages in thread From: Karthik Sundaravel @ 2024-05-15 14:22 UTC (permalink / raw) To: jesse.brandeburg, wojciech.drewek, sumang, jacob.e.keller, anthony.l.nguyen, davem, edumazet, kuba, pabeni, intel-wired-lan, netdev, linux-kernel, horms Cc: pmenzel, jiri, michal.swiatkowski, rjarry, aharivel, vchundur, ksundara, cfontain Dear Maintainers, Thanks for the review and suggestions for my patch. v8 -> v9 -------- - Rebasing against dev-queue branch of next-queue tree v7 -> v8 -------- - Added const keyword for the parameter ``mac`` in ice_set_vf_fn_mac() v6 -> v7 -------- - Addressed Smatch and checkpatch issues v5 -> v6 -------- - Changed data type of vf_id to u16 - Used container_of(port, struct ice_vf, devlink_port) to get the vf instead of ice_get_vf_by_id()/ice_put_vf() v4 -> v5 -------- - Cloned ice_set_vf_mac() to ice_set_vf_fn_mac() so that the parameter ice_pf is used instead of net_device of vf - removed redundant error handling v3 -> v4 -------- - Released the vf device by calling ice_put_vf() v2 -> v3 -------- - Fill the extack message instead of dev_err() v1 -> v2 -------- - called ice_set_vf_mac() directly from the devlink port function handlers. RFC -> v1 --------- - Add the function handlers to set and get the HW address for the VF representor ports. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-wired-lan] [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands 2024-05-15 14:22 ` Karthik Sundaravel @ 2024-05-15 14:22 ` Karthik Sundaravel -1 siblings, 0 replies; 10+ messages in thread From: Karthik Sundaravel @ 2024-05-15 14:22 UTC (permalink / raw) To: jesse.brandeburg, wojciech.drewek, sumang, jacob.e.keller, anthony.l.nguyen, davem, edumazet, kuba, pabeni, intel-wired-lan, netdev, linux-kernel, horms Cc: pmenzel, aharivel, jiri, cfontain, vchundur, ksundara, michal.swiatkowski, rjarry 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 <ksundara@redhat.com> --- .../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); +} + +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; +} + /** * ice_set_vf_mac * @netdev: network interface device structure diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h index 8f22313474d6..2ecbacfa8cfc 100644 --- a/drivers/net/ethernet/intel/ice/ice_sriov.h +++ b/drivers/net/ethernet/intel/ice/ice_sriov.h @@ -28,6 +28,7 @@ #ifdef CONFIG_PCI_IOV void ice_process_vflr_event(struct ice_pf *pf); int ice_sriov_configure(struct pci_dev *pdev, int num_vfs); +int ice_set_vf_fn_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac); int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac); int ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi); @@ -80,6 +81,13 @@ ice_sriov_configure(struct pci_dev __always_unused *pdev, return -EOPNOTSUPP; } +static inline int +ice_set_vf_fn_mac(struct ice_pf __always_unused *pf, + u16 __always_unused vf_id, const u8 __always_unused *mac) +{ + return -EOPNOTSUPP; +} + static inline int ice_set_vf_mac(struct net_device __always_unused *netdev, int __always_unused vf_id, u8 __always_unused *mac) -- 2.39.3 (Apple Git-146) ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands @ 2024-05-15 14:22 ` Karthik Sundaravel 0 siblings, 0 replies; 10+ messages in thread From: Karthik Sundaravel @ 2024-05-15 14:22 UTC (permalink / raw) To: jesse.brandeburg, wojciech.drewek, sumang, jacob.e.keller, anthony.l.nguyen, davem, edumazet, kuba, pabeni, intel-wired-lan, netdev, linux-kernel, horms Cc: pmenzel, jiri, michal.swiatkowski, rjarry, aharivel, vchundur, ksundara, cfontain 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 <ksundara@redhat.com> --- .../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); +} + +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; +} + /** * ice_set_vf_mac * @netdev: network interface device structure diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h index 8f22313474d6..2ecbacfa8cfc 100644 --- a/drivers/net/ethernet/intel/ice/ice_sriov.h +++ b/drivers/net/ethernet/intel/ice/ice_sriov.h @@ -28,6 +28,7 @@ #ifdef CONFIG_PCI_IOV void ice_process_vflr_event(struct ice_pf *pf); int ice_sriov_configure(struct pci_dev *pdev, int num_vfs); +int ice_set_vf_fn_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac); int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac); int ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi); @@ -80,6 +81,13 @@ ice_sriov_configure(struct pci_dev __always_unused *pdev, return -EOPNOTSUPP; } +static inline int +ice_set_vf_fn_mac(struct ice_pf __always_unused *pf, + u16 __always_unused vf_id, const u8 __always_unused *mac) +{ + return -EOPNOTSUPP; +} + static inline int ice_set_vf_mac(struct net_device __always_unused *netdev, int __always_unused vf_id, u8 __always_unused *mac) -- 2.39.3 (Apple Git-146) ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands 2024-05-15 14:22 ` Karthik Sundaravel @ 2024-05-15 16:52 ` Brett Creeley -1 siblings, 0 replies; 10+ messages in thread From: Brett Creeley @ 2024-05-15 16:52 UTC (permalink / raw) To: Karthik Sundaravel, jesse.brandeburg, wojciech.drewek, sumang, jacob.e.keller, anthony.l.nguyen, davem, edumazet, kuba, pabeni, intel-wired-lan, netdev, linux-kernel, horms Cc: pmenzel, aharivel, jiri, cfontain, vchundur, michal.swiatkowski, rjarry 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 <ksundara@redhat.com> > --- > .../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? > + > /** > * ice_set_vf_mac > * @netdev: network interface device structure > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h > index 8f22313474d6..2ecbacfa8cfc 100644 > --- a/drivers/net/ethernet/intel/ice/ice_sriov.h > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.h > @@ -28,6 +28,7 @@ > #ifdef CONFIG_PCI_IOV > void ice_process_vflr_event(struct ice_pf *pf); > int ice_sriov_configure(struct pci_dev *pdev, int num_vfs); > +int ice_set_vf_fn_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac); > int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac); > int > ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi); > @@ -80,6 +81,13 @@ ice_sriov_configure(struct pci_dev __always_unused *pdev, > return -EOPNOTSUPP; > } > > +static inline int > +ice_set_vf_fn_mac(struct ice_pf __always_unused *pf, > + u16 __always_unused vf_id, const u8 __always_unused *mac) > +{ > + return -EOPNOTSUPP; > +} > + > static inline int > ice_set_vf_mac(struct net_device __always_unused *netdev, > int __always_unused vf_id, u8 __always_unused *mac) > -- > 2.39.3 (Apple Git-146) > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands @ 2024-05-15 16:52 ` Brett Creeley 0 siblings, 0 replies; 10+ messages in thread From: Brett Creeley @ 2024-05-15 16:52 UTC (permalink / raw) To: Karthik Sundaravel, jesse.brandeburg, wojciech.drewek, sumang, jacob.e.keller, anthony.l.nguyen, davem, edumazet, kuba, pabeni, intel-wired-lan, netdev, linux-kernel, horms Cc: pmenzel, jiri, michal.swiatkowski, rjarry, aharivel, vchundur, cfontain 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 <ksundara@redhat.com> > --- > .../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? > + > /** > * ice_set_vf_mac > * @netdev: network interface device structure > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.h b/drivers/net/ethernet/intel/ice/ice_sriov.h > index 8f22313474d6..2ecbacfa8cfc 100644 > --- a/drivers/net/ethernet/intel/ice/ice_sriov.h > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.h > @@ -28,6 +28,7 @@ > #ifdef CONFIG_PCI_IOV > void ice_process_vflr_event(struct ice_pf *pf); > int ice_sriov_configure(struct pci_dev *pdev, int num_vfs); > +int ice_set_vf_fn_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac); > int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac); > int > ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi); > @@ -80,6 +81,13 @@ ice_sriov_configure(struct pci_dev __always_unused *pdev, > return -EOPNOTSUPP; > } > > +static inline int > +ice_set_vf_fn_mac(struct ice_pf __always_unused *pf, > + u16 __always_unused vf_id, const u8 __always_unused *mac) > +{ > + return -EOPNOTSUPP; > +} > + > static inline int > ice_set_vf_mac(struct net_device __always_unused *netdev, > int __always_unused vf_id, u8 __always_unused *mac) > -- > 2.39.3 (Apple Git-146) > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands 2024-05-15 16:52 ` Brett Creeley @ 2024-05-15 21:21 ` Keller, Jacob E -1 siblings, 0 replies; 10+ messages in thread From: Keller, Jacob E @ 2024-05-15 21:21 UTC (permalink / raw) To: Brett Creeley, Karthik Sundaravel, Brandeburg, Jesse, Drewek, Wojciech, sumang@marvell.com, Nguyen, Anthony L, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, horms@kernel.org Cc: pmenzel@molgen.mpg.de, aharivel@redhat.com, jiri@resnulli.us, cfontain@redhat.com, vchundur@redhat.com, michal.swiatkowski@linux.intel.com, rjarry@redhat.com > -----Original Message----- > From: Brett Creeley <bcreeley@amd.com> > Sent: Wednesday, May 15, 2024 9:53 AM > To: Karthik Sundaravel <ksundara@redhat.com>; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; Drewek, Wojciech <wojciech.drewek@intel.com>; > sumang@marvell.com; Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, > Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; intel-wired- > lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > horms@kernel.org > Cc: pmenzel@molgen.mpg.de; jiri@resnulli.us; > michal.swiatkowski@linux.intel.com; rjarry@redhat.com; aharivel@redhat.com; > vchundur@redhat.com; cfontain@redhat.com > 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 <ksundara@redhat.com> > > --- > > .../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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands @ 2024-05-15 21:21 ` Keller, Jacob E 0 siblings, 0 replies; 10+ messages in thread From: Keller, Jacob E @ 2024-05-15 21:21 UTC (permalink / raw) To: Brett Creeley, Karthik Sundaravel, Brandeburg, Jesse, Drewek, Wojciech, sumang@marvell.com, Nguyen, Anthony L, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, horms@kernel.org Cc: pmenzel@molgen.mpg.de, jiri@resnulli.us, michal.swiatkowski@linux.intel.com, rjarry@redhat.com, aharivel@redhat.com, vchundur@redhat.com, cfontain@redhat.com > -----Original Message----- > From: Brett Creeley <bcreeley@amd.com> > Sent: Wednesday, May 15, 2024 9:53 AM > To: Karthik Sundaravel <ksundara@redhat.com>; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; Drewek, Wojciech <wojciech.drewek@intel.com>; > sumang@marvell.com; Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, > Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; intel-wired- > lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > horms@kernel.org > Cc: pmenzel@molgen.mpg.de; jiri@resnulli.us; > michal.swiatkowski@linux.intel.com; rjarry@redhat.com; aharivel@redhat.com; > vchundur@redhat.com; cfontain@redhat.com > 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 <ksundara@redhat.com> > > --- > > .../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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands 2024-05-15 21:21 ` Keller, Jacob E @ 2024-05-16 15:01 ` Brett Creeley -1 siblings, 0 replies; 10+ messages in thread From: Brett Creeley @ 2024-05-16 15:01 UTC (permalink / raw) To: Keller, Jacob E, Karthik Sundaravel, Brandeburg, Jesse, Drewek, Wojciech, sumang@marvell.com, Nguyen, Anthony L, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, horms@kernel.org Cc: pmenzel@molgen.mpg.de, aharivel@redhat.com, jiri@resnulli.us, cfontain@redhat.com, vchundur@redhat.com, michal.swiatkowski@linux.intel.com, rjarry@redhat.com On 5/15/2024 2:21 PM, Keller, Jacob E wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > >> -----Original Message----- >> From: Brett Creeley <bcreeley@amd.com> >> Sent: Wednesday, May 15, 2024 9:53 AM >> To: Karthik Sundaravel <ksundara@redhat.com>; Brandeburg, Jesse >> <jesse.brandeburg@intel.com>; Drewek, Wojciech <wojciech.drewek@intel.com>; >> sumang@marvell.com; Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, >> Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; intel-wired- >> lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> horms@kernel.org >> Cc: pmenzel@molgen.mpg.de; jiri@resnulli.us; >> michal.swiatkowski@linux.intel.com; rjarry@redhat.com; aharivel@redhat.com; >> vchundur@redhat.com; cfontain@redhat.com >> 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 <ksundara@redhat.com> >>> --- >>> .../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? Yeah that's probably the best approach. Maybe someting like the following makes the most sense? __ice_set_vf_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac); For the netdev_op it's: return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac); For the devlink op it's: pf = devlink_priv(devlink); pci_vf = &attrs->pci_vf; vf_id = pci_vf->vf; return __ice_set_vf_mac(pf, vf_id, mac); Regardless duplicating the entire function is not the correct approach. Thanks, Brett ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands @ 2024-05-16 15:01 ` Brett Creeley 0 siblings, 0 replies; 10+ messages in thread From: Brett Creeley @ 2024-05-16 15:01 UTC (permalink / raw) To: Keller, Jacob E, Karthik Sundaravel, Brandeburg, Jesse, Drewek, Wojciech, sumang@marvell.com, Nguyen, Anthony L, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, horms@kernel.org Cc: pmenzel@molgen.mpg.de, jiri@resnulli.us, michal.swiatkowski@linux.intel.com, rjarry@redhat.com, aharivel@redhat.com, vchundur@redhat.com, cfontain@redhat.com On 5/15/2024 2:21 PM, Keller, Jacob E wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > >> -----Original Message----- >> From: Brett Creeley <bcreeley@amd.com> >> Sent: Wednesday, May 15, 2024 9:53 AM >> To: Karthik Sundaravel <ksundara@redhat.com>; Brandeburg, Jesse >> <jesse.brandeburg@intel.com>; Drewek, Wojciech <wojciech.drewek@intel.com>; >> sumang@marvell.com; Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, >> Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; intel-wired- >> lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> horms@kernel.org >> Cc: pmenzel@molgen.mpg.de; jiri@resnulli.us; >> michal.swiatkowski@linux.intel.com; rjarry@redhat.com; aharivel@redhat.com; >> vchundur@redhat.com; cfontain@redhat.com >> 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 <ksundara@redhat.com> >>> --- >>> .../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? Yeah that's probably the best approach. Maybe someting like the following makes the most sense? __ice_set_vf_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac); For the netdev_op it's: return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac); For the devlink op it's: pf = devlink_priv(devlink); pci_vf = &attrs->pci_vf; vf_id = pci_vf->vf; return __ice_set_vf_mac(pf, vf_id, mac); Regardless duplicating the entire function is not the correct approach. Thanks, Brett ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-16 15:01 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-15 14:22 [Intel-wired-lan] [PATCH iwl-next v9] ice: Add get/set hw address for VFs using devlink commands Karthik Sundaravel 2024-05-15 14:22 ` Karthik Sundaravel 2024-05-15 14:22 ` [Intel-wired-lan] " Karthik Sundaravel 2024-05-15 14:22 ` Karthik Sundaravel 2024-05-15 16:52 ` [Intel-wired-lan] " Brett Creeley 2024-05-15 16:52 ` Brett Creeley 2024-05-15 21:21 ` [Intel-wired-lan] " Keller, Jacob E 2024-05-15 21:21 ` Keller, Jacob E 2024-05-16 15:01 ` [Intel-wired-lan] " Brett Creeley 2024-05-16 15:01 ` Brett Creeley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.