| Message ID | 20241202183219.2312114-1-srasheed@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,RESEND] octeon_ep: add ndo ops for VFs in PF driver | expand |
On Mon, 2 Dec 2024 10:32:18 -0800 Shinas Rasheed wrote: > These APIs are needed to support applications that use netlink to get VF > information from a PF driver. > + ivi->vf = vf; > + ether_addr_copy(ivi->mac, oct->vf_info[vf].mac_addr); > + ivi->spoofchk = true; > + ivi->linkstate = IFLA_VF_LINK_STATE_ENABLE; > + ivi->trusted = oct->vf_info[vf].trusted; > + ivi->max_tx_rate = 10000; I still feel like this is using the rate limiting API to report fixed link speed, which is tangential to rate limiting.. Unless the user can set the max_tx_rate why would they want to know what the limit is at? Ideally reporting rate limit would be done in a patch set which supports setting it. > + ivi->min_tx_rate = 0; No need to set this to 0, AFAIR core initializes to 0. > /** > @@ -1560,9 +1601,12 @@ static void octep_remove(struct pci_dev *pdev) > static int octep_sriov_enable(struct octep_device *oct, int num_vfs) > { > struct pci_dev *pdev = oct->pdev; > - int err; > + int i, err; > > CFG_GET_ACTIVE_VFS(oct->conf) = num_vfs; > + for (i = 0; i < num_vfs; i++) > + oct->vf_info[i].trusted = false; I don't see it ever getting set to true, why track it if it's always false? > err = pci_enable_sriov(pdev, num_vfs); > if (err) { > dev_warn(&pdev->dev, "Failed to enable SRIOV err=%d\n", err);
Hi Jakub, > On Mon, 2 Dec 2024 10:32:18 -0800 Shinas Rasheed wrote: > > These APIs are needed to support applications that use netlink to get VF > > information from a PF driver. > > > + ivi->vf = vf; > > + ether_addr_copy(ivi->mac, oct->vf_info[vf].mac_addr); > > + ivi->spoofchk = true; > > + ivi->linkstate = IFLA_VF_LINK_STATE_ENABLE; > > + ivi->trusted = oct->vf_info[vf].trusted; > > + ivi->max_tx_rate = 10000; > > I still feel like this is using the rate limiting API to report fixed > link speed, which is tangential to rate limiting.. > > Unless the user can set the max_tx_rate why would they want to know > what the limit is at? Ideally reporting rate limit would be done > in a patch set which supports setting it. > Ack > > + ivi->min_tx_rate = 0; > > No need to set this to 0, AFAIR core initializes to 0. Ack > > /** > > @@ -1560,9 +1601,12 @@ static void octep_remove(struct pci_dev *pdev) > > static int octep_sriov_enable(struct octep_device *oct, int num_vfs) > > { > > struct pci_dev *pdev = oct->pdev; > > - int err; > > + int i, err; > > > > CFG_GET_ACTIVE_VFS(oct->conf) = num_vfs; > > + for (i = 0; i < num_vfs; i++) > > + oct->vf_info[i].trusted = false; > > I don't see it ever getting set to true, why track it if it's always > false? > In case we need to support the api in the future, just added the corresponding data structure to track it. Perhaps if you think that’s warranted only 'when' we support it then, maybe I can remove it. The data structure was used to check for 'trusted' when vf tries to set its mac. > > err = pci_enable_sriov(pdev, num_vfs); > > if (err) { > > dev_warn(&pdev->dev, "Failed to enable SRIOV err=%d\n", > err);
On Thu, 5 Dec 2024 15:47:29 +0000 Shinas Rasheed wrote: > > I don't see it ever getting set to true, why track it if it's always > > false? > > In case we need to support the api in the future, just added the > corresponding data structure to track it. Perhaps if you think that’s > warranted only 'when' we support it then, maybe I can remove it. The > data structure was used to check for 'trusted' when vf tries to set > its mac. Yes, please remove it. We try to limit merging code that's of no immediate use.
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c index 549436efc204..226a44823401 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c @@ -1137,6 +1137,45 @@ static int octep_set_features(struct net_device *dev, netdev_features_t features return err; } +static int octep_get_vf_config(struct net_device *dev, int vf, + struct ifla_vf_info *ivi) +{ + struct octep_device *oct = netdev_priv(dev); + + ivi->vf = vf; + ether_addr_copy(ivi->mac, oct->vf_info[vf].mac_addr); + ivi->spoofchk = true; + ivi->linkstate = IFLA_VF_LINK_STATE_ENABLE; + ivi->trusted = oct->vf_info[vf].trusted; + ivi->max_tx_rate = 10000; + ivi->min_tx_rate = 0; + + return 0; +} + +static int octep_set_vf_mac(struct net_device *dev, int vf, u8 *mac) +{ + struct octep_device *oct = netdev_priv(dev); + int err; + + if (!is_valid_ether_addr(mac)) { + dev_err(&oct->pdev->dev, "Invalid MAC Address %pM\n", mac); + return -EADDRNOTAVAIL; + } + + dev_dbg(&oct->pdev->dev, "set vf-%d mac to %pM\n", vf, mac); + ether_addr_copy(oct->vf_info[vf].mac_addr, mac); + oct->vf_info[vf].flags |= OCTEON_PFVF_FLAG_MAC_SET_BY_PF; + + err = octep_ctrl_net_set_mac_addr(oct, vf, mac, true); + if (err) + dev_err(&oct->pdev->dev, + "Set VF%d MAC address failed via host control Mbox\n", + vf); + + return err; +} + static const struct net_device_ops octep_netdev_ops = { .ndo_open = octep_open, .ndo_stop = octep_stop, @@ -1146,6 +1185,8 @@ static const struct net_device_ops octep_netdev_ops = { .ndo_set_mac_address = octep_set_mac, .ndo_change_mtu = octep_change_mtu, .ndo_set_features = octep_set_features, + .ndo_get_vf_config = octep_get_vf_config, + .ndo_set_vf_mac = octep_set_vf_mac }; /** @@ -1560,9 +1601,12 @@ static void octep_remove(struct pci_dev *pdev) static int octep_sriov_enable(struct octep_device *oct, int num_vfs) { struct pci_dev *pdev = oct->pdev; - int err; + int i, err; CFG_GET_ACTIVE_VFS(oct->conf) = num_vfs; + for (i = 0; i < num_vfs; i++) + oct->vf_info[i].trusted = false; + err = pci_enable_sriov(pdev, num_vfs); if (err) { dev_warn(&pdev->dev, "Failed to enable SRIOV err=%d\n", err); diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h index fee59e0e0138..1c39833ffcc0 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h @@ -220,7 +220,9 @@ struct octep_iface_link_info { /* The Octeon VF device specific info data structure.*/ struct octep_pfvf_info { u8 mac_addr[ETH_ALEN]; + u32 flags; u32 mbox_version; + bool trusted; }; /* The Octeon device specific private data structure. diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c index e6eb98d70f3c..3024f6428838 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c @@ -156,12 +156,24 @@ static void octep_pfvf_set_mac_addr(struct octep_device *oct, u32 vf_id, { int err; + if ((oct->vf_info[vf_id].flags & OCTEON_PFVF_FLAG_MAC_SET_BY_PF) && + !oct->vf_info[vf_id].trusted) { + dev_err(&oct->pdev->dev, + "VF%d attempted to override administrative set MAC address\n", + vf_id); + rsp->s_set_mac.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; + return; + } + err = octep_ctrl_net_set_mac_addr(oct, vf_id, cmd.s_set_mac.mac_addr, true); if (err) { rsp->s_set_mac.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; - dev_err(&oct->pdev->dev, "Set VF MAC address failed via host control Mbox\n"); + dev_err(&oct->pdev->dev, "Set VF%d MAC address failed via host control Mbox\n", + vf_id); return; } + + ether_addr_copy(oct->vf_info[vf_id].mac_addr, cmd.s_set_mac.mac_addr); rsp->s_set_mac.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; } @@ -171,10 +183,18 @@ static void octep_pfvf_get_mac_addr(struct octep_device *oct, u32 vf_id, { int err; + if (oct->vf_info[vf_id].flags & OCTEON_PFVF_FLAG_MAC_SET_BY_PF) { + dev_dbg(&oct->pdev->dev, "VF%d MAC address set by PF\n", vf_id); + ether_addr_copy(rsp->s_set_mac.mac_addr, + oct->vf_info[vf_id].mac_addr); + rsp->s_set_mac.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; + return; + } err = octep_ctrl_net_get_mac_addr(oct, vf_id, rsp->s_set_mac.mac_addr); if (err) { rsp->s_set_mac.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; - dev_err(&oct->pdev->dev, "Get VF MAC address failed via host control Mbox\n"); + dev_err(&oct->pdev->dev, "Get VF%d MAC address failed via host control Mbox\n", + vf_id); return; } rsp->s_set_mac.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h index 0dc6eead292a..386a095a99bc 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h @@ -8,8 +8,6 @@ #ifndef _OCTEP_PFVF_MBOX_H_ #define _OCTEP_PFVF_MBOX_H_ -/* VF flags */ -#define OCTEON_PFVF_FLAG_MAC_SET_BY_PF BIT_ULL(0) /* PF has set VF MAC address */ #define OCTEON_SDP_16K_HW_FRS 16380UL #define OCTEON_SDP_64K_HW_FRS 65531UL @@ -23,6 +21,10 @@ enum octep_pfvf_mbox_version { #define OCTEP_PFVF_MBOX_VERSION_CURRENT OCTEP_PFVF_MBOX_VERSION_V2 +/* VF flags */ +/* PF has set VF MAC address */ +#define OCTEON_PFVF_FLAG_MAC_SET_BY_PF BIT(0) + enum octep_pfvf_mbox_opcode { OCTEP_PFVF_MBOX_CMD_VERSION, OCTEP_PFVF_MBOX_CMD_SET_MTU,
These APIs are needed to support applications that use netlink to get VF information from a PF driver. Signed-off-by: Shinas Rasheed <srasheed@marvell.com> --- V3: - Corrected line-wrap and space checkpatch errors - Set spoof check as true and vf trusted as false to be default vf configs V2: https://lore.kernel.org/all/PH0PR18MB47344F6BCCD1B629AC012065C7252@PH0PR18MB4734.namprd18.prod.outlook.com/ - Corrected typos, and removed not supported ndo_set_vf* hooks V1: https://lore.kernel.org/all/20241107121637.1117089-1-srasheed@marvell.com/ .../ethernet/marvell/octeon_ep/octep_main.c | 46 ++++++++++++++++++- .../ethernet/marvell/octeon_ep/octep_main.h | 2 + .../marvell/octeon_ep/octep_pfvf_mbox.c | 24 +++++++++- .../marvell/octeon_ep/octep_pfvf_mbox.h | 6 ++- 4 files changed, 73 insertions(+), 5 deletions(-)