netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH intel-next v3] i40e: allow toggling loopback mode via ndo_set_features callback
@ 2022-11-16 16:47 Tirthendu Sarkar
  2022-11-17 16:15 ` Alexander Lobakin
  0 siblings, 1 reply; 3+ messages in thread
From: Tirthendu Sarkar @ 2022-11-16 16:47 UTC (permalink / raw)
  To: tirtha, magnus.karlsson, intel-wired-lan, anthony.l.nguyen,
	maciej.fijalkowski
  Cc: netdev

Add support for NETIF_F_LOOPBACK. This feature can be set via:
$ ethtool -K eth0 loopback <on|off>

This sets the MAC Tx->Rx loopback.

This feature is used for the xsk selftests, and might have other uses
too.

Changelog:
    v2 -> v3:
     - Fixed loopback macros as per NVM version 6.01+.
     - Renamed existing macros as *_LEGACY
     - Based on NVM verison appropriate macro is used for MAC loopback.

    v1 -> v2:
     - Moved loopback to netdev's hardware features as suggested by
       Alexandr Lobakin.

Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_adminq_cmd.h | 10 +++++--
 drivers/net/ethernet/intel/i40e/i40e_common.c | 26 +++++++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 28 ++++++++++++++++++-
 .../net/ethernet/intel/i40e/i40e_prototype.h  |  3 ++
 4 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
index 60f9e0a6aaca..7532553a6982 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
@@ -1795,9 +1795,13 @@ I40E_CHECK_CMD_LENGTH(i40e_aqc_an_advt_reg);
 /* Set Loopback mode (0x0618) */
 struct i40e_aqc_set_lb_mode {
 	__le16	lb_mode;
-#define I40E_AQ_LB_PHY_LOCAL	0x01
-#define I40E_AQ_LB_PHY_REMOTE	0x02
-#define I40E_AQ_LB_MAC_LOCAL	0x04
+#define I40E_LEGACY_LOOPBACK_NVM_VER	0x6000
+#define I40E_AQ_LB_MAC_LOCAL		0x01
+#define I40E_AQ_LB_PHY_LOCAL		0x05
+#define I40E_AQ_LB_PHY_REMOTE		0x06
+#define I40E_AQ_LB_PHY_LOCAL_LEGACY   	0x01
+#define I40E_AQ_LB_PHY_REMOTE_LEGACY  	0x02
+#define I40E_AQ_LB_MAC_LOCAL_LEGACY   	0x04
 	u8	reserved[14];
 };
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 4f01e2a6b6bb..8f764ff5c990 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -1830,6 +1830,32 @@ i40e_status i40e_aq_set_phy_int_mask(struct i40e_hw *hw,
 	return status;
 }
 
+/**
+ * i40e_aq_set_mac_loopback
+ * @hw: pointer to the HW struct
+ * @ena_lpbk: Enable or Disable loopback
+ * @cmd_details: pointer to command details structure or NULL
+ *
+ * Enable/disable loopback on a given port
+ */
+i40e_status i40e_aq_set_mac_loopback(struct i40e_hw *hw, bool ena_lpbk,
+				     struct i40e_asq_cmd_details *cmd_details)
+{
+	struct i40e_aq_desc desc;
+	struct i40e_aqc_set_lb_mode *cmd =
+		(struct i40e_aqc_set_lb_mode *)&desc.params.raw;
+
+	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_set_lb_modes);
+	if (ena_lpbk) {
+		if (hw->nvm.version <= I40E_LEGACY_LOOPBACK_NVM_VER)
+			cmd->lb_mode = cpu_to_le16(I40E_AQ_LB_MAC_LOCAL_LEGACY);
+		else
+			cmd->lb_mode = cpu_to_le16(I40E_AQ_LB_MAC_LOCAL);
+	}
+
+	return i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
+}
+
 /**
  * i40e_aq_set_phy_debug
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4880b740fa6e..1941715b6223 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12920,6 +12920,28 @@ static void i40e_clear_rss_lut(struct i40e_vsi *vsi)
 	}
 }
 
+/**
+ * i40e_set_loopback - turn on/off loopback mode on underlying PF
+ * @vsi: ptr to VSI
+ * @ena: flag to indicate the on/off setting
+ */
+static int i40e_set_loopback(struct i40e_vsi *vsi, bool ena)
+{
+	bool if_running = netif_running(vsi->netdev);
+	int ret;
+
+	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
+		i40e_down(vsi);
+
+	ret = i40e_aq_set_mac_loopback(&vsi->back->hw, ena, NULL);
+	if (ret)
+		netdev_err(vsi->netdev, "Failed to toggle loopback state\n");
+	if (if_running)
+		i40e_up(vsi);
+
+	return ret;
+}
+
 /**
  * i40e_set_features - set the netdev feature flags
  * @netdev: ptr to the netdev being adjusted
@@ -12960,6 +12982,10 @@ static int i40e_set_features(struct net_device *netdev,
 	if (need_reset)
 		i40e_do_reset(pf, I40E_PF_RESET_FLAG, true);
 
+	if (netdev->hw_features & NETIF_F_LOOPBACK)
+		if (i40e_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)))
+			return -EINVAL;
+
 	return 0;
 }
 
@@ -13722,7 +13748,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 	if (!(pf->flags & I40E_FLAG_MFP_ENABLED))
 		hw_features |= NETIF_F_NTUPLE | NETIF_F_HW_TC;
 
-	netdev->hw_features |= hw_features;
+	netdev->hw_features |= hw_features | NETIF_F_LOOPBACK;
 
 	netdev->features |= hw_features | NETIF_F_HW_VLAN_CTAG_FILTER;
 	netdev->hw_enc_features |= NETIF_F_TSO_MANGLEID;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index ebdcde6f1aeb..9a71121420c3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -105,6 +105,9 @@ enum i40e_status_code i40e_aq_set_phy_config(struct i40e_hw *hw,
 				struct i40e_asq_cmd_details *cmd_details);
 enum i40e_status_code i40e_set_fc(struct i40e_hw *hw, u8 *aq_failures,
 				  bool atomic_reset);
+i40e_status i40e_aq_set_mac_loopback(struct i40e_hw *hw,
+				     bool ena_lpbk,
+				     struct i40e_asq_cmd_details *cmd_details);
 i40e_status i40e_aq_set_phy_int_mask(struct i40e_hw *hw, u16 mask,
 				     struct i40e_asq_cmd_details *cmd_details);
 i40e_status i40e_aq_clear_pxe_mode(struct i40e_hw *hw,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH intel-next v3] i40e: allow toggling loopback mode via ndo_set_features callback
  2022-11-16 16:47 [PATCH intel-next v3] i40e: allow toggling loopback mode via ndo_set_features callback Tirthendu Sarkar
@ 2022-11-17 16:15 ` Alexander Lobakin
  2022-11-18  9:12   ` Sarkar, Tirthendu
  0 siblings, 1 reply; 3+ messages in thread
From: Alexander Lobakin @ 2022-11-17 16:15 UTC (permalink / raw)
  To: Tirthendu Sarkar
  Cc: Alexander Lobakin, tirtha, magnus.karlsson, intel-wired-lan,
	anthony.l.nguyen, maciej.fijalkowski, netdev

From: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
Date: Wed, 16 Nov 2022 22:17:09 +0530

> Add support for NETIF_F_LOOPBACK. This feature can be set via:
> $ ethtool -K eth0 loopback <on|off>
> 
> This sets the MAC Tx->Rx loopback.
> 
> This feature is used for the xsk selftests, and might have other uses
> too.
> 
> Changelog:
>     v2 -> v3:
>      - Fixed loopback macros as per NVM version 6.01+.
>      - Renamed existing macros as *_LEGACY
>      - Based on NVM verison appropriate macro is used for MAC loopback.
> 
>     v1 -> v2:
>      - Moved loopback to netdev's hardware features as suggested by
>        Alexandr Lobakin.
> 
> Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
> ---
>  .../net/ethernet/intel/i40e/i40e_adminq_cmd.h | 10 +++++--
>  drivers/net/ethernet/intel/i40e/i40e_common.c | 26 +++++++++++++++++
>  drivers/net/ethernet/intel/i40e/i40e_main.c   | 28 ++++++++++++++++++-
>  .../net/ethernet/intel/i40e/i40e_prototype.h  |  3 ++
>  4 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> index 60f9e0a6aaca..7532553a6982 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> @@ -1795,9 +1795,13 @@ I40E_CHECK_CMD_LENGTH(i40e_aqc_an_advt_reg);
>  /* Set Loopback mode (0x0618) */
>  struct i40e_aqc_set_lb_mode {
>  	__le16	lb_mode;
> -#define I40E_AQ_LB_PHY_LOCAL	0x01
> -#define I40E_AQ_LB_PHY_REMOTE	0x02
> -#define I40E_AQ_LB_MAC_LOCAL	0x04
> +#define I40E_LEGACY_LOOPBACK_NVM_VER	0x6000
> +#define I40E_AQ_LB_MAC_LOCAL		0x01
> +#define I40E_AQ_LB_PHY_LOCAL		0x05
> +#define I40E_AQ_LB_PHY_REMOTE		0x06
> +#define I40E_AQ_LB_PHY_LOCAL_LEGACY   	0x01
> +#define I40E_AQ_LB_PHY_REMOTE_LEGACY  	0x02
> +#define I40E_AQ_LB_MAC_LOCAL_LEGACY   	0x04

Do you need any of those, apart from MAC_LOCAL{,_LEGACY}? I think
it's better to keep only the values you actually use.

>  	u8	reserved[14];
>  };
>  
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 4f01e2a6b6bb..8f764ff5c990 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -1830,6 +1830,32 @@ i40e_status i40e_aq_set_phy_int_mask(struct i40e_hw *hw,
>  	return status;
>  }
>  
> +/**
> + * i40e_aq_set_mac_loopback
> + * @hw: pointer to the HW struct
> + * @ena_lpbk: Enable or Disable loopback
> + * @cmd_details: pointer to command details structure or NULL
> + *
> + * Enable/disable loopback on a given port
> + */
> +i40e_status i40e_aq_set_mac_loopback(struct i40e_hw *hw, bool ena_lpbk,
> +				     struct i40e_asq_cmd_details *cmd_details)
> +{
> +	struct i40e_aq_desc desc;
> +	struct i40e_aqc_set_lb_mode *cmd =
> +		(struct i40e_aqc_set_lb_mode *)&desc.params.raw;
> +
> +	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_set_lb_modes);
> +	if (ena_lpbk) {
> +		if (hw->nvm.version <= I40E_LEGACY_LOOPBACK_NVM_VER)
> +			cmd->lb_mode = cpu_to_le16(I40E_AQ_LB_MAC_LOCAL_LEGACY);
> +		else
> +			cmd->lb_mode = cpu_to_le16(I40E_AQ_LB_MAC_LOCAL);
> +	}
> +
> +	return i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
> +}
> +
>  /**
>   * i40e_aq_set_phy_debug
>   * @hw: pointer to the hw struct
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4880b740fa6e..1941715b6223 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -12920,6 +12920,28 @@ static void i40e_clear_rss_lut(struct i40e_vsi *vsi)
>  	}
>  }
>  
> +/**
> + * i40e_set_loopback - turn on/off loopback mode on underlying PF
> + * @vsi: ptr to VSI
> + * @ena: flag to indicate the on/off setting
> + */
> +static int i40e_set_loopback(struct i40e_vsi *vsi, bool ena)
> +{
> +	bool if_running = netif_running(vsi->netdev);
> +	int ret;
> +
> +	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> +		i40e_down(vsi);
> +
> +	ret = i40e_aq_set_mac_loopback(&vsi->back->hw, ena, NULL);
> +	if (ret)
> +		netdev_err(vsi->netdev, "Failed to toggle loopback state\n");
> +	if (if_running)
> +		i40e_up(vsi);

You do i40e_down() only if %__I40E_VSI_DOWN was not set. So I guess
you need to do ifup only if you did ifdown. So I think the function
start must be a bit differen? Like:

	if (if_running && !test_and_set_bit())
		i40e_down();
	else
		if_running = false;

To not do an ifup when it wasn't you who did an ifdown?

> +
> +	return ret;
> +}
> +
>  /**
>   * i40e_set_features - set the netdev feature flags
>   * @netdev: ptr to the netdev being adjusted
> @@ -12960,6 +12982,10 @@ static int i40e_set_features(struct net_device *netdev,
>  	if (need_reset)
>  		i40e_do_reset(pf, I40E_PF_RESET_FLAG, true);
>  
> +	if (netdev->hw_features & NETIF_F_LOOPBACK)

Isn't that condition redundant? I think you add %NETIF_F_LOOPBACK
to ::hw_features unconditionally, it's always there?

> +		if (i40e_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)))

The double negation is redundant as the second argument is bool.

> +			return -EINVAL;

Why don't you propagate return value from i40e_set_loopback() and
return only 0 or -%EINVAL? If you don't need the actual return code,
just make set_loopback() return bool, otherwise

	if (changed_features & NETIF_F_LOOPBACK)
		return i40e_set_loopback(vsi, features & NETIF_F_LOOPBACK);

	return 0;

> +
>  	return 0;
>  }

[...]

> -- 
> 2.34.1

Thanks,
Olek

^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH intel-next v3] i40e: allow toggling loopback mode via ndo_set_features callback
  2022-11-17 16:15 ` Alexander Lobakin
