[PATCH v1 0/2] ethdev: clarify something about new event

[PATCH v1 0/2] ethdev: clarify something about new event

[Huisong Li]

I've had some issues when I add the verification of the port id in the
event callback, which are discussed in another patch series[1]. So this
series clarify something about RTE_ETH_EVENT_NEW based on the previous
discussion.

[1] https://patches.dpdk.org/project/dpdk/cover/20250113025521.32703-1-lihuisong@huawei.com/

Huisong Li (2):
  ethdev: clarify do not something in the new event
  ethdev: fix some APIs can be used in the new event

 lib/ethdev/rte_ethdev.c | 14 +++++++++++---
 lib/ethdev/rte_ethdev.h |  9 ++++++++-
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.22.0

[PATCH v1 1/2] ethdev: clarify do not something in the new event

[Huisong Li]

If application verify the validity of the port id or configure this port in
the new event callback, application may happen to the port id is invalid.

Actually, when application receive a new event from one port, the port is
not fully probed and is just in allocated state. Application doesn't need
to verify the validity of the port id because it is definitely valid.
What's more, application shouldn't do something like configuring this port
or querying some information of this port by ethdev ops.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 lib/ethdev/rte_ethdev.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 1f71cad244..e2021f0f12 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4128,7 +4128,14 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_VF_MBOX,  /**< message from the VF received by PF */
 	RTE_ETH_EVENT_MACSEC,   /**< MACsec offload related event */
 	RTE_ETH_EVENT_INTR_RMV, /**< device removal event */
-	RTE_ETH_EVENT_NEW,      /**< port is probed */
+	/** Port is probed and application's event callback will be called.
+	 * In this moment, the port is not fully probed and is just in allocated
+	 * state. When application receive this event, application doesn't need
+	 * to verify the validity of the port id because it is definitely valid.
+	 * What's more, application shouldn't do something like configuring this
+	 * port or querying some information of this port by ethdev ops.
+	 */
+	RTE_ETH_EVENT_NEW,
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
-- 
2.22.0

[PATCH v1 2/2] ethdev: fix some APIs can be used in the new event

[Huisong Li]

The rte_eth_dev_socket_id() and rte_eth_dev_owner_*() can be used after
allocating an ethdev. So this patch relaxes the conditions for using them.

Fixes: 7dcd73e37965 ("drivers/bus: set device NUMA node to unknown by default")
Fixes: 53ef1b34776b ("ethdev: add sanity checks in control APIs")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6413c54e3b..9cfb397cee 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -600,9 +600,10 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 	struct rte_eth_dev *ethdev;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-	ethdev = &rte_eth_devices[port_id];
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return -ENODEV;
 
+	ethdev = &rte_eth_devices[port_id];
 	if (!eth_dev_is_allocated(ethdev)) {
 		RTE_ETHDEV_LOG_LINE(ERR, "Port ID %"PRIu16" is not allocated",
 			port_id);
@@ -635,8 +636,15 @@ int
 rte_eth_dev_socket_id(uint16_t port_id)
 {
 	int socket_id = SOCKET_ID_ANY;
+	struct rte_eth_dev *ethdev;
 
-	if (!rte_eth_dev_is_valid_port(port_id)) {
+	if (port_id >= RTE_MAX_ETHPORTS) {
+		rte_errno = EINVAL;
+		return socket_id;
+	}
+
+	ethdev = &rte_eth_devices[port_id];
+	if (!eth_dev_is_allocated(ethdev)) {
 		rte_errno = EINVAL;
 	} else {
 		socket_id = rte_eth_devices[port_id].data->numa_node;
-- 
2.22.0

Re: [PATCH v1 1/2] ethdev: clarify do not something in the new event

[Thomas Monjalon]

15/01/2025 04:41, Huisong Li:
> If application verify the validity of the port id or configure this port in
> the new event callback, application may happen to the port id is invalid.
> 
> Actually, when application receive a new event from one port, the port is
> not fully probed and is just in allocated state. Application doesn't need
> to verify the validity of the port id because it is definitely valid.
> What's more, application shouldn't do something like configuring this port
> or querying some information of this port by ethdev ops.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 1f71cad244..e2021f0f12 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4128,7 +4128,14 @@ enum rte_eth_event_type {
>  	RTE_ETH_EVENT_VF_MBOX,  /**< message from the VF received by PF */
>  	RTE_ETH_EVENT_MACSEC,   /**< MACsec offload related event */
>  	RTE_ETH_EVENT_INTR_RMV, /**< device removal event */
> -	RTE_ETH_EVENT_NEW,      /**< port is probed */
> +	/** Port is probed and application's event callback will be called.

We are not going to say that the callback is called for each event :)

> +	 * In this moment, the port is not fully probed and is just in allocated
> +	 * state. When application receive this event, application doesn't need

It is not a real state.

> +	 * to verify the validity of the port id because it is definitely valid.
> +	 * What's more, application shouldn't do something like configuring this
> +	 * port or querying some information of this port by ethdev ops.
> +	 */

Let me try shorter:
"
The port is being probed, i.e. allocated and not yet available.
It is too early to check validity, infos, or configuring the port.
"

What do you think? Anything missing?

Re: [PATCH v1 2/2] ethdev: fix some APIs can be used in the new event

[Thomas Monjalon]

15/01/2025 04:41, Huisong Li:
> The rte_eth_dev_socket_id() and rte_eth_dev_owner_*() can be used after
> allocating an ethdev. So this patch relaxes the conditions for using them.

You should be more explicit:
"during probing, before it becomes generally available and considered as valid".

Should we add these functions in the comment for RTE_ETH_EVENT_NEW?