[PATCH net-next 0/2] Small series of enhancements

[PATCH net-next 0/2] Small series of enhancements

[Louis Peens]

This small series adds two changes

- The first is a simple update to make sure the dev_port id is getting
  populated properly.
- The second changes the lower link state of the phy ports to be
  disconnected from the admin state. This is mostly for SRIOV cases, as
  explained in the patch. Functionality to make this behaviour
  configurable is added through an ethtool private flag.

Yinjun Zhang (2):
  nfp: initialize netdev's dev_port with correct id
  nfp: separate the port's upper state with lower phy state

 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 103 ++++++++++++++++++
 drivers/net/ethernet/netronome/nfp/nfp_port.c |   1 +
 drivers/net/ethernet/netronome/nfp/nic/main.c |  20 ++++
 3 files changed, 124 insertions(+)

-- 
2.34.1

[PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Louis Peens]

From: Yinjun Zhang <yinjun.zhang@corigine.com>

`dev_port` is used to differentiate devices that share the same
function, which is the case in most of NFP NICs.

In some customized scenario, `dev_port` is used to rename netdev
instead of `phys_port_name`, which requires to initialize it
correctly to get expected netdev name.

Example rules using `dev_port`:

  SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", ATTR{dev_port}=="0", NAME:="ens8np0"
  SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", ATTR{dev_port}=="1", NAME:="ens8np1"

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>
---
 drivers/net/ethernet/netronome/nfp/nfp_port.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c
index 4f2308570dcf..54640bcb70fb 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
@@ -189,6 +189,7 @@ int nfp_port_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
 
 	port->eth_port = &pf->eth_tbl->ports[id];
 	port->eth_id = pf->eth_tbl->ports[id].index;
+	port->netdev->dev_port = id;
 	if (pf->mac_stats_mem)
 		port->eth_stats =
 			pf->mac_stats_mem + port->eth_id * NFP_MAC_STATS_SIZE;
-- 
2.34.1

[PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Louis Peens]

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.

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

Re: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Leon Romanovsky]

On Wed, Mar 29, 2023 at 04:45:47PM +0200, Louis Peens wrote:
> From: Yinjun Zhang <yinjun.zhang@corigine.com>
> 
> `dev_port` is used to differentiate devices that share the same
> function, which is the case in most of NFP NICs.

And how did it work without dev_port?
I have no idea what does it mean "different devices that share the same
function".

Thanks

> 
> In some customized scenario, `dev_port` is used to rename netdev
> instead of `phys_port_name`, which requires to initialize it
> correctly to get expected netdev name.
> 
> Example rules using `dev_port`:
> 
>   SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", ATTR{dev_port}=="0", NAME:="ens8np0"
>   SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", ATTR{dev_port}=="1", NAME:="ens8np1"
> 
> 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>
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_port.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c
> index 4f2308570dcf..54640bcb70fb 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_port.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c
> @@ -189,6 +189,7 @@ int nfp_port_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
>  
>  	port->eth_port = &pf->eth_tbl->ports[id];
>  	port->eth_id = pf->eth_tbl->ports[id].index;
> +	port->netdev->dev_port = id;
>  	if (pf->mac_stats_mem)
>  		port->eth_stats =
>  			pf->mac_stats_mem + port->eth_id * NFP_MAC_STATS_SIZE;
> -- 
> 2.34.1
> 

Re: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Leon Romanovsky]

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
> 

Re: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Jakub Kicinski]

On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote:
> In some customized scenario, `dev_port` is used to rename netdev
> instead of `phys_port_name`, which requires to initialize it
> correctly to get expected netdev name.

What do you mean by "which requires to initialize it correctly to get
expected netdev name." ?

Re: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Jakub Kicinski]

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.

RE: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Yinjun Zhang]

On Wed, 29 Mar 2023 21:49:59 +0300, Leon Romanovsky wrote:
> On Wed, Mar 29, 2023 at 04:45:47PM +0200, Louis Peens wrote:
> > From: Yinjun Zhang <yinjun.zhang@corigine.com>
> >
> > `dev_port` is used to differentiate devices that share the same
> > function, which is the case in most of NFP NICs.
> 
> And how did it work without dev_port?

No functionality fault, just cannot rename netdev as expected when the
udev rules use `dev_port` attribute as the example below.

> I have no idea what does it mean "different devices that share the same
> function".

That's how it's commented for the `dev_port` field of struct netdev:
*      @dev_port:              Used to differentiate devices that share
*                              the same function
I think it's used when single pci function instantiates more than one netdev.
Ref: 3f85944fe207 ("net: Add sysfs file for port number")

> 
> Thanks
> 
> >
> > In some customized scenario, `dev_port` is used to rename netdev
> > instead of `phys_port_name`, which requires to initialize it
> > correctly to get expected netdev name.
> >
> > Example rules using `dev_port`:
> >
> >   SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0",
> ATTR{dev_port}=="0", NAME:="ens8np0"
> >   SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0",
> ATTR{dev_port}=="1", NAME:="ens8np1"
> >

RE: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Yinjun Zhang]

