diff mbox series

[bpf-next,v2,09/11] nfp: bpf: use extack support to improve debugging

Message ID 20180116020845.3496-10-jakub.kicinski@netronome.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series net: sched: add extack support for cls offload | expand

Commit Message

Jakub Kicinski Jan. 16, 2018, 2:08 a.m. UTC 
From: Quentin Monnet <quentin.monnet@netronome.com>

Use the recently added extack support for eBPF offload in the driver.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c    | 31 ++++++++++++++++++------
 drivers/net/ethernet/netronome/nfp/bpf/main.h    |  2 +-
 drivers/net/ethernet/netronome/nfp/bpf/offload.c | 24 +++++++++++-------
 3 files changed, 39 insertions(+), 18 deletions(-)

Comments

Jiri Pirko Jan. 16, 2018, 9:36 a.m. UTC | #1 
Tue, Jan 16, 2018 at 03:08:43AM CET, jakub.kicinski@netronome.com wrote:
>From: Quentin Monnet <quentin.monnet@netronome.com>
>
>Use the recently added extack support for eBPF offload in the driver.
>
>Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---

[...]


>@@ -303,7 +305,8 @@ static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
> 	/* Load up the JITed code */
> 	err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_BPF);
> 	if (err)
>-		nn_err(nn, "FW command error while loading BPF: %d\n", err);
>+		NL_SET_ERR_MSG_MOD(extack,
>+				   "FW command error while loading BPF");

One line please. Same for all others. Strings may overflow 80 cols.
Jakub Kicinski Jan. 16, 2018, 8:11 p.m. UTC | #2 
On Tue, 16 Jan 2018 10:36:01 +0100, Jiri Pirko wrote:
> >@@ -303,7 +305,8 @@ static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
> > 	/* Load up the JITed code */
> > 	err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_BPF);
> > 	if (err)
> >-		nn_err(nn, "FW command error while loading BPF: %d\n", err);
> >+		NL_SET_ERR_MSG_MOD(extack,
> >+				   "FW command error while loading BPF");  
> 
> One line please. Same for all others. Strings may overflow 80 cols.

Sorry, but this is the way I want things in the nfp driver.  If the
string would fit 80 chars placed on a new line, it should be placed 
on a new line.  If it doesn't fit anyway the new line is unnecessary.
This rules is adhered to throughout the driver (to the extent I'm able
to enforce it).

This is the diff of what you asked:

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index a638c3ab6b61..1e987bcbc086 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -126,20 +126,17 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
        int err;
 
        if (type != TC_SETUP_CLSBPF) {
-               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
-                                  "only offload of BPF classifiers supported");
+               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack, "only offload of BPF classifiers supported");
                return -EOPNOTSUPP;
        }
        if (!tc_can_offload_extack(nn->dp.netdev, cls_bpf->common.extack))
                return -EOPNOTSUPP;
        if (!nfp_net_ebpf_capable(nn)) {
-               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
-                                  "NFP firmware does not support eBPF offload");
+               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack, "NFP firmware does not support eBPF offload");
                return -EOPNOTSUPP;
        }
        if (cls_bpf->common.protocol != htons(ETH_P_ALL)) {
-               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
-                                  "only ETH_P_ALL supported as filter protocol");
+               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack, "only ETH_P_ALL supported as filter protocol");
                return -EOPNOTSUPP;
        }
        if (cls_bpf->common.chain_index)
@@ -148,8 +145,7 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
        /* Only support TC direct action */
        if (!cls_bpf->exts_integrated ||
            tcf_exts_has_actions(cls_bpf->exts)) {
-               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
-                                  "only direct action with no legacy actions supported");
+               NL_SET_ERR_MSG_MOD(cls_bpf->common.extack, "only direct action with no legacy actions supported");
                return -EOPNOTSUPP;
        }
 
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index 9c78a09cda24..181c90476854 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -305,8 +305,7 @@ nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog,
        /* Load up the JITed code */
        err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_BPF);
        if (err)
-               NL_SET_ERR_MSG_MOD(extack,
-                                  "FW command error while loading BPF");
+               NL_SET_ERR_MSG_MOD(extack, "FW command error while loading BPF");
 
        dma_unmap_single(nn->dp.dev, dma_addr, nfp_prog->prog_len * sizeof(u64),
                         DMA_TO_DEVICE);
