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

[Amir Vadai <amir@vadai.me>]

On Tue, Mar 01, 2016 at 05:59:59PM +0200, Saeed Mahameed wrote:
> On Tue, Mar 1, 2016 at 4:24 PM, Amir Vadai <amir@vadai.me> wrote:
> > +#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

> 
> 
> > @@ -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

> 
> > +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.
> 
> > +               }
> > +
> > +               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;
> > +}
> > +
> 
> > +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


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