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

[Guy Kaneti <guyk@marvell.com>]

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 >=

> +	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 >=

> +	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 >=

> +	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 >=

> +	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