Re: [PATCH net-next v2 09/10] octeontx2: switch: Flow offload support

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-01-08 at 21:37:44, Kalesh Anakkur Purayil (kalesh-anakkur.purayil@broadcom.com) wrote:
> On Wed, Jan 7, 2026 at 7:23 PM Ratheesh Kannoth <rkannoth@marvell.com> wrote:
> >
> > +               return 0;
> > +       }
> > +       mutex_unlock(&sw_fl_stats_lock);
> > +
> > +       snode = kcalloc(1, sizeof(*snode), GFP_KERNEL);
> Why not kzalloc() instead of kcalloc with size 1?
All fields are assigned with values. why to initialize to zero ?

> > +       if (!snode)
> > +
> > +       return 0;
> > +}
> > +
> > +int rvu_sw_fl_stats_sync2db(struct rvu *rvu, struct fl_info *fl, int cnt)
> > +{
> > +       struct npc_mcam_get_mul_stats_req *req = NULL;
> > +       struct npc_mcam_get_mul_stats_rsp *rsp = NULL;
> there is no need to initialize these two variables
This is initialized so that (after fail lablel) free() does not act
on garbage value.

> > +       int tot = 0;
> > +       u16 i2idx_map[256];
> follow RCT order
ACK.

> > +       int rc = 0;
> > +       u64 pkts;
> > +       int idx;
> > +
> > +       cnt = min(cnt, 64);
> > +
> > +       for (int i = 0; i < cnt; i++) {
> I think you can move the declaration of i at the beginning of the
> function. it is repeated in the for loops below as well
I think, better to keep the scope local. Does kernel coding guidlines
mandate it ?

> > +               tot++;
> > +               if (fl[i].uni_di)
> > +                       continue;
> > +
> > +               tot++;
> > +       }
> > +
> > +       req = kcalloc(1, sizeof(*req), GFP_KERNEL);
> I think you can use kzalloc kere
ACK.

> > +       if (!req) {
> > +               rc = -ENOMEM;
> You can return directly here
ACK.
> > +               goto fail;
> > +       }
> > +
> > +       rsp = kcalloc(1, sizeof(*rsp), GFP_KERNEL);
> > +       if (!rsp) {
> > +               rc = -ENOMEM;
> > +               goto fail;
> better do individual cleanup by adding a label and use goto free_req
free: lablel is enough , right ?
> > +       }
> > +
> > +       req->cnt = tot;
> > +       idx = 0;
> > +       for (int i = 0; i < tot; idx++) {
> > +               i2idx_map[i] = idx;
> > +               req->entry[i++] = fl[idx].mcam_idx[0];
> > +               if (fl[idx].uni_di)
> > +                       continue;
> > +
> > +               i2idx_map[i] = idx;
> > +               req->entry[i++] = fl[idx].mcam_idx[1];
> > +       }
> > +
> > +       if (rvu_mbox_handler_npc_mcam_mul_stats(rvu, req, rsp)) {
> > +               dev_err(rvu->dev, "Error to get multiple stats\n");
> > +               rc = -EFAULT;
> You can add a new label and use goto free_resp
same comment as above.

>
> > +               goto fail;
> > +       }
> > +
> > +
> > +fail:
> > +       kfree(req);
> > +       kfree(rsp);
> > +       return rc;
> > +}
> > +
> > +       fl_entry->features = req->features;
> > +
> > +       mutex_lock(&fl_offl_llock);
> > +       list_add_tail(&fl_entry->list, &fl_offl_lh);
> > +       mutex_unlock(&fl_offl_llock);
> > +
> > +       if (!fl_offl_work_running) {
> > +               sw_fl_offl_wq = alloc_workqueue("sw_af_fl_wq", 0, 0);
> > +               if (!sw_fl_offl_wq)
> free fl_entry here? also, do you want to move list_add_tail() after
> this if() condition?
ACK.

> > +                       return -ENOMEM;
> > +
> >                   void *type_data)
> >  {
> > +       struct otx2_nic *nic = netdev_priv(netdev);
> > +
> >         switch (type) {
> >         case TC_SETUP_BLOCK:
> > +               if (netif_is_ovs_port(netdev)) {
> > +                       return flow_block_cb_setup_simple(type_data,
> > +                                                         &otx2_block_cb_list,
> > +                                                         sw_fl_setup_ft_block_ingress_cb,
> > +                                                         nic, nic, true);
> > +               }
> braces are not required here
ACK.

> > +
> >                 return otx2_setup_tc_block(netdev, type_data);
> > +}
> > +
> > +static int sw_fl_parse_actions(struct otx2_nic *nic,
> > +                              struct flow_action *flow_action,
> > +                              struct flow_cls_offload *f,
> > +                              struct fl_tuple *tuple, u64 *op)
> > +{
> > +       struct flow_action_entry *act;
> > +       struct otx2_nic *out_nic;
> > +       int err;
> > +       int used = 0;
> RCT order here
ACK.

> > +       int i;
> > +                       return rc;
> > +       }
> > +
> > +       sw_fl_add_to_list(nic, &tuple, f->cookie, true);
> > +       return 0;
> > +}
> > +
> > +static int sw_fl_del(struct otx2_nic *nic, struct flow_cls_offload *f)
> function prototype can be changed to void?
ACK.

> > +{