[RFC v2 0/6] use extack when setting up XDP

[RFC v2 0/6] use extack when setting up XDP

[Stephen Hemminger]

Try and make multiple drivers use extack for errors when
configuring XDP.

Still compile tested only.

Stephen Hemminger (6):
  bnxt: use extack for xdp error messages
  ixgbe: use extack for xdp errors
  i40e: use extack for bpf errors
  ixgebvf: report xdp errors through extack
  mlx4: report errors through extack
  mlx5: report XDP errors through extack

 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 26 +++++++++---------
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 15 ++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 27 ++++++++++++++-----
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++++---
 .../net/ethernet/mellanox/mlx4/en_netdev.c    | 14 ++++++----
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 22 ++++++++-------
 6 files changed, 74 insertions(+), 41 deletions(-)

-- 
2.17.1

[RFC v2 1/6] bnxt: use extack for xdp error messages

[Stephen Hemminger]

XDP has a netlink error message buffer available that should
be used for errors instead of console logging.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 0184ef6f05a7..a9f6a3a66b4c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -150,19 +150,21 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 }
 
 /* Under rtnl_lock */
-static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
+static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog,
+			struct netlink_ext_ack *extack)
 {
 	struct net_device *dev = bp->dev;
 	int tx_xdp = 0, rc, tc;
 	struct bpf_prog *old;
 
 	if (prog && bp->dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
-		netdev_warn(dev, "MTU %d larger than largest XDP supported MTU %d.\n",
-			    bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
+		NL_SET_ERR_MSG_MOD(extack,
+				   "MTU larger than largest XDP supported MTU");
 		return -EOPNOTSUPP;
 	}
 	if (!(bp->flags & BNXT_FLAG_SHARED_RINGS)) {
-		netdev_warn(dev, "ethtool rx/tx channels must be combined to support XDP.\n");
+		NL_SET_ERR_MSG_MOD(extack,
+				   "ethtool rx/tx channels must be combined to support XDP");
 		return -EOPNOTSUPP;
 	}
 	if (prog)
@@ -174,7 +176,8 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
 	rc = bnxt_check_rings(bp, bp->tx_nr_rings_per_tc, bp->rx_nr_rings,
 			      true, tc, tx_xdp);
 	if (rc) {
-		netdev_warn(dev, "Unable to reserve enough TX rings to support XDP.\n");
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Unable to reserve enough TX rings to support XDP.\n");
 		return rc;
 	}
 	if (netif_running(dev))
@@ -211,19 +214,16 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
 int bnxt_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	struct bnxt *bp = netdev_priv(dev);
-	int rc;
 
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		rc = bnxt_xdp_set(bp, xdp->prog);
-		break;
+		return bnxt_xdp_set(bp, xdp->prog, xdp->extack);
+
 	case XDP_QUERY_PROG:
 		xdp->prog_id = bp->xdp_prog ? bp->xdp_prog->aux->id : 0;
-		rc = 0;
-		break;
+		return 0;
 	default:
-		rc = -EINVAL;
-		break;
+		NL_SET_ERR_MSG_MOD(xdp->extack, "Unsupported XDP command");
+		return -EINVAL;
 	}
-	return rc;
 }
-- 
2.17.1

[RFC v2 2/6] ixgbe: use extack for xdp errors

[Stephen Hemminger]

Give a reason for returning error for bpf setup.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 27 ++++++++++++++-----
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a4e7584a50cb..d5bac44df793 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10224,18 +10224,25 @@ ixgbe_features_check(struct sk_buff *skb, struct net_device *dev,
 	return features;
 }
 
-static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
+static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
+			   struct netlink_ext_ack *extack)
 {
 	int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
 	struct ixgbe_adapter *adapter = netdev_priv(dev);
 	struct bpf_prog *old_prog;
 	bool need_reset;
 
-	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "XDP not supported with SRIOV enabled");
 		return -EINVAL;
+	}
 
-	if (adapter->flags & IXGBE_FLAG_DCB_ENABLED)
+	if (adapter->flags & IXGBE_FLAG_DCB_ENABLED) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "XDP not supported with DCB enabled");
 		return -EINVAL;
