Thanks Cristian. Will change it in V7 patch. Regards, Li Zhang
> -----Original Message----- > From: Dumitrescu, Cristian <[email protected]> > Sent: Thursday, April 15, 2021 12:16 AM > To: Li Zhang <[email protected]>; [email protected]; Ori Kam > <[email protected]>; Slava Ovsiienko <[email protected]>; Matan > Azrad <[email protected]>; Shahaf Shuler <[email protected]>; > [email protected]; [email protected]; Yigit, Ferruh > <[email protected]>; [email protected]; Wisam Monther > <[email protected]>; Li, Xiaoyun <[email protected]>; Singh, > Jasvinder <[email protected]>; NBU-Contact-Thomas Monjalon > <[email protected]>; Andrew Rybchenko > <[email protected]>; Ray Kinsella <[email protected]>; Neil > Horman <[email protected]>; Jerin Jacob <[email protected]>; > Hemant Agrawal <[email protected]> > Cc: [email protected]; Raslan Darawsheh <[email protected]>; Roni Bar Yanai > <[email protected]>; Haifei Luo <[email protected]>; Jiawei(Jonny) Wang > <[email protected]> > Subject: RE: [PATCH v6 1/2] ethdev: add pre-defined meter policy API > > External email: Use caution opening links or attachments > > > Hi Li, > > Following the API changes, there are lots of changes in the drivers, as > expected, so we'll have to take the necessary time to review them. > > Here are just a few comments below, please expect more during the next > few days. > > <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; > > + > > + /* Meter policy ID must be valid. */ > > + if (meter_policy_id == UINT32_MAX) > > + return -rte_mtr_error_set(error, > > + EINVAL, > > + RTE_MTR_ERROR_TYPE_METER_POLICY_ID, > > + NULL, > > + "Meter policy id not valid"); > > This is obviously not correct, we need to check whether the meter_policy_id > provided by the user is already in use (by a policy previously added) or not. > You can do this with the policy find function that you have already > implemented. > > > + > > + for (i = 0; i < RTE_COLORS; i++) { > > + act = policy->actions[i]; > > + if (act && act->type != > > RTE_FLOW_ACTION_TYPE_METER_COLOR && > > + act->type != RTE_FLOW_ACTION_TYPE_DROP) > > + return -rte_mtr_error_set(error, > > + EINVAL, > > + RTE_MTR_ERROR_TYPE_METER_POLICY, > > + NULL, > > + "Action invalid"); > > + } > > This check does not look right either: obviously we cannot accept a null > action list for any color, plus the action list should contain only those > action > types we support (RTE_FLOW_ACTION_TYPE_METER_COLOR or > RTE_FLOW_ACTION_TYPE_DROP). > > I agree, fist you need to check that the linked list of policy actions for > each > color is non-NULL, then you need to traverse it until you meet the END > action, skip any PAD actions, then check that exactly one (and one only) of > METER_COLOR or DROP exist, but not both at the same time, and also we > don't have the same action showing up multiple times. Makes sense? > > Regards, > Cristian
