From: Leon Romanovsky <leon@kernel.org>
To: Dan Carpenter <dan.carpenter@linaro.org>,
Saeed Mahameed <saeedm@nvidia.com>
Cc: paulb@nvidia.com, linux-rdma@vger.kernel.org,
linux-netdev <netdev@vger.kernel.org>
Subject: Re: [bug report] net/mlx5e: TC, Set CT miss to the specific ct action instance
Date: Sun, 11 Jun 2023 20:35:03 +0300 [thread overview]
Message-ID: <20230611173503.GF12152@unreal> (raw)
In-Reply-To: <497f5a7a-9f14-478e-b551-1fa74720b6f8@moroto.mountain>
+ netdev
On Wed, Jun 07, 2023 at 10:09:57AM +0300, Dan Carpenter wrote:
> Hello Paul Blakey,
>
> The patch 6702782845a5: "net/mlx5e: TC, Set CT miss to the specific
> ct action instance" from Feb 18, 2023, leads to the following Smatch
> static checker warning:
>
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:5648 mlx5e_tc_action_miss_mapping_get() warn: missing error code 'err'
>
> Let's include some older unpublished Smatch stuff as well.
>
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:1683 mlx5e_tc_query_route_vport() info: return a literal instead of 'err'
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:4786 mlx5e_stats_flower() warn: missing error code 'err'
>
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> 1665 int mlx5e_tc_query_route_vport(struct net_device *out_dev, struct net_device *route_dev, u16 *vport)
> 1666 {
> 1667 struct mlx5e_priv *out_priv, *route_priv;
> 1668 struct mlx5_core_dev *route_mdev;
> 1669 struct mlx5_devcom *devcom;
> 1670 struct mlx5_eswitch *esw;
> 1671 u16 vhca_id;
> 1672 int err;
> 1673 int i;
> 1674
> 1675 out_priv = netdev_priv(out_dev);
> 1676 esw = out_priv->mdev->priv.eswitch;
> 1677 route_priv = netdev_priv(route_dev);
> 1678 route_mdev = route_priv->mdev;
> 1679
> 1680 vhca_id = MLX5_CAP_GEN(route_mdev, vhca_id);
> 1681 err = mlx5_eswitch_vhca_id_to_vport(esw, vhca_id, vport);
> 1682 if (!err)
> 1683 return err;
>
> This seems intentional but it look more intentional as "return 0;"
>
> 1684
> 1685 if (!mlx5_lag_is_active(out_priv->mdev))
> 1686 return err;
>
> return -ENOENT; here?
>
> 1687
> 1688 rcu_read_lock();
> 1689 devcom = out_priv->mdev->priv.devcom;
> 1690 err = -ENODEV;
> 1691 mlx5_devcom_for_each_peer_entry_rcu(devcom, MLX5_DEVCOM_ESW_OFFLOADS,
> 1692 esw, i) {
> 1693 err = mlx5_eswitch_vhca_id_to_vport(esw, vhca_id, vport);
> 1694 if (!err)
> 1695 break;
> 1696 }
> 1697 rcu_read_unlock();
> 1698
> 1699 return err;
> 1700 }
>
> [ snip ]
>
> 4727 int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv,
> 4728 struct flow_cls_offload *f, unsigned long flags)
> 4729 {
> 4730 struct mlx5_devcom *devcom = priv->mdev->priv.devcom;
> 4731 struct rhashtable *tc_ht = get_tc_ht(priv, flags);
> 4732 struct mlx5e_tc_flow *flow;
> 4733 struct mlx5_fc *counter;
> 4734 u64 lastuse = 0;
> 4735 u64 packets = 0;
> 4736 u64 bytes = 0;
> 4737 int err = 0;
> 4738
> 4739 rcu_read_lock();
> 4740 flow = mlx5e_flow_get(rhashtable_lookup(tc_ht, &f->cookie,
> 4741 tc_ht_params));
> 4742 rcu_read_unlock();
> 4743 if (IS_ERR(flow))
> 4744 return PTR_ERR(flow);
> 4745
> 4746 if (!same_flow_direction(flow, flags)) {
> 4747 err = -EINVAL;
> 4748 goto errout;
> 4749 }
> 4750
> 4751 if (mlx5e_is_offloaded_flow(flow)) {
> 4752 if (flow_flag_test(flow, USE_ACT_STATS)) {
> 4753 f->use_act_stats = true;
> 4754 } else {
> 4755 counter = mlx5e_tc_get_counter(flow);
> 4756 if (!counter)
> 4757 goto errout;
>
> No error code. In this function it's hard to say if these should be
> error paths or not.
>
> 4758
> 4759 mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
> 4760 }
> 4761 }
> 4762
> 4763 /* Under multipath it's possible for one rule to be currently
> 4764 * un-offloaded while the other rule is offloaded.
> 4765 */
> 4766 if (!mlx5_devcom_for_each_peer_begin(devcom, MLX5_DEVCOM_ESW_OFFLOADS))
> 4767 goto out;
>
> No error code
>
> 4768
> 4769 if (flow_flag_test(flow, DUP)) {
> 4770 struct mlx5e_tc_flow *peer_flow;
> 4771
> 4772 list_for_each_entry(peer_flow, &flow->peer_flows, peer_flows) {
> 4773 u64 packets2;
> 4774 u64 lastuse2;
> 4775 u64 bytes2;
> 4776
> 4777 if (!flow_flag_test(peer_flow, OFFLOADED))
> 4778 continue;
> 4779 if (flow_flag_test(flow, USE_ACT_STATS)) {
> 4780 f->use_act_stats = true;
> 4781 break;
> 4782 }
> 4783
> 4784 counter = mlx5e_tc_get_counter(peer_flow);
> 4785 if (!counter)
> 4786 goto no_peer_counter;
>
> No error code
>
> 4787 mlx5_fc_query_cached(counter, &bytes2, &packets2,
> 4788 &lastuse2);
> 4789
> 4790 bytes += bytes2;
> 4791 packets += packets2;
> 4792 lastuse = max_t(u64, lastuse, lastuse2);
> 4793 }
> 4794 }
> 4795
> 4796 no_peer_counter:
> 4797 mlx5_devcom_for_each_peer_end(devcom, MLX5_DEVCOM_ESW_OFFLOADS);
> 4798 out:
> 4799 flow_stats_update(&f->stats, bytes, packets, 0, lastuse,
> 4800 FLOW_ACTION_HW_STATS_DELAYED);
> 4801 trace_mlx5e_stats_flower(f);
> 4802 errout:
> 4803 mlx5e_flow_put(priv, flow);
> 4804 return err;
> 4805 }
>
> [ snip ]
>
> 5627 int mlx5e_tc_action_miss_mapping_get(struct mlx5e_priv *priv, struct mlx5_flow_attr *attr,
> 5628 u64 act_miss_cookie, u32 *act_miss_mapping)
> 5629 {
> 5630 struct mlx5_mapped_obj mapped_obj = {};
> 5631 struct mlx5_eswitch *esw;
> 5632 struct mapping_ctx *ctx;
> 5633 int err;
> 5634
> 5635 ctx = mlx5e_get_priv_obj_mapping(priv);
> 5636 mapped_obj.type = MLX5_MAPPED_OBJ_ACT_MISS;
> 5637 mapped_obj.act_miss_cookie = act_miss_cookie;
> 5638 err = mapping_add(ctx, &mapped_obj, act_miss_mapping);
> 5639 if (err)
> 5640 return err;
> 5641
> 5642 if (!is_mdev_switchdev_mode(priv->mdev))
> 5643 return 0;
> 5644
> 5645 esw = priv->mdev->priv.eswitch;
> 5646 attr->act_id_restore_rule = esw_add_restore_rule(esw, *act_miss_mapping);
> 5647 if (IS_ERR(attr->act_id_restore_rule))
> 5648 goto err_rule;
>
> This one is definitely an error path.
>
> 5649
> 5650 return 0;
> 5651
> 5652 err_rule:
> 5653 mapping_remove(ctx, *act_miss_mapping);
> 5654 return err;
> 5655 }
>
> regards,
> dan carpenter
prev parent reply other threads:[~2023-06-11 17:35 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-07 7:09 [bug report] net/mlx5e: TC, Set CT miss to the specific ct action instance Dan Carpenter
2023-06-11 17:35 ` Leon Romanovsky [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230611173503.GF12152@unreal \
--to=leon@kernel.org \
--cc=dan.carpenter@linaro.org \
--cc=linux-rdma@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=paulb@nvidia.com \
--cc=saeedm@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.