| Message ID | 20230329144548.66708-3-louis.peens@corigine.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Small series of enhancements | expand |
On Wed, Mar 29, 2023 at 04:45:48PM +0200, Louis Peens wrote: > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > For nic application firmware, enable the ports' phy state at the > beginning. And by default its state doesn't change in pace with > the upper state, unless the ethtool private flag "link_state_detach" > is turned off by: > > ethtool --set-private-flags <netdev> link_state_detach off > > With this separation, we're able to keep the VF state up while > bringing down the PF. What does it mean "bringing down the PF"? Thanks > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > Acked-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Louis Peens <louis.peens@corigine.com> > --- > .../ethernet/netronome/nfp/nfp_net_ethtool.c | 103 ++++++++++++++++++ > drivers/net/ethernet/netronome/nfp/nic/main.c | 20 ++++ > 2 files changed, 123 insertions(+) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > index dfedb52b7e70..fd4cf865da4a 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > @@ -841,6 +841,102 @@ static void nfp_net_self_test(struct net_device *netdev, struct ethtool_test *et > netdev_info(netdev, "Test end\n"); > } > > +static bool nfp_pflag_get_link_state_detach(struct net_device *netdev) > +{ > + struct nfp_port *port = nfp_port_from_netdev(netdev); > + > + if (!__nfp_port_get_eth_port(port)) > + return false; > + > + return port->eth_forced; > +} > + > +static int nfp_pflag_set_link_state_detach(struct net_device *netdev, bool en) > +{ > + struct nfp_port *port = nfp_port_from_netdev(netdev); > + struct nfp_eth_table_port *eth_port; > + > + eth_port = __nfp_port_get_eth_port(port); > + if (!eth_port) > + return -EOPNOTSUPP; > + > + if (!en) { > + /* When turning link_state_detach off, we need change the lower > + * phy state if it's different with admin state. > + * Contrarily, we can leave the lower phy state as it is when > + * turning the flag on, since it's detached. > + */ > + int err = nfp_eth_set_configured(port->app->cpp, eth_port->index, > + netif_running(netdev)); > + if (err && err != -EOPNOTSUPP) > + return err; > + } > + > + port->eth_forced = en; > + return 0; > +} > + > +#define DECLARE_NFP_PFLAG(flag) { \ > + .name = #flag, \ > + .get = nfp_pflag_get_##flag, \ > + .set = nfp_pflag_set_##flag, \ > + } > + > +static const struct { > + const char name[ETH_GSTRING_LEN]; > + bool (*get)(struct net_device *netdev); > + int (*set)(struct net_device *netdev, bool en); > +} nfp_pflags[] = { > + DECLARE_NFP_PFLAG(link_state_detach), > +}; > + > +#define NFP_PFLAG_MAX ARRAY_SIZE(nfp_pflags) > + > +static void nfp_get_pflag_strings(struct net_device *netdev, u8 *data) > +{ > + for (u32 i = 0; i < NFP_PFLAG_MAX; i++) > + ethtool_sprintf(&data, nfp_pflags[i].name); > +} > + > +static int nfp_get_pflag_count(struct net_device *netdev) > +{ > + return NFP_PFLAG_MAX; > +} > + > +static u32 nfp_net_get_pflags(struct net_device *netdev) > +{ > + u32 pflags = 0; > + > + for (u32 i = 0; i < NFP_PFLAG_MAX; i++) { > + if (nfp_pflags[i].get(netdev)) > + pflags |= BIT(i); > + } > + > + return pflags; > +} > + > +static int nfp_net_set_pflags(struct net_device *netdev, u32 pflags) > +{ > + u32 changed = nfp_net_get_pflags(netdev) ^ pflags; > + int err; > + > + for (u32 i = 0; i < NFP_PFLAG_MAX; i++) { > + bool en; > + > + if (!(changed & BIT(i))) > + continue; > + > + en = !!(pflags & BIT(i)); > + err = nfp_pflags[i].set(netdev, en); > + if (err) > + return err; > + > + netdev_info(netdev, "%s is %sabled.", nfp_pflags[i].name, en ? "en" : "dis"); > + } > + > + return 0; > +} > + > static unsigned int nfp_vnic_get_sw_stats_count(struct net_device *netdev) > { > struct nfp_net *nn = netdev_priv(netdev); > @@ -1107,6 +1203,9 @@ static void nfp_net_get_strings(struct net_device *netdev, > case ETH_SS_TEST: > nfp_get_self_test_strings(netdev, data); > break; > + case ETH_SS_PRIV_FLAGS: > + nfp_get_pflag_strings(netdev, data); > + break; > } > } > > @@ -1143,6 +1242,8 @@ static int nfp_net_get_sset_count(struct net_device *netdev, int sset) > return cnt; > case ETH_SS_TEST: > return nfp_get_self_test_count(netdev); > + case ETH_SS_PRIV_FLAGS: > + return nfp_get_pflag_count(netdev); > default: > return -EOPNOTSUPP; > } > @@ -2116,6 +2217,8 @@ static const struct ethtool_ops nfp_net_ethtool_ops = { > .set_fecparam = nfp_port_set_fecparam, > .get_pauseparam = nfp_port_get_pauseparam, > .set_phys_id = nfp_net_set_phys_id, > + .get_priv_flags = nfp_net_get_pflags, > + .set_priv_flags = nfp_net_set_pflags, > }; > > const struct ethtool_ops nfp_port_ethtool_ops = { > diff --git a/drivers/net/ethernet/netronome/nfp/nic/main.c b/drivers/net/ethernet/netronome/nfp/nic/main.c > index 9dd5afe37f6e..7d8505c033ee 100644 > --- a/drivers/net/ethernet/netronome/nfp/nic/main.c > +++ b/drivers/net/ethernet/netronome/nfp/nic/main.c > @@ -6,6 +6,7 @@ > #include "../nfp_app.h" > #include "../nfp_main.h" > #include "../nfp_net.h" > +#include "../nfp_port.h" > #include "main.h" > > static int nfp_nic_init(struct nfp_app *app) > @@ -32,11 +33,30 @@ static void nfp_nic_sriov_disable(struct nfp_app *app) > > static int nfp_nic_vnic_init(struct nfp_app *app, struct nfp_net *nn) > { > + struct nfp_port *port = nn->port; > + > + if (port->type == NFP_PORT_PHYS_PORT) { > + /* Enable PHY state here, and its state doesn't change in > + * pace with the port upper state by default. The behavior > + * can be modified by ethtool private flag "link_state_detach". > + */ > + int err = nfp_eth_set_configured(app->cpp, > + port->eth_port->index, > + true); > + if (err >= 0) > + port->eth_forced = true; > + } > + > return nfp_nic_dcb_init(nn); > } > > static void nfp_nic_vnic_clean(struct nfp_app *app, struct nfp_net *nn) > { > + struct nfp_port *port = nn->port; > + > + if (port->type == NFP_PORT_PHYS_PORT) > + nfp_eth_set_configured(app->cpp, port->eth_port->index, false); > + > nfp_nic_dcb_clean(nn); > } > > -- > 2.34.1 >
On Wed, 29 Mar 2023 16:45:48 +0200 Louis Peens wrote: > For nic application firmware, enable the ports' phy state at the > beginning. And by default its state doesn't change in pace with > the upper state, unless the ethtool private flag "link_state_detach" > is turned off by: > > ethtool --set-private-flags <netdev> link_state_detach off > > With this separation, we're able to keep the VF state up while > bringing down the PF. This commit message is very confusing. Please rewrite it.
On Wed, 29 Mar 2023 21:52:35 +0300, Leon Romanovsky wrote: > On Wed, Mar 29, 2023 at 04:45:48PM +0200, Louis Peens wrote: > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > For nic application firmware, enable the ports' phy state at the > > beginning. And by default its state doesn't change in pace with > > the upper state, unless the ethtool private flag "link_state_detach" > > is turned off by: > > > > ethtool --set-private-flags <netdev> link_state_detach off > > > > With this separation, we're able to keep the VF state up while > > bringing down the PF. > > What does it mean "bringing down the PF"? Sorry for confusing, it means if-down/admin-down the uplink port. I'll rewrite the commit message as Jakub suggested. > > Thanks >
On Wed, 29 Mar 2023 12:24:22 -0700, Jakub Kicinski wrote: > On Wed, 29 Mar 2023 16:45:48 +0200 Louis Peens wrote: > > For nic application firmware, enable the ports' phy state at the > > beginning. And by default its state doesn't change in pace with > > the upper state, unless the ethtool private flag "link_state_detach" > > is turned off by: > > > > ethtool --set-private-flags <netdev> link_state_detach off > > > > With this separation, we're able to keep the VF state up while > > bringing down the PF. > > This commit message is very confusing. Please rewrite it. How about " With this separation, the lower phy state of uplink port can be kept link-on no matter what the upper admin state is. Thus the corresponding VFs can also link up and communicate with exterior through the uplink port. "
On Thu, 30 Mar 2023 01:56:13 +0000 Yinjun Zhang wrote: > On Wed, 29 Mar 2023 12:24:22 -0700, Jakub Kicinski wrote: > > On Wed, 29 Mar 2023 16:45:48 +0200 Louis Peens wrote: > > > For nic application firmware, enable the ports' phy state at the > > > beginning. And by default its state doesn't change in pace with > > > the upper state, unless the ethtool private flag "link_state_detach" > > > is turned off by: > > > > > > ethtool --set-private-flags <netdev> link_state_detach off > > > > > > With this separation, we're able to keep the VF state up while > > > bringing down the PF. > > > > This commit message is very confusing. Please rewrite it. > > How about > " > With this separation, the lower phy state of uplink port can be kept > link-on no matter what the upper admin state is. What is "upper", in this context? grep the networking code for upper, is that what you mean? > Thus the corresponding > VFs can also link up and communicate with exterior through the uplink > port. > "
On Wed, 29 Mar 2023 19:41:26 -0700, Jakub Kicinski wrote: > On Thu, 30 Mar 2023 01:56:13 +0000 Yinjun Zhang wrote: > > On Wed, 29 Mar 2023 12:24:22 -0700, Jakub Kicinski wrote: > > > On Wed, 29 Mar 2023 16:45:48 +0200 Louis Peens wrote: > > > > For nic application firmware, enable the ports' phy state at the > > > > beginning. And by default its state doesn't change in pace with > > > > the upper state, unless the ethtool private flag "link_state_detach" > > > > is turned off by: > > > > > > > > ethtool --set-private-flags <netdev> link_state_detach off > > > > > > > > With this separation, we're able to keep the VF state up while > > > > bringing down the PF. > > > > > > This commit message is very confusing. Please rewrite it. > > > > How about > > " > > With this separation, the lower phy state of uplink port can be kept > > link-on no matter what the upper admin state is. > > What is "upper", in this context? grep the networking code for upper, > is that what you mean? Sorry, it's not that meaning. I'll remove this "upper", use netdev state instead. > > > Thus the corresponding > > VFs can also link up and communicate with exterior through the uplink > > port. > > "
On Thu, 30 Mar 2023 02:57:34 +0000 Yinjun Zhang wrote: > > What is "upper", in this context? grep the networking code for upper, > > is that what you mean? > > Sorry, it's not that meaning. I'll remove this "upper", use netdev state > instead. Alright, so legacy SR-IOV, no representors, and you just want to let the VFs talk to the world even when the PF netdev is ifdown'ed ? Why?
On Wed, 29 Mar 2023 20:12:25 -0700, Jakub Kicinski wrote: > On Thu, 30 Mar 2023 02:57:34 +0000 Yinjun Zhang wrote: > > > What is "upper", in this context? grep the networking code for upper, > > > is that what you mean? > > > > Sorry, it's not that meaning. I'll remove this "upper", use netdev state > > instead. > > Alright, so legacy SR-IOV, no representors, and you just want to let > the VFs talk to the world even when the PF netdev is ifdown'ed ? > > Why? I have to say most of other vendors behave like this. It's more practical and required by users.
On Thu, 30 Mar 2023 03:33:30 +0000 Yinjun Zhang wrote: > > Why? > > I have to say most of other vendors behave like this. It's more practical > and required by users. That's not really a practical explanation. Why does anyone want traffic to flow thru a downed port. Don't down the port, if you want it to be up, I'd think. This patch is very unlikely to be accepted upstream. Custom knobs to do weird things are really not our favorite.
On Wed, 29 Mar 2023 21:00:48 -0700, Jakub Kicinski wrote: > On Thu, 30 Mar 2023 03:33:30 +0000 Yinjun Zhang wrote: > > > Why? > > > > I have to say most of other vendors behave like this. It's more practical > > and required by users. > > That's not really a practical explanation. Why does anyone want traffic > to flow thru a downed port. Don't down the port, if you want it to be > up, I'd think. Here it means down in netdev layer, not physical layer down. We don't expect the host to talk outside through a downed port, only allow the VMs that used VFs to talk(it depends on the VF netdev state). > > This patch is very unlikely to be accepted upstream. > Custom knobs to do weird things are really not our favorite. As I said, it's not so custom, but rather very common.
On Thu, 30 Mar 2023 05:55:22 +0000 Yinjun Zhang wrote: > > This patch is very unlikely to be accepted upstream. > > Custom knobs to do weird things are really not our favorite. > > As I said, it's not so custom, but rather very common. Just to be 100% clear, if you send v2 of this patch please include: Nacked-by: Jakub Kicinski <kuba@kernel.org>
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c index dfedb52b7e70..fd4cf865da4a 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c @@ -841,6 +841,102 @@ static void nfp_net_self_test(struct net_device *netdev, struct ethtool_test *et netdev_info(netdev, "Test end\n"); } +static bool nfp_pflag_get_link_state_detach(struct net_device *netdev) +{ + struct nfp_port *port = nfp_port_from_netdev(netdev); + + if (!__nfp_port_get_eth_port(port)) + return false; + + return port->eth_forced; +} + +static int nfp_pflag_set_link_state_detach(struct net_device *netdev, bool en) +{ + struct nfp_port *port = nfp_port_from_netdev(netdev); + struct nfp_eth_table_port *eth_port; + + eth_port = __nfp_port_get_eth_port(port); + if (!eth_port) + return -EOPNOTSUPP; + + if (!en) { + /* When turning link_state_detach off, we need change the lower + * phy state if it's different with admin state. + * Contrarily, we can leave the lower phy state as it is when + * turning the flag on, since it's detached. + */ + int err = nfp_eth_set_configured(port->app->cpp, eth_port->index, + netif_running(netdev)); + if (err && err != -EOPNOTSUPP) + return err; + } + + port->eth_forced = en; + return 0; +} + +#define DECLARE_NFP_PFLAG(flag) { \ + .name = #flag, \ + .get = nfp_pflag_get_##flag, \ + .set = nfp_pflag_set_##flag, \ + } + +static const struct { + const char name[ETH_GSTRING_LEN]; + bool (*get)(struct net_device *netdev); + int (*set)(struct net_device *netdev, bool en); +} nfp_pflags[] = { + DECLARE_NFP_PFLAG(link_state_detach), +}; + +#define NFP_PFLAG_MAX ARRAY_SIZE(nfp_pflags) + +static void nfp_get_pflag_strings(struct net_device *netdev, u8 *data) +{ + for (u32 i = 0; i < NFP_PFLAG_MAX; i++) + ethtool_sprintf(&data, nfp_pflags[i].name); +} + +static int nfp_get_pflag_count(struct net_device *netdev) +{ + return NFP_PFLAG_MAX; +} + +static u32 nfp_net_get_pflags(struct net_device *netdev) +{ + u32 pflags = 0; + + for (u32 i = 0; i < NFP_PFLAG_MAX; i++) { + if (nfp_pflags[i].get(netdev)) + pflags |= BIT(i); + } + + return pflags; +} + +static int nfp_net_set_pflags(struct net_device *netdev, u32 pflags) +{ + u32 changed = nfp_net_get_pflags(netdev) ^ pflags; + int err; + + for (u32 i = 0; i < NFP_PFLAG_MAX; i++) { + bool en; + + if (!(changed & BIT(i))) + continue; + + en = !!(pflags & BIT(i)); + err = nfp_pflags[i].set(netdev, en); + if (err) + return err; + + netdev_info(netdev, "%s is %sabled.", nfp_pflags[i].name, en ? "en" : "dis"); + } + + return 0; +} + static unsigned int nfp_vnic_get_sw_stats_count(struct net_device *netdev) { struct nfp_net *nn = netdev_priv(netdev); @@ -1107,6 +1203,9 @@ static void nfp_net_get_strings(struct net_device *netdev, case ETH_SS_TEST: nfp_get_self_test_strings(netdev, data); break; + case ETH_SS_PRIV_FLAGS: + nfp_get_pflag_strings(netdev, data); + break; } } @@ -1143,6 +1242,8 @@ static int nfp_net_get_sset_count(struct net_device *netdev, int sset) return cnt; case ETH_SS_TEST: return nfp_get_self_test_count(netdev); + case ETH_SS_PRIV_FLAGS: + return nfp_get_pflag_count(netdev); default: return -EOPNOTSUPP; } @@ -2116,6 +2217,8 @@ static const struct ethtool_ops nfp_net_ethtool_ops = { .set_fecparam = nfp_port_set_fecparam, .get_pauseparam = nfp_port_get_pauseparam, .set_phys_id = nfp_net_set_phys_id, + .get_priv_flags = nfp_net_get_pflags, + .set_priv_flags = nfp_net_set_pflags, }; const struct ethtool_ops nfp_port_ethtool_ops = { diff --git a/drivers/net/ethernet/netronome/nfp/nic/main.c b/drivers/net/ethernet/netronome/nfp/nic/main.c index 9dd5afe37f6e..7d8505c033ee 100644 --- a/drivers/net/ethernet/netronome/nfp/nic/main.c +++ b/drivers/net/ethernet/netronome/nfp/nic/main.c @@ -6,6 +6,7 @@ #include "../nfp_app.h" #include "../nfp_main.h" #include "../nfp_net.h" +#include "../nfp_port.h" #include "main.h" static int nfp_nic_init(struct nfp_app *app) @@ -32,11 +33,30 @@ static void nfp_nic_sriov_disable(struct nfp_app *app) static int nfp_nic_vnic_init(struct nfp_app *app, struct nfp_net *nn) { + struct nfp_port *port = nn->port; + + if (port->type == NFP_PORT_PHYS_PORT) { + /* Enable PHY state here, and its state doesn't change in + * pace with the port upper state by default. The behavior + * can be modified by ethtool private flag "link_state_detach". + */ + int err = nfp_eth_set_configured(app->cpp, + port->eth_port->index, + true); + if (err >= 0) + port->eth_forced = true; + } + return nfp_nic_dcb_init(nn); } static void nfp_nic_vnic_clean(struct nfp_app *app, struct nfp_net *nn) { + struct nfp_port *port = nn->port; + + if (port->type == NFP_PORT_PHYS_PORT) + nfp_eth_set_configured(app->cpp, port->eth_port->index, false); + nfp_nic_dcb_clean(nn); }