Re: [PATCH v2] net/mlx5e: Add extack msgs related to TC for better debug
From: Abhiram R N
Date: Mon Sep 06 2021 - 05:29:54 EST
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@xxxxxxxxx>
> > ---
> > .../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