+	}
 
 	/* verify ixgbe ring attributes are sufficient for XDP */
 	for (i = 0; i < adapter->num_rx_queues; i++) {
@@ -10244,12 +10251,17 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 		if (ring_is_rsc_enabled(ring))
 			return -EINVAL;
 
-		if (frame_size > ixgbe_rx_bufsz(ring))
+		if (frame_size > ixgbe_rx_bufsz(ring)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "XDP does not support multiple buffers");
 			return -EINVAL;
+		}
 	}
 
-	if (nr_cpu_ids > MAX_XDP_QUEUES)
+	if (nr_cpu_ids > MAX_XDP_QUEUES) {
+		NL_SET_ERR_MSG_MOD(extack, "number of cpus > MAX_XDP_QUEUES");
 		return -ENOMEM;
+	}
 
 	old_prog = xchg(&adapter->xdp_prog, prog);
 	need_reset = (!!prog != !!old_prog);
@@ -10260,7 +10272,7 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 
 		if (err) {
 			rcu_assign_pointer(adapter->xdp_prog, old_prog);
-			return -EINVAL;
+			return err;
 		}
 	} else {
 		for (i = 0; i < adapter->num_rx_queues; i++)
@@ -10288,7 +10300,7 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return ixgbe_xdp_setup(dev, xdp->prog);
+		return ixgbe_xdp_setup(dev, xdp->prog, xdp->extack);
 	case XDP_QUERY_PROG:
 		xdp->prog_id = adapter->xdp_prog ?
 			adapter->xdp_prog->aux->id : 0;
@@ -10298,6 +10310,7 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 					    xdp->xsk.queue_id);
 
 	default:
+		NL_SET_ERR_MSG_MOD(xdp->extack, "Unknown XDP command");
 		return -EINVAL;
 	}
 }
-- 
2.17.1

[RFC v2 3/6] i40e: use extack for bpf errors

[Stephen Hemminger]

If ndo_bpf fails fill in error string with reason.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index da62218eb70a..a6a5866d79a8 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -11831,7 +11831,8 @@ static netdev_features_t i40e_features_check(struct sk_buff *skb,
  * @prog: XDP program
  **/
 static int i40e_xdp_setup(struct i40e_vsi *vsi,
-			  struct bpf_prog *prog)
+			  struct bpf_prog *prog,
+			  struct netlink_ext_ack *extack)
 {
 	int frame_size = vsi->netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
 	struct i40e_pf *pf = vsi->back;
@@ -11840,8 +11841,11 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
 	int i;
 
 	/* Don't allow frames that span over multiple buffers */
-	if (frame_size > vsi->rx_buf_len)
+	if (frame_size > vsi->rx_buf_len) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "XDP does not support multiple buffers");
 		return -EINVAL;
+	}
 
 	if (!i40e_enabled_xdp_vsi(vsi) && !prog)
 		return 0;
@@ -12140,12 +12144,14 @@ static int i40e_xdp(struct net_device *dev,
 	struct i40e_netdev_priv *np = netdev_priv(dev);
 	struct i40e_vsi *vsi = np->vsi;
 
-	if (vsi->type != I40E_VSI_MAIN)
+	if (vsi->type != I40E_VSI_MAIN) {
+		NL_SET_ERR_MSG_MOD(xdp->extack, "XDP not allowed on VF");
 		return -EINVAL;
+	}
 
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return i40e_xdp_setup(vsi, xdp->prog);
+		return i40e_xdp_setup(vsi, xdp->prog, xdp->extack);
 	case XDP_QUERY_PROG:
 		xdp->prog_id = vsi->xdp_prog ? vsi->xdp_prog->aux->id : 0;
 		return 0;
@@ -12153,6 +12159,7 @@ static int i40e_xdp(struct net_device *dev,
 		return i40e_xsk_umem_setup(vsi, xdp->xsk.umem,
 					   xdp->xsk.queue_id);
 	default:
+		NL_SET_ERR_MSG_MOD(xdp->extack, "Unknown XDP command");
 		return -EINVAL;
 	}
 }
-- 
2.17.1

[RFC v2 4/6] ixgebvf: report xdp errors through extack

[Stephen Hemminger]

In case of XDP errors report error string through extack.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 49e23afa05a2..c8c96adc032d 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -4436,7 +4436,8 @@ ixgbevf_features_check(struct sk_buff *skb, struct net_device *dev,
 	return features;
 }
 
