Hi Stephen, Thank you for the review, see below,
Le 11/03/26 09:35, Stephen Hemminger a écrit : > On Wed, 11 Mar 2026 00:26:53 +0100 > Vincent Jardin <[email protected]> wrote: > > > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mlx5_pp_rate_table_query, 26.07) > > +int rte_pmd_mlx5_pp_rate_table_query(uint16_t port_id, > > + struct rte_pmd_mlx5_pp_rate_table_info > > *info) > > +{ > > + struct rte_eth_dev *dev; > > + struct mlx5_priv *priv; > > + uint16_t used = 0; > > + uint16_t *seen; > > + unsigned int i; > > + > > + if (info == NULL) > > + return -EINVAL; > > Prefer NULL checks in ethdev layer > > > + if (!rte_eth_dev_is_valid_port(port_id)) > > + return -ENODEV; > > Ditto checks for port_id should be at ethdev This function is a PMD-specific API declared in rte_pmd_mlx5.h, not an ethdev op. application -> rte_pmd_mlx5_pp_rate_table_query() -> mlx5 internals Therefore, the function must validate its own inputs: - port_id validity (via rte_eth_dev_get_by_name() / rte_eth_dev_is_valid_port()) - info != NULL Adding a generic ethdev op (ex eth_rate_table_query_t) for a concept only one driver supports would be over-engineering. If other drivers later expose a similar rate table concept, that would be the time to factor out a generic ethdev API. > > + dev = &rte_eth_devices[port_id]; > > + priv = dev->data->dev_private; > > + if (!priv->sh->cdev->config.hca_attr.qos.packet_pacing) { > > + rte_errno = ENOTSUP; > > + return -ENOTSUP; > > + } > > + info->total = priv->sh->cdev->config.hca_attr.qos > > + .packet_pacing_rate_table_size; > > Since DPDK allows 100 character lines now, don't need line break ok > > + if (priv->txqs == NULL || priv->txqs_n == 0) { > > + info->used = 0; > > + return 0; > > + } > > + seen = mlx5_malloc(MLX5_MEM_ZERO, priv->txqs_n * sizeof(*seen), > > + 0, SOCKET_ID_ANY); > > Since this only has lifetime of this function, use calloc() instead > since that avoids using huge page memory, and compiler and other checkers > "know about" malloc functions and engage more checks. Nope, I'll use mlx5_malloc(MLX5_MEM_SYS | MLX5_MEM_ZERO, ... in order to remain consistent with mlx5's cases I could grep despite there are still 3 other calloc() occurences that I did not analyze. In any cases, it'll end up with calloc() (mlx5_malloc()). Best regards, Vincent