@@ -325,8 +324,7 @@ nfp_net_bpf_start(struct nfp_net *nn, struct netlink_ext_ack *extack)
        nn_writel(nn, NFP_NET_CFG_CTRL, nn->dp.ctrl);
        err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_GEN);
        if (err)
-               NL_SET_ERR_MSG_MOD(extack,
-                                  "FW command error while enabling BPF");
+               NL_SET_ERR_MSG_MOD(extack, "FW command error while enabling BPF");
 }
 
 static int nfp_net_bpf_stop(struct nfp_net *nn)
@@ -359,8 +357,7 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
 
                cap = nn_readb(nn, NFP_NET_CFG_BPF_CAP);
                if (!(cap & NFP_NET_BPF_CAP_RELO)) {
-                       NL_SET_ERR_MSG_MOD(extack,
-                                          "FW does not support live reload");
+                       NL_SET_ERR_MSG_MOD(extack, "FW does not support live reload");
                        return -EBUSY;
                }
        }

strings overflow to new line on the left which is yucky and avoidable.

Please speak up if you feel strongly about this, otherwise I will
repost addressing only your other comment.

Thanks for reviewing!
David Ahern Jan. 17, 2018, 4:06 a.m. UTC | #3 
On 1/16/18 12:11 PM, Jakub Kicinski wrote:
> On Tue, 16 Jan 2018 10:36:01 +0100, Jiri Pirko wrote:
>>> @@ -303,7 +305,8 @@ static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
>>> 	/* Load up the JITed code */
>>> 	err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_BPF);
>>> 	if (err)
>>> -		nn_err(nn, "FW command error while loading BPF: %d\n", err);
>>> +		NL_SET_ERR_MSG_MOD(extack,
>>> +				   "FW command error while loading BPF");  
>>
>> One line please. Same for all others. Strings may overflow 80 cols.
> 
> Sorry, but this is the way I want things in the nfp driver.  If the
> string would fit 80 chars placed on a new line, it should be placed 
> on a new line.  If it doesn't fit anyway the new line is unnecessary.
> This rules is adhered to throughout the driver (to the extent I'm able
> to enforce it).
> 

+1
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index e8816ab8fb63..a638c3ab6b61 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -70,7 +70,7 @@  nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
 	if (prog && running && !xdp_running)
 		return -EBUSY;
 
-	ret = nfp_net_bpf_offload(nn, prog, running);
+	ret = nfp_net_bpf_offload(nn, prog, running, extack);
 	/* Stop offload if replace not possible */
 	if (ret && prog)
 		nfp_bpf_xdp_offload(app, nn, NULL, extack);
@@ -125,17 +125,31 @@  static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 	struct nfp_bpf_vnic *bv;
 	int err;
 
-	if (type != TC_SETUP_CLSBPF ||
-	    !tc_can_offload(nn->dp.netdev) ||
-	    !nfp_net_ebpf_capable(nn) ||
-	    cls_bpf->common.protocol != htons(ETH_P_ALL) ||
-	    cls_bpf->common.chain_index)
+	if (type != TC_SETUP_CLSBPF) {
+		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
+				   "only offload of BPF classifiers supported");
+		return -EOPNOTSUPP;
+	}
+	if (!tc_can_offload_extack(nn->dp.netdev, cls_bpf->common.extack))
+		return -EOPNOTSUPP;
+	if (!nfp_net_ebpf_capable(nn)) {
+		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
+				   "NFP firmware does not support eBPF offload");
+		return -EOPNOTSUPP;
+	}
+	if (cls_bpf->common.protocol != htons(ETH_P_ALL)) {
+		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
+				   "only ETH_P_ALL supported as filter protocol");
+		return -EOPNOTSUPP;
+	}
+	if (cls_bpf->common.chain_index)
 		return -EOPNOTSUPP;
 
 	/* Only support TC direct action */
 	if (!cls_bpf->exts_integrated ||
 	    tcf_exts_has_actions(cls_bpf->exts)) {
-		nn_err(nn, "only direct action with no legacy actions supported\n");
+		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
+				   "only direct action with no legacy actions supported");
 		return -EOPNOTSUPP;
 	}
 