@ 2022-11-18  9:12   ` Sarkar, Tirthendu
  0 siblings, 0 replies; 3+ messages in thread
From: Sarkar, Tirthendu @ 2022-11-18  9:12 UTC (permalink / raw)
  To: Lobakin, Alexandr
  Cc: tirtha, Karlsson, Magnus, intel-wired-lan, Nguyen, Anthony L,
	Fijalkowski, Maciej, netdev

> -----Original Message-----
> From: Lobakin, Alexandr <alexandr.lobakin@intel.com>
> Sent: Thursday, November 17, 2022 9:45 PM
> 
> From: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
> Date: Wed, 16 Nov 2022 22:17:09 +0530
> 
> > Add support for NETIF_F_LOOPBACK. This feature can be set via:
> > $ ethtool -K eth0 loopback <on|off>
> >
> > This sets the MAC Tx->Rx loopback.
> >
> > This feature is used for the xsk selftests, and might have other uses
> > too.
> >
> > Changelog:
> >     v2 -> v3:
> >      - Fixed loopback macros as per NVM version 6.01+.
> >      - Renamed existing macros as *_LEGACY
> >      - Based on NVM verison appropriate macro is used for MAC loopback.
> >
> >     v1 -> v2:
> >      - Moved loopback to netdev's hardware features as suggested by
> >        Alexandr Lobakin.
> >
> > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.com>
> > ---
> >  .../net/ethernet/intel/i40e/i40e_adminq_cmd.h | 10 +++++--
> >  drivers/net/ethernet/intel/i40e/i40e_common.c | 26 +++++++++++++++++
> >  drivers/net/ethernet/intel/i40e/i40e_main.c   | 28 ++++++++++++++++++-
> >  .../net/ethernet/intel/i40e/i40e_prototype.h  |  3 ++
> >  4 files changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> > index 60f9e0a6aaca..7532553a6982 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
> > @@ -1795,9 +1795,13 @@ I40E_CHECK_CMD_LENGTH(i40e_aqc_an_advt_reg);
> >  /* Set Loopback mode (0x0618) */
> >  struct i40e_aqc_set_lb_mode {
> >  	__le16	lb_mode;
> > -#define I40E_AQ_LB_PHY_LOCAL	0x01
> > -#define I40E_AQ_LB_PHY_REMOTE	0x02
> > -#define I40E_AQ_LB_MAC_LOCAL	0x04
> > +#define I40E_LEGACY_LOOPBACK_NVM_VER	0x6000
> > +#define I40E_AQ_LB_MAC_LOCAL		0x01
> > +#define I40E_AQ_LB_PHY_LOCAL		0x05
> > +#define I40E_AQ_LB_PHY_REMOTE		0x06
> > +#define I40E_AQ_LB_PHY_LOCAL_LEGACY   	0x01
> > +#define I40E_AQ_LB_PHY_REMOTE_LEGACY  	0x02
> > +#define I40E_AQ_LB_MAC_LOCAL_LEGACY   	0x04
> 
> Do you need any of those, apart from MAC_LOCAL{,_LEGACY}? I think
> it's better to keep only the values you actually use.
> 

OK, I will remove the rest.

> >  	u8	reserved[14];
> >  };
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
> > index 4f01e2a6b6bb..8f764ff5c990 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> > @@ -1830,6 +1830,32 @@ i40e_status i40e_aq_set_phy_int_mask(struct i40e_hw *hw,
> >  	return status;
> >  }
> >
> > +/**
> > + * i40e_aq_set_mac_loopback
> > + * @hw: pointer to the HW struct
> > + * @ena_lpbk: Enable or Disable loopback
> > + * @cmd_details: pointer to command details structure or NULL
> > + *
> > + * Enable/disable loopback on a given port
> > + */
> > +i40e_status i40e_aq_set_mac_loopback(struct i40e_hw *hw, bool ena_lpbk,
> > +				     struct i40e_asq_cmd_details *cmd_details)
> > +{
> > +	struct i40e_aq_desc desc;
> > +	struct i40e_aqc_set_lb_mode *cmd =
> > +		(struct i40e_aqc_set_lb_mode *)&desc.params.raw;
> > +
> > +	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_set_lb_modes);
> > +	if (ena_lpbk) {
> > +		if (hw->nvm.version <= I40E_LEGACY_LOOPBACK_NVM_VER)
> > +			cmd->lb_mode = cpu_to_le16(I40E_AQ_LB_MAC_LOCAL_LEGACY);
> > +		else
> > +			cmd->lb_mode = cpu_to_le16(I40E_AQ_LB_MAC_LOCAL);
> > +	}
> > +
> > +	return i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
> > +}
> > +
> >  /**
> >   * i40e_aq_set_phy_debug
> >   * @hw: pointer to the hw struct
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index 4880b740fa6e..1941715b6223 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -12920,6 +12920,28 @@ static void i40e_clear_rss_lut(struct i40e_vsi *vsi)
> >  	}
> >  }
> >
> > +/**
> > + * i40e_set_loopback - turn on/off loopback mode on underlying PF
> > + * @vsi: ptr to VSI
> > + * @ena: flag to indicate the on/off setting
> > + */
> > +static int i40e_set_loopback(struct i40e_vsi *vsi, bool ena)
> > +{
> > +	bool if_running = netif_running(vsi->netdev);
> > +	int ret;
> > +
> > +	if (if_running && !test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
> > +		i40e_down(vsi);
> > +
> > +	ret = i40e_aq_set_mac_loopback(&vsi->back->hw, ena, NULL);
> > +	if (ret)
> > +		netdev_err(vsi->netdev, "Failed to toggle loopback state\n");
> > +	if (if_running)
> > +		i40e_up(vsi);
> 
> You do i40e_down() only if %__I40E_VSI_DOWN was not set. So I guess
> you need to do ifup only if you did ifdown. So I think the function
> start must be a bit differen? Like:
> 
> 	if (if_running && !test_and_set_bit())
> 		i40e_down();
> 	else
> 		if_running = false;
> 
> To not do an ifup when it wasn't you who did an ifdown?
> 

This was done similar to ice. I have now modified this.

> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * i40e_set_features - set the netdev feature flags
> >   * @netdev: ptr to the netdev being adjusted
> > @@ -12960,6 +12982,10 @@ static int i40e_set_features(struct net_device *netdev,
> >  	if (need_reset)
> >  		i40e_do_reset(pf, I40E_PF_RESET_FLAG, true);
> >
> > +	if (netdev->hw_features & NETIF_F_LOOPBACK)
> 
> Isn't that condition redundant? I think you add %NETIF_F_LOOPBACK
> to ::hw_features unconditionally, it's always there?
> 

Ack. I have now fixed this with correct check.

> > +		if (i40e_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)))
> 
> The double negation is redundant as the second argument is bool.
> 
> > +			return -EINVAL;
> 
> Why don't you propagate return value from i40e_set_loopback() and
> return only 0 or -%EINVAL? If you don't need the actual return code,
> just make set_loopback() return bool, otherwise
> 
> 	if (changed_features & NETIF_F_LOOPBACK)
> 		return i40e_set_loopback(vsi, features & NETIF_F_LOOPBACK);
> 
> 	return 0;
> 

Have fixed this.

> > +
> >  	return 0;
> >  }
> 
> [...]
> 
> > --
> > 2.34.1
> 
> Thanks,
> Olek

Thanks for the comments.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-11-18  9:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-16 16:47 [PATCH intel-next v3] i40e: allow toggling loopback mode via ndo_set_features callback Tirthendu Sarkar
2022-11-17 16:15 ` Alexander Lobakin
2022-11-18  9:12   ` Sarkar, Tirthendu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).