* Re: [PATCH v2] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-05 13:39 ` Roi Dayan
@ 2021-09-06 9:29 ` Abhiram R N
2021-09-14 10:06 ` [PATCH net-next v3] " Abhiram R N
2021-09-14 11:50 ` [PATCH net-next v4] " Abhiram R N
2 siblings, 0 replies; 13+ messages in thread
From: Abhiram R N @ 2021-09-06 9:29 UTC (permalink / raw)
To: Roi Dayan
Cc: Abhiram R N, saeedm, arn, hakhande, Leon Romanovsky,
David S. Miller, Jakub Kicinski, netdev, linux-rdma,
linux-kernel
Hi,
Thanks for your feedback. Please see replies inline.
On Sun, Sep 05, 2021 at 04:39:41PM +0300, Roi Dayan wrote:
>
>
> On 2021-08-31 11:44 AM, Abhiram R N wrote:
> > As multiple places EOPNOTSUPP and EINVAL is returned from driver
> > it becomes difficult to understand the reason only with error code.
> > With the netlink extack message exact reason will be known and will
> > aid in debugging.
> >
> > Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
> > 1 file changed, 76 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index d273758255c3..911eab2acaad 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -1894,8 +1894,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
> > bool needs_mapping, sets_mapping;
> > int err;
> > - if (!mlx5e_is_eswitch_flow(flow))
> > + if (!mlx5e_is_eswitch_flow(flow)) {
> > + NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
> > return -EOPNOTSUPP;
> > + }
> > needs_mapping = !!flow->attr->chain;
> > sets_mapping = !flow->attr->chain && flow_has_tc_fwd_action(f);
> > @@ -2267,8 +2269,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> > addr_type = match.key->addr_type;
> > /* the HW doesn't support frag first/later */
> > - if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
> > + if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
> > + NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
> > return -EOPNOTSUPP;
> > + }
> > if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
> > MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
> > @@ -2435,8 +2439,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> > switch (ip_proto) {
> > case IPPROTO_ICMP:
> > if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> > - MLX5_FLEX_PROTO_ICMP))
> > + MLX5_FLEX_PROTO_ICMP)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Match on Flex protocols for ICMP not supported");
>
> can you add "is" as ".. is not supported"?
Sure. Will change it.
>
> > return -EOPNOTSUPP;
> > + }
> > MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
> > match.mask->type);
> > MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
> > @@ -2448,8 +2455,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> > break;
> > case IPPROTO_ICMPV6:
> > if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> > - MLX5_FLEX_PROTO_ICMPV6))
> > + MLX5_FLEX_PROTO_ICMPV6)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Match on Flex protocols for ICMPV6 not supported");
>
> can you add "is" as ".. is not supported"?
Sure. will change it.
>
> also, can you submit this patch to net-next and not net please?
> it's not really fixing an issue and will help avoid conflicts with
> changes we plan on this file.
> thanks
Sure. I will submit it to net-next. I checked now and net-next is closed
it says (http://vger.kernel.org/~davem/net-next.html).
I will wait for it to be open and then submit there.
>
>
> > return -EOPNOTSUPP;
> > + }
> > MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
> > match.mask->type);
> > MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
> > @@ -2555,15 +2565,19 @@ static int pedit_header_offsets[] = {
> > #define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
> > static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
> > - struct pedit_headers_action *hdrs)
> > + struct pedit_headers_action *hdrs,
> > + struct netlink_ext_ack *extack)
> > {
> > u32 *curr_pmask, *curr_pval;
> > curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
> > curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
> > - if (*curr_pmask & mask) /* disallow acting twice on the same location */
> > + if (*curr_pmask & mask) { /* disallow acting twice on the same location */
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "curr_pmask and new mask same. Acting twice on same location");
> > goto out_err;
> > + }
> > *curr_pmask |= mask;
> > *curr_pval |= (val & mask);
> > @@ -2893,7 +2907,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
> > val = act->mangle.val;
> > offset = act->mangle.offset;
> > - err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
> > + err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
> > if (err)
> > goto out_err;
> > @@ -2913,8 +2927,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
> > u32 mask, val, offset;
> > u32 *p;
> > - if (act->id != FLOW_ACTION_MANGLE)
> > + if (act->id != FLOW_ACTION_MANGLE) {
> > + NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
> > return -EOPNOTSUPP;
> > + }
> > if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
> > NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
> > @@ -3363,12 +3379,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> > u32 action = 0;
> > int err, i;
> > - if (!flow_action_has_entries(flow_action))
> > + if (!flow_action_has_entries(flow_action)) {
> > + NL_SET_ERR_MSG_MOD(extack, "Flow Action doesn't have any entries");
> > return -EINVAL;
> > + }
> > if (!flow_action_hw_stats_check(flow_action, extack,
> > - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> > + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> > + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check not supported");
> > return -EOPNOTSUPP;
> > + }
> > nic_attr = attr->nic_attr;
> > @@ -3459,7 +3479,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> > flow_flag_set(flow, CT);
> > break;
> > default:
> > - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "The offload action is not supported in NIC action");
> > return -EOPNOTSUPP;
> > }
> > }
> > @@ -3514,19 +3535,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
> > static int parse_tc_vlan_action(struct mlx5e_priv *priv,
> > const struct flow_action_entry *act,
> > struct mlx5_esw_flow_attr *attr,
> > - u32 *action)
> > + u32 *action,
> > + struct netlink_ext_ack *extack)
> > {
> > u8 vlan_idx = attr->total_vlan;
> > - if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
> > + if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
> > + NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
> > return -EOPNOTSUPP;
> > + }
> > switch (act->id) {
> > case FLOW_ACTION_VLAN_POP:
> > if (vlan_idx) {
> > if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> > - MLX5_FS_VLAN_DEPTH))
> > + MLX5_FS_VLAN_DEPTH)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "vlan pop action is not supported");
> > return -EOPNOTSUPP;
> > + }
> > *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
> > } else {
> > @@ -3542,20 +3569,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
> > if (vlan_idx) {
> > if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> > - MLX5_FS_VLAN_DEPTH))
> > + MLX5_FS_VLAN_DEPTH)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "vlan push action is not supported for vlan depth > 1");
> > return -EOPNOTSUPP;
> > + }
> > *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
> > } else {
> > if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
> > (act->vlan.proto != htons(ETH_P_8021Q) ||
> > - act->vlan.prio))
> > + act->vlan.prio)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "vlan push action is not supported");
> > return -EOPNOTSUPP;
> > + }
> > *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
> > }
> > break;
> > default:
> > + NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
> > return -EINVAL;
> > }
> > @@ -3589,7 +3623,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
> > static int add_vlan_push_action(struct mlx5e_priv *priv,
> > struct mlx5_flow_attr *attr,
> > struct net_device **out_dev,
> > - u32 *action)
> > + u32 *action,
> > + struct netlink_ext_ack *extack)
> > {
> > struct net_device *vlan_dev = *out_dev;
> > struct flow_action_entry vlan_act = {
> > @@ -3600,7 +3635,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> > };
> > int err;
> > - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> > + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> > if (err)
> > return err;
> > @@ -3611,14 +3646,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> > return -ENODEV;
> > if (is_vlan_dev(*out_dev))
> > - err = add_vlan_push_action(priv, attr, out_dev, action);
> > + err = add_vlan_push_action(priv, attr, out_dev, action, extack);
> > return err;
> > }
> > static int add_vlan_pop_action(struct mlx5e_priv *priv,
> > struct mlx5_flow_attr *attr,
> > - u32 *action)
> > + u32 *action,
> > + struct netlink_ext_ack *extack)
> > {
> > struct flow_action_entry vlan_act = {
> > .id = FLOW_ACTION_VLAN_POP,
> > @@ -3628,7 +3664,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
> > nest_level = attr->parse_attr->filter_dev->lower_level -
> > priv->netdev->lower_level;
> > while (nest_level--) {
> > - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> > + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> > if (err)
> > return err;
> > }
> > @@ -3751,12 +3787,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> > int err, i, if_count = 0;
> > bool mpls_push = false;
> > - if (!flow_action_has_entries(flow_action))
> > + if (!flow_action_has_entries(flow_action)) {
> > + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> > return -EINVAL;
> > + }
> > if (!flow_action_hw_stats_check(flow_action, extack,
> > - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> > + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> > + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
> > return -EOPNOTSUPP;
> > + }
> > esw_attr = attr->esw_attr;
> > parse_attr = attr->parse_attr;
> > @@ -3900,14 +3940,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> > if (is_vlan_dev(out_dev)) {
> > err = add_vlan_push_action(priv, attr,
> > &out_dev,
> > - &action);
> > + &action, extack);
> > if (err)
> > return err;
> > }
> > if (is_vlan_dev(parse_attr->filter_dev)) {
> > err = add_vlan_pop_action(priv, attr,
> > - &action);
> > + &action, extack);
> > if (err)
> > return err;
> > }
> > @@ -3953,10 +3993,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> > break;
> > case FLOW_ACTION_TUNNEL_ENCAP:
> > info = act->tunnel;
> > - if (info)
> > + if (info) {
> > encap = true;
> > - else
> > + } else {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Zero tunnel attributes is not supported");
> > return -EOPNOTSUPP;
> > + }
> > break;
> > case FLOW_ACTION_VLAN_PUSH:
> > @@ -3970,7 +4013,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> > act, parse_attr, hdrs,
> > &action, extack);
> > } else {
> > - err = parse_tc_vlan_action(priv, act, esw_attr, &action);
> > + err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
> > }
> > if (err)
> > return err;
> > @@ -4023,7 +4066,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> > flow_flag_set(flow, SAMPLE);
> > break;
> > default:
> > - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "The offload action is not supported in FDB action");
> > return -EOPNOTSUPP;
> > }
> > }
> > @@ -4731,8 +4775,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
> > return -EOPNOTSUPP;
> > }
> > - if (!flow_action_basic_hw_stats_check(flow_action, extack))
> > + if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
> > + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
> > return -EOPNOTSUPP;
> > + }
> > flow_action_for_each(i, act, flow_action) {
> > switch (act->id) {
> >
Thanks & Regards,
Abhiram R N
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH net-next v3] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-05 13:39 ` Roi Dayan
2021-09-06 9:29 ` Abhiram R N
@ 2021-09-14 10:06 ` Abhiram R N
2021-09-14 11:50 ` [PATCH net-next v4] " Abhiram R N
2 siblings, 0 replies; 13+ messages in thread
From: Abhiram R N @ 2021-09-14 10:06 UTC (permalink / raw)
To: roid
Cc: arn, hakhande, saeedm, Abhiram R N, Leon Romanovsky,
David S. Miller, Jakub Kicinski, netdev, linux-rdma,
linux-kernel
As multiple places EOPNOTSUPP and EINVAL is returned from driver
it becomes difficult to understand the reason only with error code.
With the netlink extack message exact reason will be known and will
aid in debugging.
Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
---
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
1 file changed, 76 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index d273758255c3..3096f9eb812b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1894,8 +1894,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
bool needs_mapping, sets_mapping;
int err;
- if (!mlx5e_is_eswitch_flow(flow))
+ if (!mlx5e_is_eswitch_flow(flow)) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
return -EOPNOTSUPP;
+ }
needs_mapping = !!flow->attr->chain;
sets_mapping = !flow->attr->chain && flow_has_tc_fwd_action(f);
@@ -2267,8 +2269,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
addr_type = match.key->addr_type;
/* the HW doesn't support frag first/later */
- if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
+ if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
return -EOPNOTSUPP;
+ }
if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
@@ -2435,8 +2439,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
switch (ip_proto) {
case IPPROTO_ICMP:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMP))
+ MLX5_FLEX_PROTO_ICMP)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMP is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
@@ -2448,8 +2455,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
break;
case IPPROTO_ICMPV6:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMPV6))
+ MLX5_FLEX_PROTO_ICMPV6)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMPV6 is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
@@ -2555,15 +2565,19 @@ static int pedit_header_offsets[] = {
#define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
- struct pedit_headers_action *hdrs)
+ struct pedit_headers_action *hdrs,
+ struct netlink_ext_ack *extack)
{
u32 *curr_pmask, *curr_pval;
curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
- if (*curr_pmask & mask) /* disallow acting twice on the same location */
+ if (*curr_pmask & mask) { /* disallow acting twice on the same location */
+ NL_SET_ERR_MSG_MOD(extack,
+ "curr_pmask and new mask same. Acting twice on same location");
goto out_err;
+ }
*curr_pmask |= mask;
*curr_pval |= (val & mask);
@@ -2893,7 +2907,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
val = act->mangle.val;
offset = act->mangle.offset;
- err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
+ err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
if (err)
goto out_err;
@@ -2913,8 +2927,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
u32 mask, val, offset;
u32 *p;
- if (act->id != FLOW_ACTION_MANGLE)
+ if (act->id != FLOW_ACTION_MANGLE) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
return -EOPNOTSUPP;
+ }
if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
@@ -3363,12 +3379,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
u32 action = 0;
int err, i;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check not supported");
return -EOPNOTSUPP;
+ }
nic_attr = attr->nic_attr;
@@ -3459,7 +3479,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, CT);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in NIC action");
return -EOPNOTSUPP;
}
}
@@ -3514,19 +3535,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
static int parse_tc_vlan_action(struct mlx5e_priv *priv,
const struct flow_action_entry *act,
struct mlx5_esw_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
u8 vlan_idx = attr->total_vlan;
- if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
+ if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
+ NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
return -EOPNOTSUPP;
+ }
switch (act->id) {
case FLOW_ACTION_VLAN_POP:
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan pop action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
} else {
@@ -3542,20 +3569,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported for vlan depth > 1");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
} else {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
(act->vlan.proto != htons(ETH_P_8021Q) ||
- act->vlan.prio))
+ act->vlan.prio)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
}
break;
default:
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
return -EINVAL;
}
@@ -3589,7 +3623,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
static int add_vlan_push_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
struct net_device **out_dev,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct net_device *vlan_dev = *out_dev;
struct flow_action_entry vlan_act = {
@@ -3600,7 +3635,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
};
int err;
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
@@ -3611,14 +3646,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
return -ENODEV;
if (is_vlan_dev(*out_dev))
- err = add_vlan_push_action(priv, attr, out_dev, action);
+ err = add_vlan_push_action(priv, attr, out_dev, action, extack);
return err;
}
static int add_vlan_pop_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct flow_action_entry vlan_act = {
.id = FLOW_ACTION_VLAN_POP,
@@ -3628,7 +3664,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
nest_level = attr->parse_attr->filter_dev->lower_level -
priv->netdev->lower_level;
while (nest_level--) {
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
}
@@ -3751,12 +3787,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
int err, i, if_count = 0;
bool mpls_push = false;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
return -EOPNOTSUPP;
+ }
esw_attr = attr->esw_attr;
parse_attr = attr->parse_attr;
@@ -3900,14 +3940,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
if (is_vlan_dev(out_dev)) {
err = add_vlan_push_action(priv, attr,
&out_dev,
- &action);
+ &action, extack);
if (err)
return err;
}
if (is_vlan_dev(parse_attr->filter_dev)) {
err = add_vlan_pop_action(priv, attr,
- &action);
+ &action, extack);
if (err)
return err;
}
@@ -3953,10 +3993,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
break;
case FLOW_ACTION_TUNNEL_ENCAP:
info = act->tunnel;
- if (info)
+ if (info) {
encap = true;
- else
+ } else {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Zero tunnel attributes is not supported");
return -EOPNOTSUPP;
+ }
break;
case FLOW_ACTION_VLAN_PUSH:
@@ -3970,7 +4013,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
act, parse_attr, hdrs,
&action, extack);
} else {
- err = parse_tc_vlan_action(priv, act, esw_attr, &action);
+ err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
}
if (err)
return err;
@@ -4023,7 +4066,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, SAMPLE);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in FDB action");
return -EOPNOTSUPP;
}
}
@@ -4731,8 +4775,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
return -EOPNOTSUPP;
}
- if (!flow_action_basic_hw_stats_check(flow_action, extack))
+ if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
return -EOPNOTSUPP;
+ }
flow_action_for_each(i, act, flow_action) {
switch (act->id) {
--
2.27.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next v4] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-05 13:39 ` Roi Dayan
2021-09-06 9:29 ` Abhiram R N
2021-09-14 10:06 ` [PATCH net-next v3] " Abhiram R N
@ 2021-09-14 11:50 ` Abhiram R N
2021-09-19 7:26 ` Roi Dayan
2021-09-19 7:40 ` Roi Dayan
2 siblings, 2 replies; 13+ messages in thread
From: Abhiram R N @ 2021-09-14 11:50 UTC (permalink / raw)
To: roid
Cc: arn, hakhande, saeedm, Abhiram R N, Leon Romanovsky,
David S. Miller, Jakub Kicinski, netdev, linux-rdma,
linux-kernel
As multiple places EOPNOTSUPP and EINVAL is returned from driver
it becomes difficult to understand the reason only with error code.
With the netlink extack message exact reason will be known and will
aid in debugging.
Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
---
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
1 file changed, 76 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index ba8164792016..0272ba429c81 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
bool needs_mapping, sets_mapping;
int err;
- if (!mlx5e_is_eswitch_flow(flow))
+ if (!mlx5e_is_eswitch_flow(flow)) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
return -EOPNOTSUPP;
+ }
needs_mapping = !!flow->attr->chain;
sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
@@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
addr_type = match.key->addr_type;
/* the HW doesn't support frag first/later */
- if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
+ if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
return -EOPNOTSUPP;
+ }
if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
@@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
switch (ip_proto) {
case IPPROTO_ICMP:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMP))
+ MLX5_FLEX_PROTO_ICMP)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMP is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
@@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
break;
case IPPROTO_ICMPV6:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMPV6))
+ MLX5_FLEX_PROTO_ICMPV6)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMPV6 is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
@@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
#define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
- struct pedit_headers_action *hdrs)
+ struct pedit_headers_action *hdrs,
+ struct netlink_ext_ack *extack)
{
u32 *curr_pmask, *curr_pval;
curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
- if (*curr_pmask & mask) /* disallow acting twice on the same location */
+ if (*curr_pmask & mask) { /* disallow acting twice on the same location */
+ NL_SET_ERR_MSG_MOD(extack,
+ "curr_pmask and new mask same. Acting twice on same location");
goto out_err;
+ }
*curr_pmask |= mask;
*curr_pval |= (val & mask);
@@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
val = act->mangle.val;
offset = act->mangle.offset;
- err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
+ err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
if (err)
goto out_err;
@@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
u32 mask, val, offset;
u32 *p;
- if (act->id != FLOW_ACTION_MANGLE)
+ if (act->id != FLOW_ACTION_MANGLE) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
return -EOPNOTSUPP;
+ }
if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
@@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
u32 action = 0;
int err, i;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check not supported");
return -EOPNOTSUPP;
+ }
nic_attr = attr->nic_attr;
nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
@@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, CT);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in NIC action");
return -EOPNOTSUPP;
}
}
@@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
static int parse_tc_vlan_action(struct mlx5e_priv *priv,
const struct flow_action_entry *act,
struct mlx5_esw_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
u8 vlan_idx = attr->total_vlan;
- if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
+ if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
+ NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
return -EOPNOTSUPP;
+ }
switch (act->id) {
case FLOW_ACTION_VLAN_POP:
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan pop action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
} else {
@@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported for vlan depth > 1");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
} else {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
(act->vlan.proto != htons(ETH_P_8021Q) ||
- act->vlan.prio))
+ act->vlan.prio)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
}
break;
default:
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
return -EINVAL;
}
@@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
static int add_vlan_push_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
struct net_device **out_dev,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct net_device *vlan_dev = *out_dev;
struct flow_action_entry vlan_act = {
@@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
};
int err;
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
@@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
return -ENODEV;
if (is_vlan_dev(*out_dev))
- err = add_vlan_push_action(priv, attr, out_dev, action);
+ err = add_vlan_push_action(priv, attr, out_dev, action, extack);
return err;
}
static int add_vlan_pop_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct flow_action_entry vlan_act = {
.id = FLOW_ACTION_VLAN_POP,
@@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
nest_level = attr->parse_attr->filter_dev->lower_level -
priv->netdev->lower_level;
while (nest_level--) {
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
}
@@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
int err, i, if_count = 0;
bool mpls_push = false;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
return -EOPNOTSUPP;
+ }
esw_attr = attr->esw_attr;
parse_attr = attr->parse_attr;
@@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
if (is_vlan_dev(out_dev)) {
err = add_vlan_push_action(priv, attr,
&out_dev,
- &action);
+ &action, extack);
if (err)
return err;
}
if (is_vlan_dev(parse_attr->filter_dev)) {
err = add_vlan_pop_action(priv, attr,
- &action);
+ &action, extack);
if (err)
return err;
}
@@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
break;
case FLOW_ACTION_TUNNEL_ENCAP:
info = act->tunnel;
- if (info)
+ if (info) {
encap = true;
- else
+ } else {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Zero tunnel attributes is not supported");
return -EOPNOTSUPP;
+ }
break;
case FLOW_ACTION_VLAN_PUSH:
@@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
act, parse_attr, hdrs,
&action, extack);
} else {
- err = parse_tc_vlan_action(priv, act, esw_attr, &action);
+ err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
}
if (err)
return err;
@@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, SAMPLE);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in FDB action");
return -EOPNOTSUPP;
}
}
@@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
return -EOPNOTSUPP;
}
- if (!flow_action_basic_hw_stats_check(flow_action, extack))
+ if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
return -EOPNOTSUPP;
+ }
flow_action_for_each(i, act, flow_action) {
switch (act->id) {
--
2.27.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v4] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-14 11:50 ` [PATCH net-next v4] " Abhiram R N
@ 2021-09-19 7:26 ` Roi Dayan
2021-09-19 7:40 ` Roi Dayan
1 sibling, 0 replies; 13+ messages in thread
From: Roi Dayan @ 2021-09-19 7:26 UTC (permalink / raw)
To: Abhiram R N
Cc: arn, hakhande, saeedm, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, linux-kernel
so what is the diff between v3 and v4?
when u send a new revision you should specify what changed to help with
the review.
On 2021-09-14 2:50 PM, Abhiram R N wrote:
> As multiple places EOPNOTSUPP and EINVAL is returned from driver
> it becomes difficult to understand the reason only with error code.
> With the netlink extack message exact reason will be known and will
> aid in debugging.
>
> Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
> 1 file changed, 76 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index ba8164792016..0272ba429c81 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
> bool needs_mapping, sets_mapping;
> int err;
>
> - if (!mlx5e_is_eswitch_flow(flow))
> + if (!mlx5e_is_eswitch_flow(flow)) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
> return -EOPNOTSUPP;
> + }
>
> needs_mapping = !!flow->attr->chain;
> sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
> @@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> addr_type = match.key->addr_type;
>
> /* the HW doesn't support frag first/later */
> - if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
> + if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
> return -EOPNOTSUPP;
> + }
>
> if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
> MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
> @@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> switch (ip_proto) {
> case IPPROTO_ICMP:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMP))
> + MLX5_FLEX_PROTO_ICMP)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMP is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
> @@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> break;
> case IPPROTO_ICMPV6:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMPV6))
> + MLX5_FLEX_PROTO_ICMPV6)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMPV6 is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
> @@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
> #define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
>
> static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
> - struct pedit_headers_action *hdrs)
> + struct pedit_headers_action *hdrs,
> + struct netlink_ext_ack *extack)
> {
> u32 *curr_pmask, *curr_pval;
>
> curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
> curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
>
> - if (*curr_pmask & mask) /* disallow acting twice on the same location */
> + if (*curr_pmask & mask) { /* disallow acting twice on the same location */
> + NL_SET_ERR_MSG_MOD(extack,
> + "curr_pmask and new mask same. Acting twice on same location");
> goto out_err;
> + }
>
> *curr_pmask |= mask;
> *curr_pval |= (val & mask);
> @@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
> val = act->mangle.val;
> offset = act->mangle.offset;
>
> - err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
> + err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
> if (err)
> goto out_err;
>
> @@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
> u32 mask, val, offset;
> u32 *p;
>
> - if (act->id != FLOW_ACTION_MANGLE)
> + if (act->id != FLOW_ACTION_MANGLE) {
> + NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
> return -EOPNOTSUPP;
> + }
>
> if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
> NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
> @@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> u32 action = 0;
> int err, i;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check not supported");
> return -EOPNOTSUPP;
> + }
>
> nic_attr = attr->nic_attr;
> nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
> @@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, CT);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in NIC action");
> return -EOPNOTSUPP;
> }
> }
> @@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
> static int parse_tc_vlan_action(struct mlx5e_priv *priv,
> const struct flow_action_entry *act,
> struct mlx5_esw_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> u8 vlan_idx = attr->total_vlan;
>
> - if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
> + if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
> + NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
> return -EOPNOTSUPP;
> + }
>
> switch (act->id) {
> case FLOW_ACTION_VLAN_POP:
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan pop action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
> } else {
> @@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
>
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported for vlan depth > 1");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
> } else {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
> (act->vlan.proto != htons(ETH_P_8021Q) ||
> - act->vlan.prio))
> + act->vlan.prio)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
> }
> break;
> default:
> + NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
> return -EINVAL;
> }
>
> @@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
> static int add_vlan_push_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> struct net_device **out_dev,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct net_device *vlan_dev = *out_dev;
> struct flow_action_entry vlan_act = {
> @@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> };
> int err;
>
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
>
> @@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> return -ENODEV;
>
> if (is_vlan_dev(*out_dev))
> - err = add_vlan_push_action(priv, attr, out_dev, action);
> + err = add_vlan_push_action(priv, attr, out_dev, action, extack);
>
> return err;
> }
>
> static int add_vlan_pop_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct flow_action_entry vlan_act = {
> .id = FLOW_ACTION_VLAN_POP,
> @@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
> nest_level = attr->parse_attr->filter_dev->lower_level -
> priv->netdev->lower_level;
> while (nest_level--) {
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
> }
> @@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> int err, i, if_count = 0;
> bool mpls_push = false;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
> return -EOPNOTSUPP;
> + }
>
> esw_attr = attr->esw_attr;
> parse_attr = attr->parse_attr;
> @@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> if (is_vlan_dev(out_dev)) {
> err = add_vlan_push_action(priv, attr,
> &out_dev,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
>
> if (is_vlan_dev(parse_attr->filter_dev)) {
> err = add_vlan_pop_action(priv, attr,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
> @@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> break;
> case FLOW_ACTION_TUNNEL_ENCAP:
> info = act->tunnel;
> - if (info)
> + if (info) {
> encap = true;
> - else
> + } else {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Zero tunnel attributes is not supported");
> return -EOPNOTSUPP;
> + }
>
> break;
> case FLOW_ACTION_VLAN_PUSH:
> @@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> act, parse_attr, hdrs,
> &action, extack);
> } else {
> - err = parse_tc_vlan_action(priv, act, esw_attr, &action);
> + err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
> }
> if (err)
> return err;
> @@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, SAMPLE);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in FDB action");
> return -EOPNOTSUPP;
> }
> }
> @@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
> return -EOPNOTSUPP;
> }
>
> - if (!flow_action_basic_hw_stats_check(flow_action, extack))
> + if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
> return -EOPNOTSUPP;
> + }
>
> flow_action_for_each(i, act, flow_action) {
> switch (act->id) {
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next v4] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-14 11:50 ` [PATCH net-next v4] " Abhiram R N
2021-09-19 7:26 ` Roi Dayan
@ 2021-09-19 7:40 ` Roi Dayan
2021-09-19 19:16 ` [PATCH net-next v5] " Abhiram R N
1 sibling, 1 reply; 13+ messages in thread
From: Roi Dayan @ 2021-09-19 7:40 UTC (permalink / raw)
To: Abhiram R N
Cc: arn, hakhande, saeedm, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, linux-kernel
On 2021-09-14 2:50 PM, Abhiram R N wrote:
> As multiple places EOPNOTSUPP and EINVAL is returned from driver
> it becomes difficult to understand the reason only with error code.
> With the netlink extack message exact reason will be known and will
> aid in debugging.
>
> Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
> 1 file changed, 76 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index ba8164792016..0272ba429c81 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
> bool needs_mapping, sets_mapping;
> int err;
>
> - if (!mlx5e_is_eswitch_flow(flow))
> + if (!mlx5e_is_eswitch_flow(flow)) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
> return -EOPNOTSUPP;
> + }
>
> needs_mapping = !!flow->attr->chain;
> sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
> @@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> addr_type = match.key->addr_type;
>
> /* the HW doesn't support frag first/later */
> - if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
> + if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
> return -EOPNOTSUPP;
> + }
>
> if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
> MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
> @@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> switch (ip_proto) {
> case IPPROTO_ICMP:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMP))
> + MLX5_FLEX_PROTO_ICMP)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMP is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
> @@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> break;
> case IPPROTO_ICMPV6:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMPV6))
> + MLX5_FLEX_PROTO_ICMPV6)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMPV6 is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
> @@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
> #define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
>
> static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
> - struct pedit_headers_action *hdrs)
> + struct pedit_headers_action *hdrs,
> + struct netlink_ext_ack *extack)
> {
> u32 *curr_pmask, *curr_pval;
>
> curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
> curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
>
> - if (*curr_pmask & mask) /* disallow acting twice on the same location */
> + if (*curr_pmask & mask) { /* disallow acting twice on the same location */
> + NL_SET_ERR_MSG_MOD(extack,
> + "curr_pmask and new mask same. Acting twice on same location");
> goto out_err;
> + }
>
> *curr_pmask |= mask;
> *curr_pval |= (val & mask);
> @@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
> val = act->mangle.val;
> offset = act->mangle.offset;
>
> - err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
> + err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
> if (err)
> goto out_err;
>
> @@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
> u32 mask, val, offset;
> u32 *p;
>
> - if (act->id != FLOW_ACTION_MANGLE)
> + if (act->id != FLOW_ACTION_MANGLE) {
> + NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
> return -EOPNOTSUPP;
> + }
>
> if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
> NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
> @@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> u32 action = 0;
> int err, i;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action doesn't have any entries");
There is no need for capital letter in action. just "Flow action .."
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check not supported");
There is no need for capital first letter action.
also the msg is about hw stats type so a better msg would be:
"Flow action HW stats type is not supported"
> return -EOPNOTSUPP;
> + }
>
> nic_attr = attr->nic_attr;
> nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
> @@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, CT);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in NIC action");
> return -EOPNOTSUPP;
> }
> }
> @@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
> static int parse_tc_vlan_action(struct mlx5e_priv *priv,
> const struct flow_action_entry *act,
> struct mlx5_esw_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> u8 vlan_idx = attr->total_vlan;
>
> - if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
> + if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
> + NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
> return -EOPNOTSUPP;
> + }
>
> switch (act->id) {
> case FLOW_ACTION_VLAN_POP:
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan pop action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
> } else {
> @@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
>
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported for vlan depth > 1");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
> } else {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
> (act->vlan.proto != htons(ETH_P_8021Q) ||
> - act->vlan.prio))
> + act->vlan.prio)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
> }
> break;
> default:
> + NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
> return -EINVAL;
> }
>
> @@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
> static int add_vlan_push_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> struct net_device **out_dev,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct net_device *vlan_dev = *out_dev;
> struct flow_action_entry vlan_act = {
> @@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> };
> int err;
>
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
>
> @@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> return -ENODEV;
>
> if (is_vlan_dev(*out_dev))
> - err = add_vlan_push_action(priv, attr, out_dev, action);
> + err = add_vlan_push_action(priv, attr, out_dev, action, extack);
>
> return err;
> }
>
> static int add_vlan_pop_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct flow_action_entry vlan_act = {
> .id = FLOW_ACTION_VLAN_POP,
> @@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
> nest_level = attr->parse_attr->filter_dev->lower_level -
> priv->netdev->lower_level;
> while (nest_level--) {
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
> }
> @@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> int err, i, if_count = 0;
> bool mpls_push = false;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
same here.
the msg is about hw stats type so a better msg would be:
"Flow action HW stats type is not supported"
> return -EOPNOTSUPP;
> + }
>
> esw_attr = attr->esw_attr;
> parse_attr = attr->parse_attr;
> @@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> if (is_vlan_dev(out_dev)) {
> err = add_vlan_push_action(priv, attr,
> &out_dev,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
>
> if (is_vlan_dev(parse_attr->filter_dev)) {
> err = add_vlan_pop_action(priv, attr,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
> @@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> break;
> case FLOW_ACTION_TUNNEL_ENCAP:
> info = act->tunnel;
> - if (info)
> + if (info) {
> encap = true;
> - else
> + } else {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Zero tunnel attributes is not supported");
> return -EOPNOTSUPP;
> + }
>
> break;
> case FLOW_ACTION_VLAN_PUSH:
> @@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> act, parse_attr, hdrs,
> &action, extack);
> } else {
> - err = parse_tc_vlan_action(priv, act, esw_attr, &action);
> + err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
> }
> if (err)
> return err;
> @@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, SAMPLE);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in FDB action");
> return -EOPNOTSUPP;
> }
> }
> @@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
> return -EOPNOTSUPP;
> }
>
> - if (!flow_action_basic_hw_stats_check(flow_action, extack))
> + if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow Action HW stats check is not supported");
same here
the msg is about hw stats type so a better msg would be:
"Flow action HW stats type is not supported"
> return -EOPNOTSUPP;
> + }
>
> flow_action_for_each(i, act, flow_action) {
> switch (act->id) {
>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH net-next v5] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-19 7:40 ` Roi Dayan
@ 2021-09-19 19:16 ` Abhiram R N
2021-09-22 6:02 ` Roi Dayan
0 siblings, 1 reply; 13+ messages in thread
From: Abhiram R N @ 2021-09-19 19:16 UTC (permalink / raw)
To: roid
Cc: arn, hakhande, saeedm, Abhiram R N, Leon Romanovsky,
David S. Miller, Jakub Kicinski, netdev, linux-rdma,
linux-kernel
As multiple places EOPNOTSUPP and EINVAL is returned from driver
it becomes difficult to understand the reason only with error code.
With the netlink extack message exact reason will be known and will
aid in debugging.
V4->V5: Adressed comments (Rephrasing of msgs)
V3->V4: Rebased net-next (Fixed the merge conflicts in net-next branch)
V2->V3: Addressed comments (Rephrasing of msgs)
V1->V2: Addressed comments (Removed redundant msgs, rephrasing of msgs)
Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
---
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
1 file changed, 76 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index ba8164792016..0fda231c07cd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
bool needs_mapping, sets_mapping;
int err;
- if (!mlx5e_is_eswitch_flow(flow))
+ if (!mlx5e_is_eswitch_flow(flow)) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
return -EOPNOTSUPP;
+ }
needs_mapping = !!flow->attr->chain;
sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
@@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
addr_type = match.key->addr_type;
/* the HW doesn't support frag first/later */
- if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
+ if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
return -EOPNOTSUPP;
+ }
if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
@@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
switch (ip_proto) {
case IPPROTO_ICMP:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMP))
+ MLX5_FLEX_PROTO_ICMP)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMP is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
@@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
break;
case IPPROTO_ICMPV6:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMPV6))
+ MLX5_FLEX_PROTO_ICMPV6)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMPV6 is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
@@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
#define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
- struct pedit_headers_action *hdrs)
+ struct pedit_headers_action *hdrs,
+ struct netlink_ext_ack *extack)
{
u32 *curr_pmask, *curr_pval;
curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
- if (*curr_pmask & mask) /* disallow acting twice on the same location */
+ if (*curr_pmask & mask) { /* disallow acting twice on the same location */
+ NL_SET_ERR_MSG_MOD(extack,
+ "curr_pmask and new mask same. Acting twice on same location");
goto out_err;
+ }
*curr_pmask |= mask;
*curr_pval |= (val & mask);
@@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
val = act->mangle.val;
offset = act->mangle.offset;
- err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
+ err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
if (err)
goto out_err;
@@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
u32 mask, val, offset;
u32 *p;
- if (act->id != FLOW_ACTION_MANGLE)
+ if (act->id != FLOW_ACTION_MANGLE) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
return -EOPNOTSUPP;
+ }
if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
@@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
u32 action = 0;
int err, i;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
return -EOPNOTSUPP;
+ }
nic_attr = attr->nic_attr;
nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
@@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, CT);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in NIC action");
return -EOPNOTSUPP;
}
}
@@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
static int parse_tc_vlan_action(struct mlx5e_priv *priv,
const struct flow_action_entry *act,
struct mlx5_esw_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
u8 vlan_idx = attr->total_vlan;
- if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
+ if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
+ NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
return -EOPNOTSUPP;
+ }
switch (act->id) {
case FLOW_ACTION_VLAN_POP:
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan pop action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
} else {
@@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported for vlan depth > 1");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
} else {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
(act->vlan.proto != htons(ETH_P_8021Q) ||
- act->vlan.prio))
+ act->vlan.prio)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
}
break;
default:
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
return -EINVAL;
}
@@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
static int add_vlan_push_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
struct net_device **out_dev,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct net_device *vlan_dev = *out_dev;
struct flow_action_entry vlan_act = {
@@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
};
int err;
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
@@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
return -ENODEV;
if (is_vlan_dev(*out_dev))
- err = add_vlan_push_action(priv, attr, out_dev, action);
+ err = add_vlan_push_action(priv, attr, out_dev, action, extack);
return err;
}
static int add_vlan_pop_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct flow_action_entry vlan_act = {
.id = FLOW_ACTION_VLAN_POP,
@@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
nest_level = attr->parse_attr->filter_dev->lower_level -
priv->netdev->lower_level;
while (nest_level--) {
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
}
@@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
int err, i, if_count = 0;
bool mpls_push = false;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
return -EOPNOTSUPP;
+ }
esw_attr = attr->esw_attr;
parse_attr = attr->parse_attr;
@@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
if (is_vlan_dev(out_dev)) {
err = add_vlan_push_action(priv, attr,
&out_dev,
- &action);
+ &action, extack);
if (err)
return err;
}
if (is_vlan_dev(parse_attr->filter_dev)) {
err = add_vlan_pop_action(priv, attr,
- &action);
+ &action, extack);
if (err)
return err;
}
@@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
break;
case FLOW_ACTION_TUNNEL_ENCAP:
info = act->tunnel;
- if (info)
+ if (info) {
encap = true;
- else
+ } else {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Zero tunnel attributes is not supported");
return -EOPNOTSUPP;
+ }
break;
case FLOW_ACTION_VLAN_PUSH:
@@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
act, parse_attr, hdrs,
&action, extack);
} else {
- err = parse_tc_vlan_action(priv, act, esw_attr, &action);
+ err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
}
if (err)
return err;
@@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, SAMPLE);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in FDB action");
return -EOPNOTSUPP;
}
}
@@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
return -EOPNOTSUPP;
}
- if (!flow_action_basic_hw_stats_check(flow_action, extack))
+ if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
return -EOPNOTSUPP;
+ }
flow_action_for_each(i, act, flow_action) {
switch (act->id) {
--
2.27.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-19 19:16 ` [PATCH net-next v5] " Abhiram R N
@ 2021-09-22 6:02 ` Roi Dayan
2021-09-22 6:30 ` [PATCH net-next v6] " Abhiram R N
0 siblings, 1 reply; 13+ messages in thread
From: Roi Dayan @ 2021-09-22 6:02 UTC (permalink / raw)
To: Abhiram R N
Cc: arn, hakhande, saeedm, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, linux-kernel
On 2021-09-19 10:16 PM, Abhiram R N wrote:
> As multiple places EOPNOTSUPP and EINVAL is returned from driver
> it becomes difficult to understand the reason only with error code.
> With the netlink extack message exact reason will be known and will
> aid in debugging.
>
> V4->V5: Adressed comments (Rephrasing of msgs)
> V3->V4: Rebased net-next (Fixed the merge conflicts in net-next branch)
> V2->V3: Addressed comments (Rephrasing of msgs)
> V1->V2: Addressed comments (Removed redundant msgs, rephrasing of msgs)
>
> Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> ---
hi
the changelog is not needed as part of the commit msg.
it will have no meaning after merging.
the changelog should be here after the three dashes.
everything else looks ok to me but can you move the changelog to
this part.
thanks
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
> 1 file changed, 76 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index ba8164792016..0fda231c07cd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
> bool needs_mapping, sets_mapping;
> int err;
>
> - if (!mlx5e_is_eswitch_flow(flow))
> + if (!mlx5e_is_eswitch_flow(flow)) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
> return -EOPNOTSUPP;
> + }
>
> needs_mapping = !!flow->attr->chain;
> sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
> @@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> addr_type = match.key->addr_type;
>
> /* the HW doesn't support frag first/later */
> - if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
> + if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
> return -EOPNOTSUPP;
> + }
>
> if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
> MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
> @@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> switch (ip_proto) {
> case IPPROTO_ICMP:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMP))
> + MLX5_FLEX_PROTO_ICMP)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMP is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
> @@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> break;
> case IPPROTO_ICMPV6:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMPV6))
> + MLX5_FLEX_PROTO_ICMPV6)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMPV6 is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
> @@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
> #define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
>
> static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
> - struct pedit_headers_action *hdrs)
> + struct pedit_headers_action *hdrs,
> + struct netlink_ext_ack *extack)
> {
> u32 *curr_pmask, *curr_pval;
>
> curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
> curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
>
> - if (*curr_pmask & mask) /* disallow acting twice on the same location */
> + if (*curr_pmask & mask) { /* disallow acting twice on the same location */
> + NL_SET_ERR_MSG_MOD(extack,
> + "curr_pmask and new mask same. Acting twice on same location");
> goto out_err;
> + }
>
> *curr_pmask |= mask;
> *curr_pval |= (val & mask);
> @@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
> val = act->mangle.val;
> offset = act->mangle.offset;
>
> - err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
> + err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
> if (err)
> goto out_err;
>
> @@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
> u32 mask, val, offset;
> u32 *p;
>
> - if (act->id != FLOW_ACTION_MANGLE)
> + if (act->id != FLOW_ACTION_MANGLE) {
> + NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
> return -EOPNOTSUPP;
> + }
>
> if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
> NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
> @@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> u32 action = 0;
> int err, i;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
> return -EOPNOTSUPP;
> + }
>
> nic_attr = attr->nic_attr;
> nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
> @@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, CT);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in NIC action");
> return -EOPNOTSUPP;
> }
> }
> @@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
> static int parse_tc_vlan_action(struct mlx5e_priv *priv,
> const struct flow_action_entry *act,
> struct mlx5_esw_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> u8 vlan_idx = attr->total_vlan;
>
> - if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
> + if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
> + NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
> return -EOPNOTSUPP;
> + }
>
> switch (act->id) {
> case FLOW_ACTION_VLAN_POP:
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan pop action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
> } else {
> @@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
>
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported for vlan depth > 1");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
> } else {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
> (act->vlan.proto != htons(ETH_P_8021Q) ||
> - act->vlan.prio))
> + act->vlan.prio)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
> }
> break;
> default:
> + NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
> return -EINVAL;
> }
>
> @@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
> static int add_vlan_push_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> struct net_device **out_dev,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct net_device *vlan_dev = *out_dev;
> struct flow_action_entry vlan_act = {
> @@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> };
> int err;
>
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
>
> @@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> return -ENODEV;
>
> if (is_vlan_dev(*out_dev))
> - err = add_vlan_push_action(priv, attr, out_dev, action);
> + err = add_vlan_push_action(priv, attr, out_dev, action, extack);
>
> return err;
> }
>
> static int add_vlan_pop_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct flow_action_entry vlan_act = {
> .id = FLOW_ACTION_VLAN_POP,
> @@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
> nest_level = attr->parse_attr->filter_dev->lower_level -
> priv->netdev->lower_level;
> while (nest_level--) {
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
> }
> @@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> int err, i, if_count = 0;
> bool mpls_push = false;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
> return -EOPNOTSUPP;
> + }
>
> esw_attr = attr->esw_attr;
> parse_attr = attr->parse_attr;
> @@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> if (is_vlan_dev(out_dev)) {
> err = add_vlan_push_action(priv, attr,
> &out_dev,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
>
> if (is_vlan_dev(parse_attr->filter_dev)) {
> err = add_vlan_pop_action(priv, attr,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
> @@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> break;
> case FLOW_ACTION_TUNNEL_ENCAP:
> info = act->tunnel;
> - if (info)
> + if (info) {
> encap = true;
> - else
> + } else {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Zero tunnel attributes is not supported");
> return -EOPNOTSUPP;
> + }
>
> break;
> case FLOW_ACTION_VLAN_PUSH:
> @@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> act, parse_attr, hdrs,
> &action, extack);
> } else {
> - err = parse_tc_vlan_action(priv, act, esw_attr, &action);
> + err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
> }
> if (err)
> return err;
> @@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, SAMPLE);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in FDB action");
> return -EOPNOTSUPP;
> }
> }
> @@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
> return -EOPNOTSUPP;
> }
>
> - if (!flow_action_basic_hw_stats_check(flow_action, extack))
> + if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
> return -EOPNOTSUPP;
> + }
>
> flow_action_for_each(i, act, flow_action) {
> switch (act->id) {
>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH net-next v6] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-22 6:02 ` Roi Dayan
@ 2021-09-22 6:30 ` Abhiram R N
2021-09-22 7:23 ` Roi Dayan
0 siblings, 1 reply; 13+ messages in thread
From: Abhiram R N @ 2021-09-22 6:30 UTC (permalink / raw)
To: roid
Cc: arn, hakhande, saeedm, Abhiram R N, Leon Romanovsky,
David S. Miller, Jakub Kicinski, netdev, linux-rdma,
linux-kernel
As multiple places EOPNOTSUPP and EINVAL is returned from driver
it becomes difficult to understand the reason only with error code.
With the netlink extack message exact reason will be known and will
aid in debugging.
Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
---
V5->V6: Removed changelog from commit msg
V4->V5: Addressed comments (Rephrasing of msgs)
V3->V4: Rebased net-next (Fixed the merge conflicts in net-next branch)
V2->V3: Addressed comments (Rephrasing of msgs)
V1->V2: Addressed comments (Removed redundant msgs, rephrasing of msgs)
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
1 file changed, 76 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index ba8164792016..0fda231c07cd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
bool needs_mapping, sets_mapping;
int err;
- if (!mlx5e_is_eswitch_flow(flow))
+ if (!mlx5e_is_eswitch_flow(flow)) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
return -EOPNOTSUPP;
+ }
needs_mapping = !!flow->attr->chain;
sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
@@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
addr_type = match.key->addr_type;
/* the HW doesn't support frag first/later */
- if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
+ if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
+ NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
return -EOPNOTSUPP;
+ }
if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
@@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
switch (ip_proto) {
case IPPROTO_ICMP:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMP))
+ MLX5_FLEX_PROTO_ICMP)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMP is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
@@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
break;
case IPPROTO_ICMPV6:
if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
- MLX5_FLEX_PROTO_ICMPV6))
+ MLX5_FLEX_PROTO_ICMPV6)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Match on Flex protocols for ICMPV6 is not supported");
return -EOPNOTSUPP;
+ }
MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
match.mask->type);
MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
@@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
#define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
- struct pedit_headers_action *hdrs)
+ struct pedit_headers_action *hdrs,
+ struct netlink_ext_ack *extack)
{
u32 *curr_pmask, *curr_pval;
curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
- if (*curr_pmask & mask) /* disallow acting twice on the same location */
+ if (*curr_pmask & mask) { /* disallow acting twice on the same location */
+ NL_SET_ERR_MSG_MOD(extack,
+ "curr_pmask and new mask same. Acting twice on same location");
goto out_err;
+ }
*curr_pmask |= mask;
*curr_pval |= (val & mask);
@@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
val = act->mangle.val;
offset = act->mangle.offset;
- err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
+ err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
if (err)
goto out_err;
@@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
u32 mask, val, offset;
u32 *p;
- if (act->id != FLOW_ACTION_MANGLE)
+ if (act->id != FLOW_ACTION_MANGLE) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
return -EOPNOTSUPP;
+ }
if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
@@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
u32 action = 0;
int err, i;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
return -EOPNOTSUPP;
+ }
nic_attr = attr->nic_attr;
nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
@@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, CT);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in NIC action");
return -EOPNOTSUPP;
}
}
@@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
static int parse_tc_vlan_action(struct mlx5e_priv *priv,
const struct flow_action_entry *act,
struct mlx5_esw_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
u8 vlan_idx = attr->total_vlan;
- if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
+ if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
+ NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
return -EOPNOTSUPP;
+ }
switch (act->id) {
case FLOW_ACTION_VLAN_POP:
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan pop action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
} else {
@@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
if (vlan_idx) {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
- MLX5_FS_VLAN_DEPTH))
+ MLX5_FS_VLAN_DEPTH)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported for vlan depth > 1");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
} else {
if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
(act->vlan.proto != htons(ETH_P_8021Q) ||
- act->vlan.prio))
+ act->vlan.prio)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "vlan push action is not supported");
return -EOPNOTSUPP;
+ }
*action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
}
break;
default:
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
return -EINVAL;
}
@@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
static int add_vlan_push_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
struct net_device **out_dev,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct net_device *vlan_dev = *out_dev;
struct flow_action_entry vlan_act = {
@@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
};
int err;
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
@@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
return -ENODEV;
if (is_vlan_dev(*out_dev))
- err = add_vlan_push_action(priv, attr, out_dev, action);
+ err = add_vlan_push_action(priv, attr, out_dev, action, extack);
return err;
}
static int add_vlan_pop_action(struct mlx5e_priv *priv,
struct mlx5_flow_attr *attr,
- u32 *action)
+ u32 *action,
+ struct netlink_ext_ack *extack)
{
struct flow_action_entry vlan_act = {
.id = FLOW_ACTION_VLAN_POP,
@@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
nest_level = attr->parse_attr->filter_dev->lower_level -
priv->netdev->lower_level;
while (nest_level--) {
- err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
+ err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
if (err)
return err;
}
@@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
int err, i, if_count = 0;
bool mpls_push = false;
- if (!flow_action_has_entries(flow_action))
+ if (!flow_action_has_entries(flow_action)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
return -EINVAL;
+ }
if (!flow_action_hw_stats_check(flow_action, extack,
- FLOW_ACTION_HW_STATS_DELAYED_BIT))
+ FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
return -EOPNOTSUPP;
+ }
esw_attr = attr->esw_attr;
parse_attr = attr->parse_attr;
@@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
if (is_vlan_dev(out_dev)) {
err = add_vlan_push_action(priv, attr,
&out_dev,
- &action);
+ &action, extack);
if (err)
return err;
}
if (is_vlan_dev(parse_attr->filter_dev)) {
err = add_vlan_pop_action(priv, attr,
- &action);
+ &action, extack);
if (err)
return err;
}
@@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
break;
case FLOW_ACTION_TUNNEL_ENCAP:
info = act->tunnel;
- if (info)
+ if (info) {
encap = true;
- else
+ } else {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Zero tunnel attributes is not supported");
return -EOPNOTSUPP;
+ }
break;
case FLOW_ACTION_VLAN_PUSH:
@@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
act, parse_attr, hdrs,
&action, extack);
} else {
- err = parse_tc_vlan_action(priv, act, esw_attr, &action);
+ err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
}
if (err)
return err;
@@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
flow_flag_set(flow, SAMPLE);
break;
default:
- NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
+ NL_SET_ERR_MSG_MOD(extack,
+ "The offload action is not supported in FDB action");
return -EOPNOTSUPP;
}
}
@@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
return -EOPNOTSUPP;
}
- if (!flow_action_basic_hw_stats_check(flow_action, extack))
+ if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
+ NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
return -EOPNOTSUPP;
+ }
flow_action_for_each(i, act, flow_action) {
switch (act->id) {
--
2.27.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v6] net/mlx5e: Add extack msgs related to TC for better debug
2021-09-22 6:30 ` [PATCH net-next v6] " Abhiram R N
@ 2021-09-22 7:23 ` Roi Dayan
0 siblings, 0 replies; 13+ messages in thread
From: Roi Dayan @ 2021-09-22 7:23 UTC (permalink / raw)
To: Abhiram R N
Cc: arn, hakhande, saeedm, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, linux-kernel
On 2021-09-22 9:30 AM, Abhiram R N wrote:
> As multiple places EOPNOTSUPP and EINVAL is returned from driver
> it becomes difficult to understand the reason only with error code.
> With the netlink extack message exact reason will be known and will
> aid in debugging.
>
> Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> ---
> V5->V6: Removed changelog from commit msg
> V4->V5: Addressed comments (Rephrasing of msgs)
> V3->V4: Rebased net-next (Fixed the merge conflicts in net-next branch)
> V2->V3: Addressed comments (Rephrasing of msgs)
> V1->V2: Addressed comments (Removed redundant msgs, rephrasing of msgs)
>
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 106 +++++++++++++-----
> 1 file changed, 76 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index ba8164792016..0fda231c07cd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -1896,8 +1896,10 @@ static int parse_tunnel_attr(struct mlx5e_priv *priv,
> bool needs_mapping, sets_mapping;
> int err;
>
> - if (!mlx5e_is_eswitch_flow(flow))
> + if (!mlx5e_is_eswitch_flow(flow)) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on tunnel is not supported");
> return -EOPNOTSUPP;
> + }
>
> needs_mapping = !!flow->attr->chain;
> sets_mapping = flow_requires_tunnel_mapping(flow->attr->chain, f);
> @@ -2269,8 +2271,10 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> addr_type = match.key->addr_type;
>
> /* the HW doesn't support frag first/later */
> - if (match.mask->flags & FLOW_DIS_FIRST_FRAG)
> + if (match.mask->flags & FLOW_DIS_FIRST_FRAG) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on frag first/later is not supported");
> return -EOPNOTSUPP;
> + }
>
> if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) {
> MLX5_SET(fte_match_set_lyr_2_4, headers_c, frag, 1);
> @@ -2437,8 +2441,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> switch (ip_proto) {
> case IPPROTO_ICMP:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMP))
> + MLX5_FLEX_PROTO_ICMP)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMP is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmp_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmp_type,
> @@ -2450,8 +2457,11 @@ static int __parse_cls_flower(struct mlx5e_priv *priv,
> break;
> case IPPROTO_ICMPV6:
> if (!(MLX5_CAP_GEN(priv->mdev, flex_parser_protocols) &
> - MLX5_FLEX_PROTO_ICMPV6))
> + MLX5_FLEX_PROTO_ICMPV6)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Match on Flex protocols for ICMPV6 is not supported");
> return -EOPNOTSUPP;
> + }
> MLX5_SET(fte_match_set_misc3, misc_c_3, icmpv6_type,
> match.mask->type);
> MLX5_SET(fte_match_set_misc3, misc_v_3, icmpv6_type,
> @@ -2557,15 +2567,19 @@ static int pedit_header_offsets[] = {
> #define pedit_header(_ph, _htype) ((void *)(_ph) + pedit_header_offsets[_htype])
>
> static int set_pedit_val(u8 hdr_type, u32 mask, u32 val, u32 offset,
> - struct pedit_headers_action *hdrs)
> + struct pedit_headers_action *hdrs,
> + struct netlink_ext_ack *extack)
> {
> u32 *curr_pmask, *curr_pval;
>
> curr_pmask = (u32 *)(pedit_header(&hdrs->masks, hdr_type) + offset);
> curr_pval = (u32 *)(pedit_header(&hdrs->vals, hdr_type) + offset);
>
> - if (*curr_pmask & mask) /* disallow acting twice on the same location */
> + if (*curr_pmask & mask) { /* disallow acting twice on the same location */
> + NL_SET_ERR_MSG_MOD(extack,
> + "curr_pmask and new mask same. Acting twice on same location");
> goto out_err;
> + }
>
> *curr_pmask |= mask;
> *curr_pval |= (val & mask);
> @@ -2898,7 +2912,7 @@ parse_pedit_to_modify_hdr(struct mlx5e_priv *priv,
> val = act->mangle.val;
> offset = act->mangle.offset;
>
> - err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd]);
> + err = set_pedit_val(htype, ~mask, val, offset, &hdrs[cmd], extack);
> if (err)
> goto out_err;
>
> @@ -2918,8 +2932,10 @@ parse_pedit_to_reformat(struct mlx5e_priv *priv,
> u32 mask, val, offset;
> u32 *p;
>
> - if (act->id != FLOW_ACTION_MANGLE)
> + if (act->id != FLOW_ACTION_MANGLE) {
> + NL_SET_ERR_MSG_MOD(extack, "Unsupported action id");
> return -EOPNOTSUPP;
> + }
>
> if (act->mangle.htype != FLOW_ACT_MANGLE_HDR_TYPE_ETH) {
> NL_SET_ERR_MSG_MOD(extack, "Only Ethernet modification is supported");
> @@ -3368,12 +3384,16 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> u32 action = 0;
> int err, i;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
> return -EOPNOTSUPP;
> + }
>
> nic_attr = attr->nic_attr;
> nic_attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
> @@ -3462,7 +3482,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, CT);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in NIC action");
> return -EOPNOTSUPP;
> }
> }
> @@ -3517,19 +3538,25 @@ static bool is_merged_eswitch_vfs(struct mlx5e_priv *priv,
> static int parse_tc_vlan_action(struct mlx5e_priv *priv,
> const struct flow_action_entry *act,
> struct mlx5_esw_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> u8 vlan_idx = attr->total_vlan;
>
> - if (vlan_idx >= MLX5_FS_VLAN_DEPTH)
> + if (vlan_idx >= MLX5_FS_VLAN_DEPTH) {
> + NL_SET_ERR_MSG_MOD(extack, "Total vlans used is greater than supported");
> return -EOPNOTSUPP;
> + }
>
> switch (act->id) {
> case FLOW_ACTION_VLAN_POP:
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan pop action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2;
> } else {
> @@ -3545,20 +3572,27 @@ static int parse_tc_vlan_action(struct mlx5e_priv *priv,
>
> if (vlan_idx) {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev,
> - MLX5_FS_VLAN_DEPTH))
> + MLX5_FS_VLAN_DEPTH)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported for vlan depth > 1");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2;
> } else {
> if (!mlx5_eswitch_vlan_actions_supported(priv->mdev, 1) &&
> (act->vlan.proto != htons(ETH_P_8021Q) ||
> - act->vlan.prio))
> + act->vlan.prio)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "vlan push action is not supported");
> return -EOPNOTSUPP;
> + }
>
> *action |= MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH;
> }
> break;
> default:
> + NL_SET_ERR_MSG_MOD(extack, "Unexpected action id for VLAN");
> return -EINVAL;
> }
>
> @@ -3592,7 +3626,8 @@ static struct net_device *get_fdb_out_dev(struct net_device *uplink_dev,
> static int add_vlan_push_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> struct net_device **out_dev,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct net_device *vlan_dev = *out_dev;
> struct flow_action_entry vlan_act = {
> @@ -3603,7 +3638,7 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> };
> int err;
>
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
>
> @@ -3614,14 +3649,15 @@ static int add_vlan_push_action(struct mlx5e_priv *priv,
> return -ENODEV;
>
> if (is_vlan_dev(*out_dev))
> - err = add_vlan_push_action(priv, attr, out_dev, action);
> + err = add_vlan_push_action(priv, attr, out_dev, action, extack);
>
> return err;
> }
>
> static int add_vlan_pop_action(struct mlx5e_priv *priv,
> struct mlx5_flow_attr *attr,
> - u32 *action)
> + u32 *action,
> + struct netlink_ext_ack *extack)
> {
> struct flow_action_entry vlan_act = {
> .id = FLOW_ACTION_VLAN_POP,
> @@ -3631,7 +3667,7 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
> nest_level = attr->parse_attr->filter_dev->lower_level -
> priv->netdev->lower_level;
> while (nest_level--) {
> - err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action);
> + err = parse_tc_vlan_action(priv, &vlan_act, attr->esw_attr, action, extack);
> if (err)
> return err;
> }
> @@ -3753,12 +3789,16 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> int err, i, if_count = 0;
> bool mpls_push = false;
>
> - if (!flow_action_has_entries(flow_action))
> + if (!flow_action_has_entries(flow_action)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action doesn't have any entries");
> return -EINVAL;
> + }
>
> if (!flow_action_hw_stats_check(flow_action, extack,
> - FLOW_ACTION_HW_STATS_DELAYED_BIT))
> + FLOW_ACTION_HW_STATS_DELAYED_BIT)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
> return -EOPNOTSUPP;
> + }
>
> esw_attr = attr->esw_attr;
> parse_attr = attr->parse_attr;
> @@ -3902,14 +3942,14 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> if (is_vlan_dev(out_dev)) {
> err = add_vlan_push_action(priv, attr,
> &out_dev,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
>
> if (is_vlan_dev(parse_attr->filter_dev)) {
> err = add_vlan_pop_action(priv, attr,
> - &action);
> + &action, extack);
> if (err)
> return err;
> }
> @@ -3955,10 +3995,13 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> break;
> case FLOW_ACTION_TUNNEL_ENCAP:
> info = act->tunnel;
> - if (info)
> + if (info) {
> encap = true;
> - else
> + } else {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Zero tunnel attributes is not supported");
> return -EOPNOTSUPP;
> + }
>
> break;
> case FLOW_ACTION_VLAN_PUSH:
> @@ -3972,7 +4015,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> act, parse_attr, hdrs,
> &action, extack);
> } else {
> - err = parse_tc_vlan_action(priv, act, esw_attr, &action);
> + err = parse_tc_vlan_action(priv, act, esw_attr, &action, extack);
> }
> if (err)
> return err;
> @@ -4025,7 +4068,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> flow_flag_set(flow, SAMPLE);
> break;
> default:
> - NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + NL_SET_ERR_MSG_MOD(extack,
> + "The offload action is not supported in FDB action");
> return -EOPNOTSUPP;
> }
> }
> @@ -4733,8 +4777,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
> return -EOPNOTSUPP;
> }
>
> - if (!flow_action_basic_hw_stats_check(flow_action, extack))
> + if (!flow_action_basic_hw_stats_check(flow_action, extack)) {
> + NL_SET_ERR_MSG_MOD(extack, "Flow action HW stats type is not supported");
> return -EOPNOTSUPP;
> + }
>
> flow_action_for_each(i, act, flow_action) {
> switch (act->id) {
>
thanks
Reviewed-by: Roi Dayan <roid@nvidia.com>
^ permalink raw reply [flat|nested] 13+ messages in thread