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