-static int ixgbevf_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
+static int ixgbevf_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
+			     struct netlink_ext_ack *extack)
 {
 	int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
 	struct ixgbevf_adapter *adapter = netdev_priv(dev);
@@ -4446,8 +4447,11 @@ static int ixgbevf_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct ixgbevf_ring *ring = adapter->rx_ring[i];
 
-		if (frame_size > ixgbevf_rx_bufsz(ring))
+		if (frame_size > ixgbevf_rx_bufsz(ring)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "XDP does not support multiple buffers");
 			return -EINVAL;
+		}
 	}
 
 	old_prog = xchg(&adapter->xdp_prog, prog);
@@ -4483,12 +4487,13 @@ static int ixgbevf_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return ixgbevf_xdp_setup(dev, xdp->prog);
+		return ixgbevf_xdp_setup(dev, xdp->prog, xdp->extack);
 	case XDP_QUERY_PROG:
 		xdp->prog_id = adapter->xdp_prog ?
 			       adapter->xdp_prog->aux->id : 0;
 		return 0;
 	default:
+		NL_SET_ERR_MSG_MOD(xdp->extack, "Unsupported XDP command");
 		return -EINVAL;
 	}
 }
-- 
2.17.1

[RFC v2 5/6] mlx4: report errors through extack

[Stephen Hemminger]

XDP errors should be reported via extack (back to command)
rather than on console.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index c1438ae52a11..75ae06c82294 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2779,7 +2779,8 @@ static int mlx4_en_set_tx_maxrate(struct net_device *dev, int queue_index, u32 m
 	return err;
 }
 
-static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
+static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+			struct netlink_ext_ack *extack)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	struct mlx4_en_dev *mdev = priv->mdev;
@@ -2816,8 +2817,10 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 		return 0;
 	}
 
-	if (!mlx4_en_check_xdp_mtu(dev, dev->mtu))
+	if (!mlx4_en_check_xdp_mtu(dev, dev->mtu)) {
+		NL_SET_ERR_MSG_MOD(extack, "MTU exceeds support XDP maximum");
 		return -EOPNOTSUPP;
+	}
 
 	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
 	if (!tmp)
@@ -2870,8 +2873,8 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	if (port_up) {
 		err = mlx4_en_start_port(dev);
 		if (err) {
-			en_err(priv, "Failed starting port %d for XDP change\n",
-			       priv->port);
+			NL_SET_ERR_MSG_MOD(extack,
+				       "Failed starting port for XDP change");
 			queue_work(mdev->workqueue, &priv->watchdog_task);
 		}
 	}
@@ -2908,11 +2911,12 @@ static int mlx4_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return mlx4_xdp_set(dev, xdp->prog);
+		return mlx4_xdp_set(dev, xdp->prog, xdp->extack);
 	case XDP_QUERY_PROG:
 		xdp->prog_id = mlx4_xdp_query(dev);
 		return 0;
 	default:
+		NL_SET_ERR_MSG_MOD(xdp->extack, "Unsupported XDP command");
 		return -EINVAL;
 	}
 }
-- 
2.17.1

[RFC v2 6/6] mlx5: report XDP errors through extack

[Stephen Hemminger]

In case of errors in setting up XDP, report error through
extack string rather than console log.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 22 +++++++++++--------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index e5f74eb986b3..555d72fb6b40 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -4177,18 +4177,20 @@ static void mlx5e_tx_timeout(struct net_device *dev)
 	queue_work(priv->wq, &priv->tx_timeout_work);
 }
 
-static int mlx5e_xdp_allowed(struct mlx5e_priv *priv, struct bpf_prog *prog)
+static int mlx5e_xdp_allowed(struct mlx5e_priv *priv, struct bpf_prog *prog,
+			     struct netlink_ext_ack *extack)
 {
-	struct net_device *netdev = priv->netdev;
 	struct mlx5e_channels new_channels = {};
 
 	if (priv->channels.params.lro_en) {
-		netdev_warn(netdev, "can't set XDP while LRO is on, disable LRO first\n");
+		NL_SET_ERR_MSG_MOD(extack,
+			       "can't set XDP while LRO is on, disable LRO first");
 		return -EINVAL;
 	}
 
 	if (MLX5_IPSEC_DEV(priv->mdev)) {
-		netdev_warn(netdev, "can't set XDP with IPSec offload\n");
+		NL_SET_ERR_MSG_MOD(extack,
+			       "can't set XDP with IPSec offload");
 		return -EINVAL;
 	}
 
@@ -4196,15 +4198,16 @@ static int mlx5e_xdp_allowed(struct mlx5e_priv *priv, struct bpf_prog *prog)
 	new_channels.params.xdp_prog = prog;
 
 	if (!mlx5e_rx_is_linear_skb(priv->mdev, &new_channels.params)) {
-		netdev_warn(netdev, "XDP is not allowed with MTU(%d) > %d\n",
-			    new_channels.params.sw_mtu, MLX5E_XDP_MAX_MTU);
+		NL_SET_ERR_MSG_MOD(extack,
+			       "XDP is not allowed with large MTU");
 		return -EINVAL;
 	}
 
 	return 0;
 }
 
