Thanks Jasvinder's comments!
Li is on vacation so I help to update the code changes based on your comments,
V9 is sent with your ack.
> -----Original Message-----
> From: Singh, Jasvinder <jasvinder.si...@intel.com>
> Sent: Monday, April 19, 2021 8:35 PM
> To: Li Zhang <l...@nvidia.com>; dek...@nvidia.com; Ori Kam
> <or...@nvidia.com>; Slava Ovsiienko <viachesl...@nvidia.com>; Matan
> Azrad <ma...@nvidia.com>; Shahaf Shuler <shah...@nvidia.com>;
> Dumitrescu, Cristian <cristian.dumitre...@intel.com>; lir...@marvell.com;
> jer...@marvell.com; Yigit, Ferruh <ferruh.yi...@intel.com>;
> ajit.khapa...@broadcom.com; Wisam Monther <wis...@nvidia.com>; Li,
> Xiaoyun <xiaoyun...@intel.com>; NBU-Contact-Thomas Monjalon
> <tho...@monjalon.net>; Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>; Ray Kinsella <m...@ashroe.eu>; Neil
> Horman <nhor...@tuxdriver.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasl...@nvidia.com>; Roni Bar Yanai
> <ron...@nvidia.com>; Haifei Luo <haif...@nvidia.com>; Jiawei(Jonny) Wang
> <jiaw...@nvidia.com>
> Subject: RE: [PATCH v8 1/2] ethdev: add pre-defined meter policy API
>
> <Snip>
>
> > +/* MTR meter policy add */
> > +static int
> > +pmd_mtr_meter_policy_add(struct rte_eth_dev *dev,
> > + uint32_t meter_policy_id,
> > + struct rte_mtr_meter_policy_params *policy,
> > + struct rte_mtr_error *error)
> > +{
> > + struct pmd_internals *p = dev->data->dev_private;
> > + struct softnic_mtr_meter_policy_list *mpl = &p-
> > >mtr.meter_policies;
> > + struct softnic_mtr_meter_policy *mp;
> > + const struct rte_flow_action *act;
> > + const struct rte_flow_action_meter_color *recolor;
> > + uint32_t i;
> > + bool valid_act_found;
> > +
> > + if (policy == NULL)
> > + return -rte_mtr_error_set(error,
> > + EINVAL,
> > + RTE_MTR_ERROR_TYPE_METER_POLICY,
> > + NULL,
> > + "Null meter policy invalid");
> > +
> > + /* Meter policy must not exist. */
> > + mp = softnic_mtr_meter_policy_find(p, meter_policy_id);
> > + if (mp != NULL)
> > + return -rte_mtr_error_set(error,
> > + EINVAL,
> > + RTE_MTR_ERROR_TYPE_METER_POLICY_ID,
> > + NULL,
> > + "Meter policy already exists");
> > +
> > + for (i = 0; i < RTE_COLORS; i++) {
> > + if (policy->actions[i] == NULL)
> > + return -rte_mtr_error_set(error,
> > + EINVAL,
> > + RTE_MTR_ERROR_TYPE_METER_POLICY,
> > + NULL,
> > + "Null action list");
> > + for (act = policy->actions[i], valid_act_found = false;
> > + act && act->type != RTE_FLOW_ACTION_TYPE_END;
> > + act++) {
>
> Hi Li,
> No need to check "act" in for loop instruction as it is validated before the
> for
> loop. Also, would be good to skip all the steps inside this loop for the
> actions
> like RTE_FLOW_ACTION_TYPE_VOID. After for loop, will be good to check
> "valid_act_found" is true else return an error for that color action.
>
> Rest seems good to me
>
> With above fix for softnic-
> Acked-by: Jasvinder Singh <jasvinder.si...@intel.com>
>
>
