[dpdk-dev] [PATCH v4] ethdev: add sanity checks in control APIs
Min Hu (Connor)
humin29 at huawei.com
Fri Apr 16 09:00:39 CEST 2021
Thanks Kevin,
all is fixed in v6, please review it, thanks.
Some comments are below.
在 2021/4/15 20:04, Kevin Traynor 写道:
> On 15/04/2021 01:52, Min Hu (Connor) wrote:
>> This patch adds more sanity checks in control path APIs.
>>
>
> Hi Connor,
>
> A few general comments,
>
> --
> Some of the functions have unit tests, you could consider adding unit
> tests for the new checks. Considering the checks are not subtle and
> unlikely to be messed up in future, not adding unit tests is not a
> blocker imho.
>
> --
> It took me a while to get what you meant with "by NULL". It's usage
> seems like in "Death by taxes". Perhaps "because NULL ptr" would be a
> better way to phrase this generically, but I think it is more useful to
> say what is NULL.
>
> e.g. "Failed to convert NULL to string\n" is very generic and would be
> better as "Failed to convert NULL link to string\n" . ok, still a bit
> generic but more of a clue.
>
> I won't comment on each log message individually but I've added a few
> suggestions here and there.
> --
>
> Did you check the usage of these functions in DPDK, and if the return
> value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling
> iterator functions. I'm not sure that having a return check is needed in
> that case, but there could be other cases where you want to take some
> different action now.
>
As iterator functions are all APIs, they may be used by APP directly.
I think param check is necessary.
> some other comments inlined,
>
>> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
>> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
>> Fixes: 0366137722a0 ("ethdev: check for invalid device name")
>> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>> Fixes: f8244c6399d9 ("ethdev: increase port id range")
>> Cc: stable at dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29 at huawei.com>
>> ---
>> v4:
>> * add error logging.
>> * delete check in control path API.
>>
>> v3:
>> * set port_id checked first.
>> * add error logging.
>>
>> v2:
>> * Removed unnecessary checks.
>> * Deleted checks in internal API.
>> * Added documentation in the header file.
>> ---
>> lib/librte_ethdev/rte_ethdev.c | 288 ++++++++++++++++++++++++++++++++++++++---
>> lib/librte_ethdev/rte_ethdev.h | 20 ++-
>> 2 files changed, 287 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 6b5cfd6..e734a30 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -199,6 +199,16 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>> char *cls_str = NULL;
>> int str_size;
>>
>> + if (iter == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to iterator init for NULL\n");
>
> "Failed to init iterator for NULL iterator\n"
>
>> + return -EINVAL;
>> + }
>> +
>> + if (devargs_str == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to iterate matching NULL\n");
>
> "Failed to init iterator for NULL devargs\n"
>
>> + return -EINVAL;
>> + }
>> +
>> memset(iter, 0, sizeof(*iter));
>>
>> /*
>> @@ -293,6 +303,11 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>> uint16_t
>> rte_eth_iterator_next(struct rte_dev_iterator *iter)
>> {
>> + if (iter == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to iterate next for NULL\n");
>> + return RTE_MAX_ETHPORTS;
>> + }
>> +
>> if (iter->cls == NULL) /* invalid ethdev iterator */
>> return RTE_MAX_ETHPORTS;
>>
>> @@ -322,6 +337,11 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
>> void
>> rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
>> {
>> + if (iter == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to iterator clear up for NULL\n");
>> + return;
>> + }
>> +
>> if (iter->bus_str == NULL)
>> return; /* nothing to free in pure class filter */
>> free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
>> @@ -622,6 +642,11 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
>> int
>> rte_eth_dev_owner_new(uint64_t *owner_id)
>> {
>> + if (owner_id == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get owner id by NULL\n");
>> + return -EINVAL;
>> + }
>> +
>> eth_dev_shared_data_prepare();
>>
>> rte_spinlock_lock(ð_dev_shared_data->ownership_lock);
>> @@ -645,6 +670,12 @@ eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
>> return -ENODEV;
>> }
>>
>> + if (new_owner == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> if (!eth_is_valid_owner_id(new_owner->id) &&
>> !eth_is_valid_owner_id(old_owner_id)) {
>> RTE_ETHDEV_LOG(ERR,
>> @@ -738,23 +769,30 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
>> int
>> rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
>> {
>> - int ret = 0;
>> - struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
>> -
>> - eth_dev_shared_data_prepare();
>> + struct rte_eth_dev *ethdev;
>>
>> - rte_spinlock_lock(ð_dev_shared_data->ownership_lock);
>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
>>
>> - if (port_id >= RTE_MAX_ETHPORTS || !eth_dev_is_allocated(ethdev)) {
>> + ethdev = &rte_eth_devices[port_id];
>> + if (!eth_dev_is_allocated(ethdev)) {
>> RTE_ETHDEV_LOG(ERR, "Port id %"PRIu16" is not allocated\n",
>> port_id);
>> - ret = -ENODEV;
>> - } else {
>> - rte_memcpy(owner, ðdev->data->owner, sizeof(*owner));
>> + return -ENODEV;
>> }
>>
>> + if (owner == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u owner by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>
> This fn uses both %u and %"PRIu16" for port_id
>
>> +
>> + eth_dev_shared_data_prepare();
>> +
>> + rte_spinlock_lock(ð_dev_shared_data->ownership_lock);
>> + rte_memcpy(owner, ðdev->data->owner, sizeof(*owner));
>> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>> - return ret;
>> +
>> + return 0;
>> }
>>
>> int
>> @@ -820,7 +858,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>> {
>> uint16_t pid;
>>
>> - if (name == NULL) {
>> + if (name == NULL || port_id == NULL) {
>
> Most of the other new checks have individual checks for NULL ptrs, this
> one doesn't. I'm not sure if it needs individual checks, but it is
> better to be consistent.
>
>> RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
>> return -EINVAL;
>> }
>> @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (dev_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
>> @@ -2139,6 +2183,12 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Rx hairpin queue to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> if (rx_queue_id >= dev->data->nb_rx_queues) {
>> RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
>> @@ -2310,6 +2360,13 @@ rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>> int ret;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Tx hairpin queue to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> if (tx_queue_id >= dev->data->nb_tx_queues) {
>> RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
>> @@ -2459,6 +2516,11 @@ int
>> rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>> buffer_tx_error_fn cbfn, void *userdata)
>> {
>> + if (buffer == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set error callback for NULL\n");
>> + return -EINVAL;
>> + }
>> +
>> buffer->error_callback = cbfn;
>> buffer->error_userdata = userdata;
>> return 0;
>> @@ -2607,6 +2669,13 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (eth_link == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u link by NULL\n",
>
> "by NULL" does not read nicely to me. How about
> "Failed to get ethdev port %u for NULL link"
>
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> if (dev->data->dev_conf.intr_conf.lsc &&
>> @@ -2627,6 +2696,13 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (eth_link == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u nowait link by NULL\n",
>> + port_id);
>
> Same comment as for previous function, except you'll probably want to
> keep nowait somewhere to distinguish.
> "Failed to get nowait ethdev port %u for NULL link" ?
>
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> if (dev->data->dev_conf.intr_conf.lsc &&
>> @@ -2667,6 +2743,16 @@ rte_eth_link_speed_to_str(uint32_t link_speed)
>> int
>> rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link)
>> {
>
> The messages are very generic, especially the second one. Suggestions below
>
>> + if (str == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to convert link to NULL\n");
>
> "Failed to convert link to NULL string\n"
>
>> + return -EINVAL;
>> + }
>
> You should probably also check the 'len' param for str here also.
>
>> +
>> + if (eth_link == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to convert NULL to string\n");
>
> "Failed to convert NULL link to string\n"
>
>> + return -EINVAL;
>> + }
>> +
>> if (eth_link->link_status == ETH_LINK_DOWN)
>> return snprintf(str, len, "Link down");
>> else
>> @@ -2685,6 +2771,12 @@ rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (stats == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u stats by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> memset(stats, 0, sizeof(*stats));
>>
>> @@ -3258,6 +3350,13 @@ rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (fw_version == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u fw version by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>
> Some of the drivers already check for NULL ptrs and some don't. It means
> some checks could be removed from the drivers, but it doesn't seem a
> worthwhile effort unless they are needing some rework anyway.
Agreed, this may be fixed in next patch.
>
> 'fw_size == 0' could also be checked here
>
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP);
>> @@ -3278,6 +3377,14 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>> };
>> int diag;
>>
>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (dev_info == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u info by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> /*
>> * Init dev_info before port_id check since caller does not have
>> * return status and does not know if get is successful or not.
>> @@ -3285,7 +3392,6 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>> memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>> dev_info->switch_info.domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
>>
>> - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
> This is a slight change in behaviour. With an invalid port, previously
> dev_info was memset to 0. Does it need to be? I guess no.
>
> At the very least the comment block is incorrect and should be changed.
>
>> dev = &rte_eth_devices[port_id];
>>
>> dev_info->rx_desc_lim = lim;
>> @@ -3326,6 +3432,13 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>> const uint32_t *all_ptypes;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (ptypes == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u supported ptypes by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>
> 'num == 0' could be checked here too.
>
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0);
>> all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
>> @@ -3435,6 +3548,13 @@ rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (mac_addr == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MAC address by NULL\n",
>
> "by NULL" doesn't read correctly.
> "Failed to get ethdev port %u MAC address for NULL param\n"
>
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> rte_ether_addr_copy(&dev->data->mac_addrs[0], mac_addr);
>>
>> @@ -3448,6 +3568,12 @@ rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu)
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (mtu == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MTU by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> *mtu = dev->data->mtu;
>> return 0;
>> @@ -3696,6 +3822,13 @@ rte_eth_dev_flow_ctrl_get(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (fc_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u flow conf by NULL\n",
>
> I would at least add "flow *ctrl* conf" e.g.
> "Failed to get ethdev port %u flow ctrl conf for NULL param\n"
>
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->flow_ctrl_get, -ENOTSUP);
>> memset(fc_conf, 0, sizeof(*fc_conf));
>> @@ -3708,6 +3841,13 @@ rte_eth_dev_flow_ctrl_set(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (fc_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u flow conf to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> if ((fc_conf->send_xon != 0) && (fc_conf->send_xon != 1)) {
>> RTE_ETHDEV_LOG(ERR, "Invalid send_xon, only 0/1 allowed\n");
>> return -EINVAL;
>> @@ -3725,6 +3865,13 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (pfc_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u priority flow conf to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> if (pfc_conf->priority > (ETH_DCB_NUM_USER_PRIORITIES - 1)) {
>> RTE_ETHDEV_LOG(ERR, "Invalid priority, only 0-7 allowed\n");
>> return -EINVAL;
>> @@ -3744,9 +3891,6 @@ eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
>> {
>> uint16_t i, num;
>>
>> - if (!reta_conf)
>> - return -EINVAL;
>> -
>> num = (reta_size + RTE_RETA_GROUP_SIZE - 1) / RTE_RETA_GROUP_SIZE;
>> for (i = 0; i < num; i++) {
>> if (reta_conf[i].mask)
>> @@ -3763,9 +3907,6 @@ eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
>> {
>> uint16_t i, idx, shift;
>>
>> - if (!reta_conf)
>> - return -EINVAL;
>> -
>> if (max_rxq == 0) {
>> RTE_ETHDEV_LOG(ERR, "No receive queue is available\n");
>> return -EINVAL;
>> @@ -3796,6 +3937,13 @@ rte_eth_dev_rss_reta_update(uint16_t port_id,
>> int ret;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (reta_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss reta to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>
> You may want to check reta_size as well in these rss functions?
>
>> /* Check mask bits */
>> ret = eth_check_reta_mask(reta_conf, reta_size);
>> if (ret < 0)
>> @@ -3824,6 +3972,12 @@ rte_eth_dev_rss_reta_query(uint16_t port_id,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (reta_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to query ethdev port %u rss reta by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> /* Check mask bits */
>> ret = eth_check_reta_mask(reta_conf, reta_size);
>> if (ret < 0)
>> @@ -3845,6 +3999,12 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (rss_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss hash to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> ret = rte_eth_dev_info_get(port_id, &dev_info);
>> if (ret != 0)
>> return ret;
>> @@ -3872,6 +4032,13 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (rss_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u rss hash conf by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
>> return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
>> @@ -4027,6 +4194,13 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>> int ret;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (addr == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to add ethdev port %u MAC address to NULL\n",
>> + port_id);
>
> This message is a bit confusing. Perhaps,
> "Failed to add NULL MAC address to ethdev port %u\n"
>
> thanks,
> Kevin.
>
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
>>
>> @@ -4077,6 +4251,13 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>> int index;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (addr == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to remove ethdev port %u MAC address by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
>>
>> @@ -4109,6 +4290,12 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (addr == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u default MAC address to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> if (!rte_is_valid_assigned_ether_addr(addr))
>> return -EINVAL;
>>
>> @@ -4164,6 +4351,12 @@ rte_eth_dev_uc_hash_table_set(uint16_t port_id, struct rte_ether_addr *addr,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (addr == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u uc hash table to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> if (rte_is_zero_ether_addr(addr)) {
>> RTE_ETHDEV_LOG(ERR, "Port %u: Cannot add NULL MAC address\n",
>> @@ -4265,6 +4458,13 @@ rte_eth_mirror_rule_set(uint16_t port_id,
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (mirror_conf == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u mirror rule to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> if (mirror_conf->rule_type == 0) {
>> RTE_ETHDEV_LOG(ERR, "Mirror rule type can not be 0\n");
>> return -EINVAL;
>> @@ -5208,6 +5408,13 @@ rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (timestamp == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Rx timestamp by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_rx_timestamp, -ENOTSUP);
>> @@ -5222,6 +5429,13 @@ rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (timestamp == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Tx timestamp by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_tx_timestamp, -ENOTSUP);
>> @@ -5248,6 +5462,13 @@ rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (timestamp == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u time by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_time, -ENOTSUP);
>> @@ -5261,6 +5482,13 @@ rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (timestamp == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to write ethdev port %u time to NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_write_time, -ENOTSUP);
>> @@ -5274,6 +5502,13 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + if (clock == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u clock by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>>
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->read_clock, -ENOTSUP);
>> @@ -5372,6 +5607,12 @@ rte_eth_dev_get_dcb_info(uint16_t port_id,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (dcb_info == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u dcb info by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> memset(dcb_info, 0, sizeof(struct rte_eth_dcb_info));
>>
>> @@ -5423,6 +5664,12 @@ rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> + if (cap == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u hairpin capability by NULL\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_cap_get, -ENOTSUP);
>> memset(cap, 0, sizeof(*cap));
>> @@ -5629,6 +5876,11 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>> struct rte_eth_representor_info *info = NULL;
>> size_t size;
>>
>> + if (ethdev == NULL) {
>> + RTE_ETHDEV_LOG(ERR, "Failed to get device representor info from NULL\n");
>> + return -EINVAL;
>> + }
>> +
>> if (type == RTE_ETH_REPRESENTOR_NONE)
>> return 0;
>> if (repr_id == NULL)
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 3b773b6..c1e5d4b 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -2702,6 +2702,7 @@ int rte_eth_allmulticast_get(uint16_t port_id);
>> * - (0) if successful.
>> * - (-ENOTSUP) if the function is not supported in PMD driver.
>> * - (-ENODEV) if *port_id* invalid.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>>
>> @@ -2717,6 +2718,7 @@ int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>> * - (0) if successful.
>> * - (-ENOTSUP) if the function is not supported in PMD driver.
>> * - (-ENODEV) if *port_id* invalid.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *link);
>>
>> @@ -2752,7 +2754,7 @@ const char *rte_eth_link_speed_to_str(uint32_t link_speed);
>> * @param eth_link
>> * Link status returned by rte_eth_link_get function
>> * @return
>> - * Number of bytes written to str array.
>> + * Number of bytes written to str array or -EINVAL if bad parameter.
>> */
>> __rte_experimental
>> int rte_eth_link_to_str(char *str, size_t len,
>> @@ -2997,6 +2999,7 @@ int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
>> * @return
>> * - (0) if successful
>> * - (-ENODEV) if *port_id* invalid.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>>
>> @@ -3041,6 +3044,7 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>> * - (0) if successful.
>> * - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
>> * - (-ENODEV) if *port_id* invalid.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>>
>> @@ -3060,6 +3064,7 @@ int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>> * - (-ENOTSUP) if operation is not supported.
>> * - (-ENODEV) if *port_id* invalid.
>> * - (-EIO) if device is removed.
>> + * - (-EINVAL) if bad parameter.
>> * - (>0) if *fw_size* is not enough to store firmware version, return
>> * the size of the non truncated string.
>> */
>> @@ -3103,6 +3108,7 @@ int rte_eth_dev_fw_version_get(uint16_t port_id,
>> * only num entries will be filled into the ptypes array, but the full
>> * count of supported ptypes will be returned.
>> * - (-ENODEV) if *port_id* invalid.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>> uint32_t *ptypes, int num);
>> @@ -3153,6 +3159,7 @@ int rte_eth_dev_set_ptypes(uint16_t port_id, uint32_t ptype_mask,
>> * @return
>> * - (0) if successful.
>> * - (-ENODEV) if *port_id* invalid.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu);
>>
>> @@ -3347,7 +3354,7 @@ rte_eth_tx_buffer_init(struct rte_eth_dev_tx_buffer *buffer, uint16_t size);
>> * @param userdata
>> * Arbitrary parameter to be passed to the callback function
>> * @return
>> - * 0 on success, or -1 on error with rte_errno set appropriately
>> + * 0 on success, or -EINVAL if bad parameter
>> */
>> int
>> rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>> @@ -3774,6 +3781,7 @@ int rte_eth_fec_set(uint16_t port_id, uint32_t fec_capa);
>> * - (-ENOTSUP) if hardware doesn't support flow control.
>> * - (-ENODEV) if *port_id* invalid.
>> * - (-EIO) if device is removed.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_dev_flow_ctrl_get(uint16_t port_id,
>> struct rte_eth_fc_conf *fc_conf);
>> @@ -3845,7 +3853,8 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *mac_addr,
>> * - (0) if successful, or *mac_addr* didn't exist.
>> * - (-ENOTSUP) if hardware doesn't support.
>> * - (-ENODEV) if *port* invalid.
>> - * - (-EADDRINUSE) if attempting to remove the default MAC address
>> + * - (-EADDRINUSE) if attempting to remove the default MAC address.
>> + * - (-EINVAL) if MAC address is invalid.
>> */
>> int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>> struct rte_ether_addr *mac_addr);
>> @@ -4044,6 +4053,7 @@ int rte_eth_dev_rss_hash_update(uint16_t port_id,
>> * - (-ENODEV) if port identifier is invalid.
>> * - (-EIO) if device is removed.
>> * - (-ENOTSUP) if hardware doesn't support RSS.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int
>> rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>> @@ -4112,6 +4122,7 @@ rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id,
>> * - (-ENODEV) if port identifier is invalid.
>> * - (-EIO) if device is removed.
>> * - (-ENOTSUP) if hardware doesn't support.
>> + * - (-EINVAL) if bad parameter.
>> */
>> int rte_eth_dev_get_dcb_info(uint16_t port_id,
>> struct rte_eth_dcb_info *dcb_info);
>> @@ -4628,6 +4639,7 @@ int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
>> *
>> * @return
>> * - 0: Success.
>> + * - -EINVAL: Bad parameter.
>> */
>> int rte_eth_timesync_read_time(uint16_t port_id, struct timespec *time);
>>
>> @@ -4694,6 +4706,7 @@ int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *time);
>> * - 0: Success.
>> * - -ENODEV: The port ID is invalid.
>> * - -ENOTSUP: The function is not supported by the Ethernet driver.
>> + * - -EINVAL: if bad parameter.
>> */
>> __rte_experimental
>> int
>> @@ -4797,6 +4810,7 @@ rte_eth_dev_get_sec_ctx(uint16_t port_id);
>> * @return
>> * - (0) if successful.
>> * - (-ENOTSUP) if hardware doesn't support.
>> + * - (-EINVAL) if bad parameter.
>> */
>> __rte_experimental
>> int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>>
>
> .
>
More information about the dev
mailing list