On Wed, 29 Mar 2023 12:22:27 -0700, Jakub Kicinski wrote:
> On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote:
> > In some customized scenario, `dev_port` is used to rename netdev
> > instead of `phys_port_name`, which requires to initialize it
> > correctly to get expected netdev name.
> 
> What do you mean by "which requires to initialize it correctly to get
> expected netdev name." ?

I mean it cannot be renamed by udev rules as expected if `dev_port`
is not correctly initialized, because the second port doesn't match
'ATTR{dev_port}=="1"'.

RE: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Yinjun Zhang]

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
> 

RE: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Yinjun Zhang]

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.
"

Re: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Jakub Kicinski]

On Thu, 30 Mar 2023 01:40:30 +0000 Yinjun Zhang wrote:
> On Wed, 29 Mar 2023 12:22:27 -0700, Jakub Kicinski wrote:
> > On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote:  
> > > In some customized scenario, `dev_port` is used to rename netdev
> > > instead of `phys_port_name`, which requires to initialize it
> > > correctly to get expected netdev name.  
> > 
> > What do you mean by "which requires to initialize it correctly to get
> > expected netdev name." ?  
> 
> I mean it cannot be renamed by udev rules as expected if `dev_port`
> is not correctly initialized, because the second port doesn't match
> 'ATTR{dev_port}=="1"'.

Yes, but phys_port_name is still there, and can be used, right?
So why add another attr?

Re: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Jakub Kicinski]

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.
> "

RE: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Yinjun Zhang]

On Wed, 29 Mar 2023 19:38:31 -0700, Jakub Kicinski wrote:
> On Thu, 30 Mar 2023 01:40:30 +0000 Yinjun Zhang wrote:
> > On Wed, 29 Mar 2023 12:22:27 -0700, Jakub Kicinski wrote:
> > > On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote:
> > > > In some customized scenario, `dev_port` is used to rename netdev
> > > > instead of `phys_port_name`, which requires to initialize it
> > > > correctly to get expected netdev name.
> > >
> > > What do you mean by "which requires to initialize it correctly to get
> > > expected netdev name." ?
> >
> > I mean it cannot be renamed by udev rules as expected if `dev_port`
> > is not correctly initialized, because the second port doesn't match
> > 'ATTR{dev_port}=="1"'.
> 
> Yes, but phys_port_name is still there, and can be used, right?
> So why add another attr?

Yes, phys_port_name is still there. But some users prefer to use dev_port.
I don't add this new attr, it's already existed since 
3f85944fe207 ("net: Add sysfs file for port number").
I just make the attr's value correct.

RE: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Yinjun Zhang]

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.
> > "

Re: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Jakub Kicinski]

On Thu, 30 Mar 2023 02:52:22 +0000 Yinjun Zhang wrote:
> > Yes, but phys_port_name is still there, and can be used, right?
> > So why add another attr?  
> 
> Yes, phys_port_name is still there. But some users prefer to use dev_port.
> I don't add this new attr, it's already existed since 
> 3f85944fe207 ("net: Add sysfs file for port number").
> I just make the attr's value correct.

You're using a different ID than phys_port_name, as far as I can tell :(
When the port is not split will id == label, always?

Re: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Jakub Kicinski]

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?

RE: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Yinjun Zhang]

On Wed, 29 Mar 2023 20:10:29 -0700, Jakub Kicinski wrote:
> On Thu, 30 Mar 2023 02:52:22 +0000 Yinjun Zhang wrote:
> > > Yes, but phys_port_name is still there, and can be used, right?
> > > So why add another attr?
> >
> > Yes, phys_port_name is still there. But some users prefer to use dev_port.
> > I don't add this new attr, it's already existed since
> > 3f85944fe207 ("net: Add sysfs file for port number").
> > I just make the attr's value correct.
> 
> You're using a different ID than phys_port_name, as far as I can tell :(
> When the port is not split will id == label, always?

You got the point. We create netdevs according to the port sequence in
eth_table from management firmware. I think M-FW will make sure
the sequence matches the port label id.

RE: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Yinjun Zhang]

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.

Re: [PATCH net-next 1/2] nfp: initialize netdev's dev_port with correct id

[Jakub Kicinski]

On Thu, 30 Mar 2023 03:27:56 +0000 Yinjun Zhang wrote:
> > > Yes, phys_port_name is still there. But some users prefer to use dev_port.
> > > I don't add this new attr, it's already existed since
> > > 3f85944fe207 ("net: Add sysfs file for port number").
> > > I just make the attr's value correct.  
> > 
> > You're using a different ID than phys_port_name, as far as I can tell :(
> > When the port is not split will id == label, always?  
> 
> You got the point. We create netdevs according to the port sequence in
> eth_table from management firmware. I think M-FW will make sure
> the sequence matches the port label id.

Alright, then, with an improved commit message this patch should 
be fine.

Re: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Jakub Kicinski]

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.

RE: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Yinjun Zhang]

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.

Re: [PATCH net-next 2/2] nfp: separate the port's upper state with lower phy state

[Jakub Kicinski]

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>