[dpdk-dev] [PATCH v2 4/4] regexdev: implement regex rte level functions

[Ori Kam]

Hi Guy,

> -----Original Message-----
> From: dev <dev-bounces at dpdk.org> On Behalf Of Guy Kaneti
> Sent: Tuesday, April 21, 2020 2:36 PM
> To: Ori Kam <orika at mellanox.com>; Jerin Jacob Kollanukkaran
> <jerinj at marvell.com>; xiang.w.wang at intel.com
> Cc: dev at dpdk.org; Pavan Nikhilesh Bhagavatula
> <pbhagavatula at marvell.com>; Shahaf Shuler <shahafs at mellanox.com>;
> hemant.agrawal at nxp.com; Opher Reviv <opher at mellanox.com>; Alex
> Rosenbaum <alexr at mellanox.com>; Dovrat Zifroni <dovrat at marvell.com>;
> Prasun Kapoor <pkapoor at marvell.com>; nipun.gupta at nxp.com;
> bruce.richardson at intel.com; yang.a.hong at intel.com; harry.chang at intel.com;
> gu.jian1 at zte.com.cn; shanjiangh at chinatelecom.cn;
> zhangy.yun at chinatelecom.cn; lixingfu at huachentel.com; wushuai at inspur.com;
> yuyingxia at yxlink.com; fanchenggang at sunyainfo.com;
> davidfgao at tencent.com; liuzhong1 at chinaunicom.cn;
> zhaoyong11 at huawei.com; oc at yunify.com; jim at netgate.com;
> hongjun.ni at intel.com; j.bromhead at titan-ic.com; deri at ntop.org;
> fc at napatech.com; arthur.su at lionic.com; Thomas Monjalon
> <thomas at monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] regexdev: implement regex rte level
> functions
> 
> Hi,
> 
> > +int
> > +rte_regexdev_configure(uint8_t dev_id, const struct rte_regexdev_config
> > +*cfg) {
> > +	struct rte_regexdev *dev;
> > +	struct rte_regexdev_info dev_info;
> > +	int ret;
> > +
> > +	RTE_REGEXDEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL);
> > +	if (cfg == NULL)
> > +		return -EINVAL;
> > +	dev = &rte_regex_devices[dev_id];
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> > ENOTSUP);
> > +	if (dev->data->dev_started) {
> > +		RTE_REGEXDEV_LOG
> > +			(ERR, "Dev %u must be stopped to allow
> > configuration\n",
> > +			 dev_id);
> > +		return -EBUSY;
> > +	}
> > +	ret = regexdev_info_get(dev_id, &dev_info);
> > +	if (ret < 0)
> > +		return ret;
> > +	if ((cfg->dev_cfg_flags &
> > RTE_REGEXDEV_CFG_CROSS_BUFFER_SCAN_F) &&
> > +	    !(dev_info.regexdev_capa &
> > RTE_REGEXDEV_SUPP_CROSS_BUFFER_F)) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u doesn't support cross buffer
> > scan\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if ((cfg->dev_cfg_flags & RTE_REGEXDEV_CFG_MATCH_AS_END_F)
> > &&
> > +	    !(dev_info.regexdev_capa &
> > RTE_REGEXDEV_SUPP_MATCH_AS_END_F)) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u doesn't support match as end\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if ((cfg->dev_cfg_flags & RTE_REGEXDEV_CFG_MATCH_ALL_F) &&
> > +	    !(dev_info.regexdev_capa &
> > RTE_REGEXDEV_SUPP_MATCH_ALL_F)) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u doesn't support match all\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_groups == 0) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of groups must be >
> > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_groups >= dev_info.max_groups) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of groups %d >
> > %d\n",
> > +				 dev_id, cfg->nb_groups,
> > dev_info.max_groups);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Yes, you are correct will fix.

> > +	if (cfg->nb_max_matches == 0) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of matches must be
> > > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_max_matches >= dev_info.max_matches) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of matches %d >
> > %d\n",
> > +				 dev_id, cfg->nb_max_matches,
> > +				 dev_info.max_matches);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Yes, will fix.

> > +	if (cfg->nb_queue_pairs == 0) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of queues must be
> > > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_queue_pairs >= dev_info.max_queue_pairs) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of queues %d >
> > %d\n",
> > +				 dev_id, cfg->nb_queue_pairs,
> > +				 dev_info.max_queue_pairs);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Will fix.

> > +	if (cfg->nb_rules_per_group == 0) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u num of rules per group must be >
> > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_rules_per_group >= dev_info.max_rules_per_group) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u num of rules per group %d > %d\n",
> > +				 dev_id, cfg->nb_rules_per_group,
> > +				 dev_info.max_rules_per_group);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Will fix.

> > +	ret = (*dev->dev_ops->dev_configure)(dev, cfg);
> > +	if (ret == 0)
> > +		dev->data->dev_conf = *cfg;
> > +	return ret;
> > +}
> 
> In general I think that the validation of the cfg values should be done by the
> PMD

This was done in the first version.
after comments from the community, I changed it.
As much as I like the idea that PMD should handle everything by itself.
there is no point of code duplication, all PMD will require to do those test,
and there is no advantage of doing it inside the PMD.
Also it is common practice in DPDK to assume that the input was tested in the above 
layer. (you can see ethdev)