| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Nov 2022 09:12:21 +0000
From: "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>
To: "Lobakin, Alexandr" <alexandr.lobakin@...el.com>
CC: "tirtha@...il.com" <tirtha@...il.com>,
"Karlsson, Magnus" <magnus.karlsson@...el.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH intel-next v3] i40e: allow toggling loopback mode via
ndo_set_features callback
> -----Original Message-----
> From: Lobakin, Alexandr <alexandr.lobakin@...el.com>
> Sent: Thursday, November 17, 2022 9:45 PM
>
> From: Tirthendu Sarkar <tirthendu.sarkar@...el.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@...el.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.
Powered by blists - more mailing lists