[PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[Shinas Rasheed]

These APIs are needed to support applicaitons that use netlink to get VF
information from a PF driver.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
 .../ethernet/marvell/octeon_ep/octep_main.c   | 98 +++++++++++++++++++
 .../ethernet/marvell/octeon_ep/octep_main.h   |  1 +
 .../marvell/octeon_ep/octep_pfvf_mbox.c       | 22 ++++-
 .../marvell/octeon_ep/octep_pfvf_mbox.h       |  3 +
 4 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 549436efc204..129c68f5a4ba 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1137,6 +1137,95 @@ 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->vlan = 0;
+	ivi->qos = 0;
+	ivi->spoofchk = 0;
+	ivi->linkstate = IFLA_VF_LINK_STATE_ENABLE;
+	ivi->trusted = true;
+	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 i, 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);
+	for (i = 0; i < ETH_ALEN; i++)
+		oct->vf_info[vf].mac_addr[i] = mac[i];
+	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;
+	}
+
+	return 0;
+}
+
+static int octep_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos, __be16 vlan_proto)
+{
+	struct octep_device *oct = netdev_priv(dev);
+
+	dev_err(&oct->pdev->dev, "Setting VF VLAN not supported\n");
+	return 0;
+}
+
+static int octep_set_vf_spoofchk(struct net_device *dev, int vf, bool setting)
+{
+	struct octep_device *oct = netdev_priv(dev);
+
+	dev_err(&oct->pdev->dev, "Setting VF spoof check not supported\n");
+	return 0;
+}
+
+static int octep_set_vf_trust(struct net_device *dev, int vf, bool setting)
+{
+	struct octep_device *oct = netdev_priv(dev);
+
+	dev_err(&oct->pdev->dev, "Setting VF trust not supported\n");
+	return 0;
+}
+
+static int octep_set_vf_rate(struct net_device *dev, int vf, int min_tx_rate, int max_tx_rate)
+{
+	struct octep_device *oct = netdev_priv(dev);
+
+	dev_err(&oct->pdev->dev, "Setting VF rate not supported\n");
+	return 0;
+}
+
+static int octep_set_vf_link_state(struct net_device *dev, int vf, int link_state)
+{
+	struct octep_device *oct = netdev_priv(dev);
+
+	dev_err(&oct->pdev->dev, "Setting VF link state not supported\n");
+	return 0;
+}
+
+static int octep_get_vf_stats(struct net_device *dev, int vf, struct ifla_vf_stats *vf_stats)
+{
+	struct octep_device *oct = netdev_priv(dev);
+
+	dev_err(&oct->pdev->dev, "Getting VF stats not supported\n");
+	return 0;
+}
+
 static const struct net_device_ops octep_netdev_ops = {
 	.ndo_open                = octep_open,
 	.ndo_stop                = octep_stop,
@@ -1146,6 +1235,15 @@ 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,
+	/* for VFs */
+	.ndo_get_vf_config       = octep_get_vf_config,
+	.ndo_set_vf_mac          = octep_set_vf_mac,
+	.ndo_set_vf_vlan         = octep_set_vf_vlan,
+	.ndo_set_vf_spoofchk     = octep_set_vf_spoofchk,
+	.ndo_set_vf_trust        = octep_set_vf_trust,
+	.ndo_set_vf_rate         = octep_set_vf_rate,
+	.ndo_set_vf_link_state   = octep_set_vf_link_state,
+	.ndo_get_vf_stats        = octep_get_vf_stats,
 };
 
 /**
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
index fee59e0e0138..3b56916af468 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
@@ -220,6 +220,7 @@ 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;
 };
 
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..be21ad5ec75e 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,23 @@ 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) {
+		dev_err(&oct->pdev->dev,
+			"VF%d attampted 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 +182,17 @@ 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..339977c7131a 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h
@@ -23,6 +23,9 @@ enum octep_pfvf_mbox_version {
 
 #define OCTEP_PFVF_MBOX_VERSION_CURRENT	OCTEP_PFVF_MBOX_VERSION_V2
 
+/* VF flags */
+#define OCTEON_PFVF_FLAG_MAC_SET_BY_PF  BIT_ULL(0) /* PF has set VF MAC address */
+
 enum octep_pfvf_mbox_opcode {
 	OCTEP_PFVF_MBOX_CMD_VERSION,
 	OCTEP_PFVF_MBOX_CMD_SET_MTU,
-- 
2.25.1

Re: [PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[Simon Horman]

On Thu, Nov 07, 2024 at 04:16:37AM -0800, Shinas Rasheed wrote:
> These APIs are needed to support applicaitons that use netlink to get VF
> information from a PF driver.
> 
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>

...

> +static int octep_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos, __be16 vlan_proto)
> +{
> +	struct octep_device *oct = netdev_priv(dev);
> +
> +	dev_err(&oct->pdev->dev, "Setting VF VLAN not supported\n");
> +	return 0;
> +}

Hi Shinas,

Given that the operation is not supported I think it would
be more appropriate to return -EOPNOTSUPP. And moreover, given
that this is a noop, I think it would be yet more appropriate
not to implement it at all and let the core treat it as not supported.

Likewise for other NDOs implemented as noops in this patch.

...

RE: [EXTERNAL] Re: [PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[Shinas Rasheed]

Hi Simon,

On Thu, Nov 07, 2024 at 04:16:37AM -0800, Shinas Rasheed wrote:
>> These APIs are needed to support applicaitons that use netlink to get VF
>> information from a PF driver.
>> 
>> Signed-off-by: Shinas Rasheed <mailto:srasheed@marvell.com>
>
>...
>
>> +static int octep_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos, __be16 vlan_proto)
>> +{
>> +	struct octep_device *oct = netdev_priv(dev);
>> +
>> +	dev_err(&oct->pdev->dev, "Setting VF VLAN not supported\n");>
>> +	return 0;
>> +}
>
>Hi Shinas,
>
>Given that the operation is not supported I think it would
>be more appropriate to return -EOPNOTSUPP. And moreover, given
>that this is a noop, I think it would be yet more appropriate
>not to implement it at all and let the core treat it as not supported.
>
>Likewise for other NDOs implemented as noops in this patch.
>
>...

I think the problem was for some userspace programs and operators, sometimes returning -EOPNOTSUPP is a no-go. I think the idea was at least if the user saw these messages, they would know to
set it in some other way, and also not have the operator stop just because setting these values failed. Though I understand that’s counter-intuitive, but sometimes it lets operators work and go ahead. What do you think so?

Thanks for the comments!

Re: [PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 8945 bytes --]

Hi Shinas,

On Thu, Nov 7, 2024 at 5:47 PM Shinas Rasheed <srasheed@marvell.com> wrote:
>
> These APIs are needed to support applicaitons that use netlink to get VF
[d] typo in applications
> information from a PF driver.
>
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
>  .../ethernet/marvell/octeon_ep/octep_main.c   | 98 +++++++++++++++++++
>  .../ethernet/marvell/octeon_ep/octep_main.h   |  1 +
>  .../marvell/octeon_ep/octep_pfvf_mbox.c       | 22 ++++-
>  .../marvell/octeon_ep/octep_pfvf_mbox.h       |  3 +
>  4 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 549436efc204..129c68f5a4ba 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -1137,6 +1137,95 @@ 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->vlan = 0;
> +       ivi->qos = 0;
> +       ivi->spoofchk = 0;
> +       ivi->linkstate = IFLA_VF_LINK_STATE_ENABLE;
> +       ivi->trusted = true;
> +       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 i, 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);
> +       for (i = 0; i < ETH_ALEN; i++)
> +               oct->vf_info[vf].mac_addr[i] = mac[i];
[Kalesh] Is there any reason to no do a memcpy here or a ether_addr_copy()?
> +       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);
[d] looks like this return is unnecessary. You can "return rc" at the
end of the function.
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int octep_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos, __be16 vlan_proto)
> +{
> +       struct octep_device *oct = netdev_priv(dev);
> +
> +       dev_err(&oct->pdev->dev, "Setting VF VLAN not supported\n");
> +       return 0;
> +}
> +
> +static int octep_set_vf_spoofchk(struct net_device *dev, int vf, bool setting)
> +{
> +       struct octep_device *oct = netdev_priv(dev);
> +
> +       dev_err(&oct->pdev->dev, "Setting VF spoof check not supported\n");
> +       return 0;
> +}
> +
> +static int octep_set_vf_trust(struct net_device *dev, int vf, bool setting)
> +{
> +       struct octep_device *oct = netdev_priv(dev);
> +
> +       dev_err(&oct->pdev->dev, "Setting VF trust not supported\n");
> +       return 0;
> +}
> +
> +static int octep_set_vf_rate(struct net_device *dev, int vf, int min_tx_rate, int max_tx_rate)
> +{
> +       struct octep_device *oct = netdev_priv(dev);
> +
> +       dev_err(&oct->pdev->dev, "Setting VF rate not supported\n");
> +       return 0;
> +}
> +
> +static int octep_set_vf_link_state(struct net_device *dev, int vf, int link_state)
> +{
> +       struct octep_device *oct = netdev_priv(dev);
> +
> +       dev_err(&oct->pdev->dev, "Setting VF link state not supported\n");
> +       return 0;
> +}
> +
> +static int octep_get_vf_stats(struct net_device *dev, int vf, struct ifla_vf_stats *vf_stats)
> +{
> +       struct octep_device *oct = netdev_priv(dev);
> +
> +       dev_err(&oct->pdev->dev, "Getting VF stats not supported\n");
> +       return 0;
> +}
[Kalesh] Do not expose the support for these unsupported hooks in
struct net_device_ops. Stack has a check for the support before
invoking the callback.
> +
>  static const struct net_device_ops octep_netdev_ops = {
>         .ndo_open                = octep_open,
>         .ndo_stop                = octep_stop,
> @@ -1146,6 +1235,15 @@ 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,
> +       /* for VFs */
> +       .ndo_get_vf_config       = octep_get_vf_config,
> +       .ndo_set_vf_mac          = octep_set_vf_mac,
> +       .ndo_set_vf_vlan         = octep_set_vf_vlan,
> +       .ndo_set_vf_spoofchk     = octep_set_vf_spoofchk,
> +       .ndo_set_vf_trust        = octep_set_vf_trust,
> +       .ndo_set_vf_rate         = octep_set_vf_rate,
> +       .ndo_set_vf_link_state   = octep_set_vf_link_state,
> +       .ndo_get_vf_stats        = octep_get_vf_stats,
>  };
>
>  /**
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> index fee59e0e0138..3b56916af468 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> @@ -220,6 +220,7 @@ 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;
>  };
>
> 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..be21ad5ec75e 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,23 @@ 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) {
> +               dev_err(&oct->pdev->dev,
> +                       "VF%d attampted to override administrative set MAC address\n",
[d] typo in "attempted"
> +                       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 +182,17 @@ 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..339977c7131a 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h
> @@ -23,6 +23,9 @@ enum octep_pfvf_mbox_version {
>
>  #define OCTEP_PFVF_MBOX_VERSION_CURRENT        OCTEP_PFVF_MBOX_VERSION_V2
>
> +/* VF flags */
> +#define OCTEON_PFVF_FLAG_MAC_SET_BY_PF  BIT_ULL(0) /* PF has set VF MAC address */
> +
>  enum octep_pfvf_mbox_opcode {
>         OCTEP_PFVF_MBOX_CMD_VERSION,
>         OCTEP_PFVF_MBOX_CMD_SET_MTU,
> --
> 2.25.1
>
>


-- 
Regards,
Kalesh A P

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [EXTERNAL] Re: [PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[Simon Horman]

On Mon, Nov 11, 2024 at 05:29:53AM +0000, Shinas Rasheed wrote:
> Hi Simon,
> 
> On Thu, Nov 07, 2024 at 04:16:37AM -0800, Shinas Rasheed wrote:
> >> These APIs are needed to support applicaitons that use netlink to get VF
> >> information from a PF driver.
> >> 
> >> Signed-off-by: Shinas Rasheed <mailto:srasheed@marvell.com>
> >
> >...
> >
> >> +static int octep_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos, __be16 vlan_proto)
> >> +{
> >> +	struct octep_device *oct = netdev_priv(dev);
> >> +
> >> +	dev_err(&oct->pdev->dev, "Setting VF VLAN not supported\n");>
> >> +	return 0;
> >> +}
> >
> >Hi Shinas,
> >
> >Given that the operation is not supported I think it would
> >be more appropriate to return -EOPNOTSUPP. And moreover, given
> >that this is a noop, I think it would be yet more appropriate
> >not to implement it at all and let the core treat it as not supported.
> >
> >Likewise for other NDOs implemented as noops in this patch.
> >
> >...
> 
> I think the problem was for some userspace programs and operators, sometimes returning -EOPNOTSUPP is a no-go. I think the idea was at least if the user saw these messages, they would know to
> set it in some other way, and also not have the operator stop just because setting these values failed. Though I understand that’s counter-intuitive, but sometimes it lets operators work and go ahead. What do you think so?

Hi Shinas,

I think it would be good to provide more detail of such use-cases:
my understanding is that not implementing the operations would
be the go-to solution if they are not supported by the driver.

> 
> Thanks for the comments!

RE: [EXTERNAL] Re: [PATCH net-next] octeon_ep: add ndo ops for VFs in PF driver

[Shinas Rasheed]

Hi Kalesh,

> -----Original Message-----
> From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com>
> Sent: Monday, November 11, 2024 11:39 AM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Haseeb Gani
> <hgani@marvell.com>; Sathesh B Edara <sedara@marvell.com>; Vimlesh
> Kumar <vimleshk@marvell.com>; thaller@redhat.com; wizhao@redhat.com;
> kheib@redhat.com; egallen@redhat.com; konguyen@redhat.com;
> horms@kernel.org; frank.feng@synaxg.com; Veerasenareddy Burru
> <vburru@marvell.com>; Andrew Lunn <andrew+netdev@lunn.ch>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> Subject: [EXTERNAL] Re: [PATCH net-next] octeon_ep: add ndo ops for VFs in
> PF driver
> 
> Hi Shinas,
> 
> On Thu, Nov 7, 2024 at 5:47 PM Shinas Rasheed <srasheed@marvell.com>
> wrote:
> >
> > These APIs are needed to support applicaitons that use netlink to get VF
> [d] typo in applications
> > information from a PF driver.
> >
> > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > ---
> >  .../ethernet/marvell/octeon_ep/octep_main.c   | 98 +++++++++++++++++++
> >  .../ethernet/marvell/octeon_ep/octep_main.h   |  1 +
> >  .../marvell/octeon_ep/octep_pfvf_mbox.c       | 22 ++++-
> >  .../marvell/octeon_ep/octep_pfvf_mbox.h       |  3 +
> >  4 files changed, 122 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > index 549436efc204..129c68f5a4ba 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > @@ -1137,6 +1137,95 @@ 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->vlan = 0;
> > +       ivi->qos = 0;
> > +       ivi->spoofchk = 0;
> > +       ivi->linkstate = IFLA_VF_LINK_STATE_ENABLE;
> > +       ivi->trusted = true;
> > +       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 i, 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);
> > +       for (i = 0; i < ETH_ALEN; i++)
> > +               oct->vf_info[vf].mac_addr[i] = mac[i];
> [Kalesh] Is there any reason to no do a memcpy here or a ether_addr_copy()?