-static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
+static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog,
+			 struct netlink_ext_ack *extack)
 {
 	struct mlx5e_priv *priv = netdev_priv(netdev);
 	struct bpf_prog *old_prog;
@@ -4215,7 +4218,7 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 	mutex_lock(&priv->state_lock);
 
 	if (prog) {
-		err = mlx5e_xdp_allowed(priv, prog);
+		err = mlx5e_xdp_allowed(priv, prog, extack);
 		if (err)
 			goto unlock;
 	}
@@ -4297,11 +4300,12 @@ static int mlx5e_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return mlx5e_xdp_set(dev, xdp->prog);
+		return mlx5e_xdp_set(dev, xdp->prog, xdp->extack);
 	case XDP_QUERY_PROG:
 		xdp->prog_id = mlx5e_xdp_query(dev);
 		return 0;
 	default:
+		NL_SET_ERR_MSG_MOD(xdp->extack, "Unsupported XDP command");
 		return -EINVAL;
 	}
 }
-- 
2.17.1

Re: [RFC v2 3/6] i40e: use extack for bpf errors

[Jakub Kicinski]

On Thu, 28 Feb 2019 13:54:38 -0800, Stephen Hemminger wrote:
> @@ -12140,12 +12144,14 @@ static int i40e_xdp(struct net_device *dev,
>  	struct i40e_netdev_priv *np = netdev_priv(dev);
>  	struct i40e_vsi *vsi = np->vsi;
>  
> -	if (vsi->type != I40E_VSI_MAIN)
> +	if (vsi->type != I40E_VSI_MAIN) {
> +		NL_SET_ERR_MSG_MOD(xdp->extack, "XDP not allowed on VF");

Is that right?  Intel tends to have separate drivers for VFs, I think
the i40evf got renamed to iavf.

I think it would be a good idea to CC maintainers on the driver patches.

>  		return -EINVAL;
> +	}
>  

Re: [RFC v2 5/6] mlx4: report errors through extack

[Jakub Kicinski]

On Thu, 28 Feb 2019 13:54:40 -0800, Stephen Hemminger wrote:
> @@ -2870,8 +2873,8 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>  	if (port_up) {
>  		err = mlx4_en_start_port(dev);
>  		if (err) {
> -			en_err(priv, "Failed starting port %d for XDP change\n",
> -			       priv->port);
> +			NL_SET_ERR_MSG_MOD(extack,
> +				       "Failed starting port for XDP change");

alignment is off now in a few spots

>  			queue_work(mdev->workqueue, &priv->watchdog_task);
>  		}
>  	}

Re: [RFC v2 3/6] i40e: use extack for bpf errors

[Maciej Fijalkowski]

On Thu, 28 Feb 2019 19:06:45 -0800
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Thu, 28 Feb 2019 13:54:38 -0800, Stephen Hemminger wrote:
> > @@ -12140,12 +12144,14 @@ static int i40e_xdp(struct net_device *dev,
> >  	struct i40e_netdev_priv *np = netdev_priv(dev);
> >  	struct i40e_vsi *vsi = np->vsi;
> >  
> > -	if (vsi->type != I40E_VSI_MAIN)
> > +	if (vsi->type != I40E_VSI_MAIN) {
> > +		NL_SET_ERR_MSG_MOD(xdp->extack, "XDP not allowed on VF");  
> 
> Is that right?  Intel tends to have separate drivers for VFs, I think
> the i40evf got renamed to iavf.
>
Good catch, this sanity check is to make sure that XDP is being enabled on main
VSI of PF, not for example the VSI dedicated for Flow Director management
(I40E_VSI_FDIR). Besides that, vsi->type != I40E_VSI_MAIN doesn't yield the
I40E_VSI_SRIOV type.

So it would be better to have an error message like "XDP is not allowed on VSIs
other than main VSI".

Thoughts?

> I think it would be a good idea to CC maintainers on the driver patches.
> 
> >  		return -EINVAL;
> > +	}
> >    
> 

Re: [RFC v2 3/6] i40e: use extack for bpf errors

[Jakub Kicinski]

On Fri, 1 Mar 2019 12:31:08 +0100, Maciej Fijalkowski wrote:
> On Thu, 28 Feb 2019 19:06:45 -0800
> Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> 
> > On Thu, 28 Feb 2019 13:54:38 -0800, Stephen Hemminger wrote:  
> > > @@ -12140,12 +12144,14 @@ static int i40e_xdp(struct net_device *dev,
> > >  	struct i40e_netdev_priv *np = netdev_priv(dev);
> > >  	struct i40e_vsi *vsi = np->vsi;
> > >  
> > > -	if (vsi->type != I40E_VSI_MAIN)
> > > +	if (vsi->type != I40E_VSI_MAIN) {
> > > +		NL_SET_ERR_MSG_MOD(xdp->extack, "XDP not allowed on VF");    
> > 
> > Is that right?  Intel tends to have separate drivers for VFs, I think
> > the i40evf got renamed to iavf.
> >  
> Good catch, this sanity check is to make sure that XDP is being enabled on main
> VSI of PF, not for example the VSI dedicated for Flow Director management
> (I40E_VSI_FDIR). Besides that, vsi->type != I40E_VSI_MAIN doesn't yield the
> I40E_VSI_SRIOV type.
> 
> So it would be better to have an error message like "XDP is not allowed on VSIs
> other than main VSI".
> 
> Thoughts?

Do the other VSI types have netdevs?  If they do this may be hard to
understand (unless Intel's manuals refer to VSI and users know what
that is).

If there is no netdev on other VSIs perhaps this "should never happen"
and we can convert it to a WARN_ON()?

> > I think it would be a good idea to CC maintainers on the driver patches.
> >   
> > >  		return -EINVAL;
> > > +	}
> > >      
> >   
> 

Re: [RFC v2 3/6] i40e: use extack for bpf errors

[Stephen Hemminger]

On Fri, 1 Mar 2019 08:28:49 -0800
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Fri, 1 Mar 2019 12:31:08 +0100, Maciej Fijalkowski wrote:
> > On Thu, 28 Feb 2019 19:06:45 -0800
> > Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> >   
> > > On Thu, 28 Feb 2019 13:54:38 -0800, Stephen Hemminger wrote:    
> > > > @@ -12140,12 +12144,14 @@ static int i40e_xdp(struct net_device *dev,
> > > >  	struct i40e_netdev_priv *np = netdev_priv(dev);
> > > >  	struct i40e_vsi *vsi = np->vsi;
> > > >  
> > > > -	if (vsi->type != I40E_VSI_MAIN)
> > > > +	if (vsi->type != I40E_VSI_MAIN) {
> > > > +		NL_SET_ERR_MSG_MOD(xdp->extack, "XDP not allowed on VF");      
> > > 
> > > Is that right?  Intel tends to have separate drivers for VFs, I think
> > > the i40evf got renamed to iavf.
> > >    
> > Good catch, this sanity check is to make sure that XDP is being enabled on main
> > VSI of PF, not for example the VSI dedicated for Flow Director management
> > (I40E_VSI_FDIR). Besides that, vsi->type != I40E_VSI_MAIN doesn't yield the
> > I40E_VSI_SRIOV type.
> > 
> > So it would be better to have an error message like "XDP is not allowed on VSIs
> > other than main VSI".
> > 
> > Thoughts?  
> 
> Do the other VSI types have netdevs?  If they do this may be hard to
> understand (unless Intel's manuals refer to VSI and users know what
> that is).
> 
> If there is no netdev on other VSIs perhaps this "should never happen"
> and we can convert it to a WARN_ON()?

I was just making a best guess based on similar checks elsewhere in the
code as to the correct error message

Re: [RFC v2 3/6] i40e: use extack for bpf errors

[Jakub Kicinski]

On Fri, 1 Mar 2019 13:55:53 -0800, Stephen Hemminger wrote:
> On Fri, 1 Mar 2019 08:28:49 -0800
> Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> 
> > On Fri, 1 Mar 2019 12:31:08 +0100, Maciej Fijalkowski wrote:  
> > > On Thu, 28 Feb 2019 19:06:45 -0800
> > > Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> > >     
> > > > On Thu, 28 Feb 2019 13:54:38 -0800, Stephen Hemminger wrote:      
> > > > > @@ -12140,12 +12144,14 @@ static int i40e_xdp(struct net_device *dev,
> > > > >  	struct i40e_netdev_priv *np = netdev_priv(dev);
> > > > >  	struct i40e_vsi *vsi = np->vsi;
> > > > >  
> > > > > -	if (vsi->type != I40E_VSI_MAIN)
> > > > > +	if (vsi->type != I40E_VSI_MAIN) {
> > > > > +		NL_SET_ERR_MSG_MOD(xdp->extack, "XDP not allowed on VF");        
> > > > 
> > > > Is that right?  Intel tends to have separate drivers for VFs, I think
> > > > the i40evf got renamed to iavf.
> > > >      
> > > Good catch, this sanity check is to make sure that XDP is being enabled on main
> > > VSI of PF, not for example the VSI dedicated for Flow Director management
> > > (I40E_VSI_FDIR). Besides that, vsi->type != I40E_VSI_MAIN doesn't yield the
> > > I40E_VSI_SRIOV type.
> > > 
> > > So it would be better to have an error message like "XDP is not allowed on VSIs
> > > other than main VSI".
> > > 
> > > Thoughts?    
> > 
> > Do the other VSI types have netdevs?  If they do this may be hard to
> > understand (unless Intel's manuals refer to VSI and users know what
> > that is).
> > 
> > If there is no netdev on other VSIs perhaps this "should never happen"
> > and we can convert it to a WARN_ON()?  
> 
> I was just making a best guess based on similar checks elsewhere in the
> code as to the correct error message

Digging into this I think there are more netdevs, so perhaps
"XDP is only allowed on the main netdev instance"?  Hopefully 
Intel folks can advise even better.

Re: [RFC v2 5/6] mlx4: report errors through extack

[Tariq Toukan]

On 2/28/2019 11:54 PM, Stephen Hemminger wrote:
> XDP errors should be reported via extack (back to command)
> rather than on console.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index c1438ae52a11..75ae06c82294 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2779,7 +2779,8 @@ static int mlx4_en_set_tx_maxrate(struct net_device *dev, int queue_index, u32 m
>   	return err;
>   }
>   
> -static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
> +static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> +			struct netlink_ext_ack *extack)
>   {
>   	struct mlx4_en_priv *priv = netdev_priv(dev);
>   	struct mlx4_en_dev *mdev = priv->mdev;
> @@ -2816,8 +2817,10 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   		return 0;
>   	}
>   
> -	if (!mlx4_en_check_xdp_mtu(dev, dev->mtu))
> +	if (!mlx4_en_check_xdp_mtu(dev, dev->mtu)) {
> +		NL_SET_ERR_MSG_MOD(extack, "MTU exceeds support XDP maximum");
>   		return -EOPNOTSUPP;
> +	}
>   
>   	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
>   	if (!tmp)
> @@ -2870,8 +2873,8 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   	if (port_up) {
>   		err = mlx4_en_start_port(dev);
>   		if (err) {
> -			en_err(priv, "Failed starting port %d for XDP change\n",
> -			       priv->port);
> +			NL_SET_ERR_MSG_MOD(extack,
> +				       "Failed starting port for XDP change");

Any reason not to keep the port number? It used to be printed.

>   			queue_work(mdev->workqueue, &priv->watchdog_task);
>   		}
>   	}
> @@ -2908,11 +2911,12 @@ static int mlx4_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>   {
>   	switch (xdp->command) {
>   	case XDP_SETUP_PROG:
> -		return mlx4_xdp_set(dev, xdp->prog);
> +		return mlx4_xdp_set(dev, xdp->prog, xdp->extack);
>   	case XDP_QUERY_PROG:
>   		xdp->prog_id = mlx4_xdp_query(dev);
>   		return 0;
>   	default:
> +		NL_SET_ERR_MSG_MOD(xdp->extack, "Unsupported XDP command");
>   		return -EINVAL;
>   	}
>   }
>