Re: [dpdk-dev] [PATCH v6 1/2] ethdev: add pre-defined meter policy API

From: "Singh, Jasvinder" <jasvinder.singh@intel.com>
To: Li Zhang <lizh@nvidia.com>,
	"dekelp@nvidia.com" <dekelp@nvidia.com>,
	"orika@nvidia.com" <orika@nvidia.com>,
	"viacheslavo@nvidia.com" <viacheslavo@nvidia.com>,
	"matan@nvidia.com" <matan@nvidia.com>,
	"shahafs@nvidia.com" <shahafs@nvidia.com>,
	"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
	"lironh@marvell.com" <lironh@marvell.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
	Wisam Jaddo <wisamm@nvidia.com>,
	"Li, Xiaoyun" <xiaoyun.li@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	"Andrew Rybchenko" <andrew.rybchenko@oktetlabs.ru>,
	Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"rasland@nvidia.com" <rasland@nvidia.com>,
	 "roniba@nvidia.com" <roniba@nvidia.com>,
	Haifei Luo <haifeil@nvidia.com>,
	 Jiawei Wang <jiaweiw@nvidia.com>
Subject: Re: [dpdk-dev] [PATCH v6 1/2] ethdev: add pre-defined meter policy API
Date: Wed, 14 Apr 2021 22:21:35 +0000	[thread overview]
Message-ID: <CY4PR1101MB21343959476E10D20AA31EFFE04E9@CY4PR1101MB2134.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210414085800.2051439-2-lizh@nvidia.com>

<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");

Add check for "policy",  could be null, before dereferencing this.    

<snip>
> 
> -/* MTR object policer action update */
> +/* MTR object policy update */
>  static int
> -pmd_mtr_policer_actions_update(struct rte_eth_dev *dev,
> +pmd_mtr_meter_policy_update(struct rte_eth_dev *dev,
>  	uint32_t mtr_id,
> -	uint32_t action_mask,
> -	enum rte_mtr_policer_action *actions,
> +	uint32_t meter_policy_id,
>  	struct rte_mtr_error *error)
>  {
>  	struct pmd_internals *p = dev->data->dev_private;
>  	struct softnic_mtr *m;
>  	uint32_t i;
>  	int status;
> +	struct softnic_mtr_meter_policy *mp_new, *mp_old;
> 
>  	/* MTR object id must be valid */
>  	m = softnic_mtr_find(p, mtr_id);
> @@ -527,29 +652,14 @@ pmd_mtr_policer_actions_update(struct
> rte_eth_dev *dev,
>  			RTE_MTR_ERROR_TYPE_MTR_ID,
>  			NULL,
>  			"MTR object id not valid");
> -
> -	/* Valid policer actions */
> -	if (actions == NULL)
> +	/* Meter policy must exist */
> +	mp_new = softnic_mtr_meter_policy_find(p, meter_policy_id);
> +	if (mp_new == NULL)
>  		return -rte_mtr_error_set(error,
>  			EINVAL,
> -			RTE_MTR_ERROR_TYPE_UNSPECIFIED,
> +			RTE_MTR_ERROR_TYPE_METER_POLICY_ID,
>  			NULL,
> -			"Invalid actions");
> -
> -	for (i = 0; i < RTE_COLORS; i++) {
> -		if (action_mask & (1 << i)) {
> -			if (actions[i] !=
> MTR_POLICER_ACTION_COLOR_GREEN  &&
> -				actions[i] !=
> MTR_POLICER_ACTION_COLOR_YELLOW &&
> -				actions[i] !=
> MTR_POLICER_ACTION_COLOR_RED &&
> -				actions[i] != MTR_POLICER_ACTION_DROP) {
> -				return -rte_mtr_error_set(error,
> -					EINVAL,
> -
> 	RTE_MTR_ERROR_TYPE_UNSPECIFIED,
> -					NULL,
> -					" Invalid action value");
> -			}
> -		}
> -	}
> +			"Meter policy id invalid");

Please add check whether MTR object is already set to meter policy  id,  return success if true, no need to continue.
if (m->params.meter_policy_id == meter_policy_id)
		return 0;

>  	/* MTR object owner valid? */
>  	if (m->flow) {

Regards,
Jasvinder