ACK

> > +       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);
> [d] looks like this return is unnecessary. You can "return rc" at the
> end of the function.

ACK

> > +static int octep_get_vf_stats(struct net_device *dev, int vf, struct
> ifla_vf_stats *vf_stats)
> > +{
> > +       struct octep_device *oct = netdev_priv(dev);
> > +
> > +       dev_err(&oct->pdev->dev, "Getting VF stats not supported\n");
> > +       return 0;
> > +}
> [Kalesh] Do not expose the support for these unsupported hooks in
> struct net_device_ops. Stack has a check for the support before
> invoking the callback.

ACK

> > +
> >  static const struct net_device_ops octep_netdev_ops = {
> >         .ndo_open                = octep_open,
> >         .ndo_stop                = octep_stop,
> > @@ -1146,6 +1235,15 @@ 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,
> > +       /* for VFs */
> > +       .ndo_get_vf_config       = octep_get_vf_config,
> > +       .ndo_set_vf_mac          = octep_set_vf_mac,
> > +       .ndo_set_vf_vlan         = octep_set_vf_vlan,
> > +       .ndo_set_vf_spoofchk     = octep_set_vf_spoofchk,
> > +       .ndo_set_vf_trust        = octep_set_vf_trust,
> > +       .ndo_set_vf_rate         = octep_set_vf_rate,
> > +       .ndo_set_vf_link_state   = octep_set_vf_link_state,
> > +       .ndo_get_vf_stats        = octep_get_vf_stats,
> >  };
> >
> >  /**
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> > index fee59e0e0138..3b56916af468 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
> > @@ -220,6 +220,7 @@ 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;
> >  };
> >
> > 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..be21ad5ec75e 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,23 @@ 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) {
> > +               dev_err(&oct->pdev->dev,
> > +                       "VF%d attampted to override administrative set MAC
> address\n",
> [d] typo in "attempted"

ACK

Shall post next patch

Thanks,
Shinas