Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support

On Tue, Mar 01, 2016 at 05:59:59PM +0200, Saeed Mahameed wrote:

On Tue, Mar 1, 2016 at 4:24 PM, Amir Vadai [off-list ref] wrote:

quoted

+#define FT_CAP(f) MLX5_CAP_FLOWTABLE(mdev, flow_table_properties_nic_receive.f)
+       if (FT_CAP(flow_modify_en) &&
+           FT_CAP(modify_root) &&
+           FT_CAP(identified_miss_table_mode) &&
+           FT_CAP(flow_table_modify))
+               priv->netdev->hw_features      |= NETIF_F_HW_TC;
+
        netdev->features         |= NETIF_F_HIGHDMA;

        netdev->priv_flags       |= IFF_UNICAST_FLT;

+       mlx5e_tc_init(priv);

This is not the place for this, We usually do internal data structure
initialization  after we create all HW resources in
mlx5e_create_netdev
Please see mlx5e_vxlan_init as example, and you already call
mlx5e_tc_cleanup inside mlx5e_destroy_netdev, please move the
mlx5e_tc_init
to mlx5e_create_netdev after HW resources creation,

ack

quoted

@@ -2558,6 +2588,7 @@ static void mlx5e_destroy_netdev(struct mlx5_core_dev *mdev, void *vpriv)
        mlx5_core_dealloc_transport_domain(priv->mdev, priv->tdn);
        mlx5_core_dealloc_pd(priv->mdev, priv->pdn);
        mlx5_unmap_free_uar(priv->mdev, &priv->cq_uar);
+       mlx5e_tc_cleanup(priv);

I would suggest to move  mlx5e_tc_init to be right after
mlx5e_vxlan_init and mlx5e_tc_cleanup before mlx5e_vxlan_cleanup.

ok

quoted

+struct mlx5_flow_rule *mlx5e_tc_add_flow(struct mlx5e_priv *priv,
+                                        u32 *match_c, u32 *match_v,
+                                        u32 action, u32 flow_tag)
+{
+       struct mlx5_flow_destination dest = {
+               .type = MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE,
+               {.ft = priv->fts.vlan.t},
+       };
+       struct mlx5_flow_rule *rule;
+       bool table_created = false;
+
+       if (IS_ERR_OR_NULL(priv->fts.tc.t)) {
+               priv->fts.tc.t =
+                       mlx5_create_auto_grouped_flow_table(priv->fts.ns, 0,
+                                                           MLX5E_TC_FLOW_TABLE_NUM_ENTRIES,
+                                                           MLX5E_TC_FLOW_TABLE_NUM_GROUPS);
+               if (IS_ERR(priv->fts.tc.t)) {
+                       netdev_err(priv->netdev,
+                                  "Failed to create tc offload table\n");
+                       return ERR_CAST(priv->fts.tc.t);

Here priv->fts.tc.t will be invalid pointer and in your code you treat
it as NULL in case of failure.

quoted

+               }
+
+               table_created = true;
+       }
+
+       rule = mlx5_add_flow_rule(priv->fts.tc.t, MLX5_MATCH_OUTER_HEADERS,
+                                 match_c, match_v,
+                                 action, flow_tag,
+                                 action & MLX5_FLOW_CONTEXT_ACTION_FWD_DEST ? &dest : NULL);
+
+       if (IS_ERR(rule) && table_created) {
+               mlx5_destroy_flow_table(priv->fts.tc.t);
+               priv->fts.tc.t = NULL;
+       }
+
+       return rule;
+}
+

quoted

+void mlx5e_tc_cleanup(struct mlx5e_priv *priv)
+{
+       struct mlx5e_tc_flow_table *tc = &priv->fts.tc;
+
+       rhashtable_free_and_destroy(&tc->ht, _mlx5e_tc_del_flow, priv);
+
+       if (priv->fts.tc.t) {

priv->fts.tc.t will be invalid pointer and this test will pass in case
 mlx5_create_auto_grouped_flow_table had failed

Yeh - should have used !IS_ERR_OR_NULL() or set it to NULL on failure
above.

Thanks,
Amir

quoted

+               mlx5_destroy_flow_table(priv->fts.tc.t);
+               priv->fts.tc.t = NULL;
+       }
+}