@@ -152,7 +166,8 @@  static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 			return 0;
 	}
 
-	err = nfp_net_bpf_offload(nn, cls_bpf->prog, oldprog);
+	err = nfp_net_bpf_offload(nn, cls_bpf->prog, oldprog,
+				  cls_bpf->common.extack);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index b80e75a8ecda..80855d43b25e 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -334,7 +334,7 @@  struct nfp_net;
 int nfp_ndo_bpf(struct nfp_app *app, struct nfp_net *nn,
 		struct netdev_bpf *bpf);
 int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
-			bool old_prog);
+			bool old_prog, struct netlink_ext_ack *extack);
 
 struct nfp_insn_meta *
 nfp_bpf_goto_meta(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index e2859b2e9c6a..9c78a09cda24 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -271,7 +271,9 @@  int nfp_ndo_bpf(struct nfp_app *app, struct nfp_net *nn, struct netdev_bpf *bpf)
 	}
 }
 
-static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
+static int
+nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog,
+		 struct netlink_ext_ack *extack)
 {
 	struct nfp_prog *nfp_prog = prog->aux->offload->dev_priv;
 	unsigned int max_mtu;
@@ -281,7 +283,7 @@  static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
 
 	max_mtu = nn_readb(nn, NFP_NET_CFG_BPF_INL_MTU) * 64 - 32;
 	if (max_mtu < nn->dp.netdev->mtu) {
-		nn_info(nn, "BPF offload not supported with MTU larger than HW packet split boundary\n");
+		NL_SET_ERR_MSG_MOD(extack, "BPF offload not supported with MTU larger than HW packet split boundary");
 		return -EOPNOTSUPP;
 	}
 
@@ -303,7 +305,8 @@  static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
 	/* Load up the JITed code */
 	err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_BPF);
 	if (err)
-		nn_err(nn, "FW command error while loading BPF: %d\n", err);
+		NL_SET_ERR_MSG_MOD(extack,
+				   "FW command error while loading BPF");
 
 	dma_unmap_single(nn->dp.dev, dma_addr, nfp_prog->prog_len * sizeof(u64),
 			 DMA_TO_DEVICE);
@@ -312,7 +315,8 @@  static int nfp_net_bpf_load(struct nfp_net *nn, struct bpf_prog *prog)
 	return err;
 }
 
-static void nfp_net_bpf_start(struct nfp_net *nn)
+static void
+nfp_net_bpf_start(struct nfp_net *nn, struct netlink_ext_ack *extack)
 {
 	int err;
 
@@ -321,7 +325,8 @@  static void nfp_net_bpf_start(struct nfp_net *nn)
 	nn_writel(nn, NFP_NET_CFG_CTRL, nn->dp.ctrl);
 	err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_GEN);
 	if (err)
-		nn_err(nn, "FW command error while enabling BPF: %d\n", err);
+		NL_SET_ERR_MSG_MOD(extack,
+				   "FW command error while enabling BPF");
 }
 
 static int nfp_net_bpf_stop(struct nfp_net *nn)
@@ -336,7 +341,7 @@  static int nfp_net_bpf_stop(struct nfp_net *nn)
 }
 
 int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
-			bool old_prog)
+			bool old_prog, struct netlink_ext_ack *extack)
 {
 	int err;
 
@@ -354,7 +359,8 @@  int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
 
 		cap = nn_readb(nn, NFP_NET_CFG_BPF_CAP);
 		if (!(cap & NFP_NET_BPF_CAP_RELO)) {
-			nn_err(nn, "FW does not support live reload\n");
+			NL_SET_ERR_MSG_MOD(extack,
+					   "FW does not support live reload");
 			return -EBUSY;
 		}
 	}
@@ -366,12 +372,12 @@  int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
 	if (old_prog && !prog)
 		return nfp_net_bpf_stop(nn);
 
-	err = nfp_net_bpf_load(nn, prog);
+	err = nfp_net_bpf_load(nn, prog, extack);
 	if (err)
 		return err;
 
 	if (!old_prog)
-		nfp_net_bpf_start(nn);
+		nfp_net_bpf_start(nn, extack);
 
 	return 0;
 }