[PATCH v1 0/2] ethdev: fix skip valid port in probing callback

[PATCH v1 0/2] ethdev: fix skip valid port in probing callback

[Huisong Li]

After we check the validity of the port in eth_event_callback() of testpmd
like patch 1/2, testpmd attach one port and doesn't update total port
number maintained in testmd. See the following log:
---
testpmd>
testpmd> port attach 0000:7d:00.1
Attaching a new port...
testpmd>
testpmd> port start all
Port 0 is now not stopped
ETHDEV: Device with port_id=1 is not configured.
Fail to start port 1: Invalid argument
Done
---

For details about the root cause of this issue, please see the patch 2/2.
After merging the patch 2/2. this issue can be fixed and the log is as
follows:
---
testpmd>
testpmd> port attach 0000:7d:00.1
Attaching a new port...
Port 1 is attached. Now total ports is 2
Done
---

BTW, the modification method for this issue is from the following thread.
https://mails.dpdk.org/archives/dev/2024-January/286026.html

Huisong Li (2):
  app/testpmd: check the validity of the port
  ethdev: fix skip valid port in probing callback

 app/test-pmd/testpmd.c           |  5 +++--
 drivers/net/bnxt/bnxt_ethdev.c   |  2 +-
 drivers/net/cpfl/cpfl_ethdev.h   |  2 +-
 drivers/net/ionic/ionic_ethdev.c |  2 +-
 drivers/net/mlx5/mlx5.c          |  2 +-
 drivers/net/nfp/nfp_ethdev.c     |  4 ++--
 lib/ethdev/ethdev_driver.c       | 13 ++++++++++---
 lib/ethdev/ethdev_driver.h       | 12 ++++++++++++
 lib/ethdev/ethdev_pci.h          |  2 +-
 lib/ethdev/rte_class_eth.c       |  2 +-
 lib/ethdev/rte_ethdev.c          |  4 ++--
 lib/ethdev/rte_ethdev.h          |  4 +++-
 lib/ethdev/version.map           |  1 +
 13 files changed, 39 insertions(+), 16 deletions(-)

-- 
2.22.0

[PATCH v1 1/2] app/testpmd: check the validity of the port

[Huisong Li]

This patch checks the validity of port id for all events in
'eth_event_callback()'.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/testpmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ac654048df..cc13b06ddb 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3746,14 +3746,15 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		fflush(stdout);
 	}
 
+	if (port_id_is_invalid(port_id, DISABLED_WARN))
+		return 0;
+
 	switch (type) {
 	case RTE_ETH_EVENT_NEW:
 		ports[port_id].need_setup = 1;
 		ports[port_id].port_status = RTE_PORT_HANDLING;
 		break;
 	case RTE_ETH_EVENT_INTR_RMV:
-		if (port_id_is_invalid(port_id, DISABLED_WARN))
-			break;
 		if (rte_eal_alarm_set(100000,
 				rmv_port_callback, (void *)(intptr_t)port_id))
 			fprintf(stderr,
-- 
2.22.0

[PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Huisong Li]

The event callback in application may use the macro RTE_ETH_FOREACH_DEV to
iterate over all enabled ports to do something(like, verifying the port id
validity) when receive a probing event. If the ethdev state of a port is
not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.

However, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing
event. It means that probing callback will skip this port. But this
assignment can not move to front of probing notification. See
commit be8cd210379a ("ethdev: fix port probing notification")

So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev
state to RTE_ETH_DEV_ALLOCATED before pushing probing event and set it to
RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid if its
device state is 'ALLOCATED' or 'ATTACHED'.

In addition, the new state has to be placed behind 'REMOVED' to avoid ABI
break. Fortunately, this ethdev state is internal and applications can not
access it directly. So this patch encapsulates an API, rte_eth_dev_is_used,
for ethdev or PMD to call and eliminate concerns about using this state
enum value comparison.

Fixes: be8cd210379a ("ethdev: fix port probing notification")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/bnxt/bnxt_ethdev.c   |  2 +-
 drivers/net/cpfl/cpfl_ethdev.h   |  2 +-
 drivers/net/ionic/ionic_ethdev.c |  2 +-
 drivers/net/mlx5/mlx5.c          |  2 +-
 drivers/net/nfp/nfp_ethdev.c     |  4 ++--
 lib/ethdev/ethdev_driver.c       | 13 ++++++++++---
 lib/ethdev/ethdev_driver.h       | 12 ++++++++++++
 lib/ethdev/ethdev_pci.h          |  2 +-
 lib/ethdev/rte_class_eth.c       |  2 +-
 lib/ethdev/rte_ethdev.c          |  4 ++--
 lib/ethdev/rte_ethdev.h          |  4 +++-
 lib/ethdev/version.map           |  1 +
 12 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index ef8a928c91..1441194b85 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -6706,7 +6706,7 @@ bnxt_dev_uninit(struct rte_eth_dev *eth_dev)
 
 	PMD_DRV_LOG_LINE(DEBUG, "Calling Device uninit");
 
-	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
+	if (rte_eth_dev_is_used(eth_dev->state))
 		bnxt_dev_close_op(eth_dev);
 
 	return 0;
diff --git a/drivers/net/cpfl/cpfl_ethdev.h b/drivers/net/cpfl/cpfl_ethdev.h
index 9a38a69194..aad05aafd6 100644
--- a/drivers/net/cpfl/cpfl_ethdev.h
+++ b/drivers/net/cpfl/cpfl_ethdev.h
@@ -328,7 +328,7 @@ cpfl_get_itf_by_port_id(uint16_t port_id)
 	}
 
 	dev = &rte_eth_devices[port_id];
-	if (dev->state == RTE_ETH_DEV_UNUSED) {
+	if (!rte_eth_dev_is_used(dev->state)) {
 		PMD_DRV_LOG(ERR, "eth_dev[%d] is unused.", port_id);
 		return NULL;
 	}
diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
index aa22b6a70d..2a4e565c4f 100644
--- a/drivers/net/ionic/ionic_ethdev.c
+++ b/drivers/net/ionic/ionic_ethdev.c
@@ -1109,7 +1109,7 @@ eth_ionic_dev_uninit(struct rte_eth_dev *eth_dev)
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
-	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
+	if (rte_eth_dev_is_used(eth_dev->state))
 		ionic_dev_close(eth_dev);
 
 	eth_dev->dev_ops = NULL;
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 6e4473e2f4..642e762868 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -3376,7 +3376,7 @@ mlx5_eth_find_next(uint16_t port_id, struct rte_device *odev)
 	while (port_id < RTE_MAX_ETHPORTS) {
 		struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 
-		if (dev->state != RTE_ETH_DEV_UNUSED &&
+		if (rte_eth_dev_is_used(dev->state) &&
 		    dev->device &&
 		    (dev->device == odev ||
 		     (dev->device->driver &&
diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index df5482f74a..dae4594e56 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -754,11 +754,11 @@ nfp_net_close(struct rte_eth_dev *dev)
 	/*
 	 * In secondary process, a released eth device can be found by its name
 	 * in shared memory.
-	 * If the state of the eth device is RTE_ETH_DEV_UNUSED, it means the
+	 * If the state of the eth device isn't used, it means the
 	 * eth device has been released.
 	 */
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
-		if (dev->state == RTE_ETH_DEV_UNUSED)
+		if (!rte_eth_dev_is_used(dev->state))
 			return 0;
 
 		nfp_pf_secondary_uninit(hw_priv);
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 9afef06431..5537c2f7af 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -55,8 +55,8 @@ eth_dev_find_free_port(void)
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		/* Using shared name field to find a free port. */
 		if (eth_dev_shared_data->data[i].name[0] == '\0') {
-			RTE_ASSERT(rte_eth_devices[i].state ==
-				   RTE_ETH_DEV_UNUSED);
+			RTE_ASSERT(!rte_eth_dev_is_used(
+					rte_eth_devices[i].state));
 			return i;
 		}
 	}
@@ -221,11 +221,18 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
 		eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
 
+	dev->state = RTE_ETH_DEV_ALLOCATED;
 	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
 
 	dev->state = RTE_ETH_DEV_ATTACHED;
 }
 
+bool rte_eth_dev_is_used(uint16_t dev_state)
+{
+	return dev_state == RTE_ETH_DEV_ALLOCATED ||
+		dev_state == RTE_ETH_DEV_ATTACHED;
+}
+
 int
 rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
@@ -243,7 +250,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (ret != 0)
 		return ret;
 
-	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
+	if (rte_eth_dev_is_used(eth_dev->state))
 		rte_eth_dev_callback_process(eth_dev,
 				RTE_ETH_EVENT_DESTROY, NULL);
 
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 1fd4562b40..dc496daf05 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1754,6 +1754,18 @@ int rte_eth_dev_callback_process(struct rte_eth_dev *dev,
 __rte_internal
 void rte_eth_dev_probing_finish(struct rte_eth_dev *dev);
 
+/**
+ * Check if a Ethernet device state is used or not
+ *
+ * @param dev_state
+ *   The state of the Ethernet device
+ * @return
+ *   - true if the state of the Ethernet device is allocated or attached
+ *   - false if this state is neither allocated nor attached
+ */
+__rte_internal
+bool rte_eth_dev_is_used(uint16_t dev_state);
+
 /**
  * Create memzone for HW rings.
  * malloc can't be used as the physical address is needed.
diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
index 2229ffa252..1e62f30d8d 100644
--- a/lib/ethdev/ethdev_pci.h
+++ b/lib/ethdev/ethdev_pci.h
@@ -179,7 +179,7 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
 	 * eth device has been released.
 	 */
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
-	    eth_dev->state == RTE_ETH_DEV_UNUSED)
+	    !rte_eth_dev_is_used(eth_dev->state))
 		return 0;
 
 	if (dev_uninit) {
diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
index a8d01e2595..f343c4b6eb 100644
--- a/lib/ethdev/rte_class_eth.c
+++ b/lib/ethdev/rte_class_eth.c
@@ -120,7 +120,7 @@ eth_dev_match(const struct rte_eth_dev *edev,
 	const struct rte_kvargs *kvlist = arg->kvlist;
 	unsigned int pair;
 
-	if (edev->state == RTE_ETH_DEV_UNUSED)
+	if (!rte_eth_dev_is_used(edev->state))
 		return -1;
 	if (arg->device != NULL && arg->device != edev->device)
 		return -1;
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6413c54e3b..3d7a3c39d3 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -349,7 +349,7 @@ uint16_t
 rte_eth_find_next(uint16_t port_id)
 {
 	while (port_id < RTE_MAX_ETHPORTS &&
-			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
+	       !rte_eth_dev_is_used(rte_eth_devices[port_id].state))
 		port_id++;
 
 	if (port_id >= RTE_MAX_ETHPORTS)
@@ -408,7 +408,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
 	int is_valid;
 
 	if (port_id >= RTE_MAX_ETHPORTS ||
-	    (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
+	    !rte_eth_dev_is_used(rte_eth_devices[port_id].state))
 		is_valid = 0;
 	else
 		is_valid = 1;
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 1f71cad244..f9a72b9883 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2091,10 +2091,12 @@ typedef uint16_t (*rte_tx_callback_fn)(uint16_t port_id, uint16_t queue,
 enum rte_eth_dev_state {
 	/** Device is unused before being probed. */
 	RTE_ETH_DEV_UNUSED = 0,
-	/** Device is attached when allocated in probing. */
+	/** Device is attached when definitely probed. */
 	RTE_ETH_DEV_ATTACHED,
 	/** Device is in removed state when plug-out is detected. */
 	RTE_ETH_DEV_REMOVED,
+	/** Device is allocated and is set before reporting new event. */
+	RTE_ETH_DEV_ALLOCATED,
 };
 
 struct rte_eth_dev_sriov {
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index 12f48c70a0..45b982e98d 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -351,6 +351,7 @@ INTERNAL {
 	rte_eth_dev_get_by_name;
 	rte_eth_dev_is_rx_hairpin_queue;
 	rte_eth_dev_is_tx_hairpin_queue;
+	rte_eth_dev_is_used;
 	rte_eth_dev_probing_finish;
 	rte_eth_dev_release_port;
 	rte_eth_dev_internal_reset;
-- 
2.22.0

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Thomas Monjalon]

13/01/2025 03:55, Huisong Li:
> The event callback in application may use the macro RTE_ETH_FOREACH_DEV to
> iterate over all enabled ports to do something(like, verifying the port id
> validity) when receive a probing event. If the ethdev state of a port is
> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
> 
> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing
> event. It means that probing callback will skip this port. But this
> assignment can not move to front of probing notification. See
> commit be8cd210379a ("ethdev: fix port probing notification")
> 
> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev
> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and set it to
> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid if its
> device state is 'ALLOCATED' or 'ATTACHED'.

If you do that, changing the definition of eth_dev_find_free_port()
you allow the application using a port before probing is finished.
It is the same as changing the state to RTE_ETH_DEV_ATTACHED
before calling the event callback.

So this is a NACK.

Why do you need drivers to check the state of a notified device?
If it is RTE_ETH_EVENT_NEW, you know that's a new device,
there is nothing else to check.

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[lihuisong (C)]

Hi Thomas,

在 2025/1/13 16:16, Thomas Monjalon 写道:
> 13/01/2025 03:55, Huisong Li:
>> The event callback in application may use the macro RTE_ETH_FOREACH_DEV to
>> iterate over all enabled ports to do something(like, verifying the port id
>> validity) when receive a probing event. If the ethdev state of a port is
>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>
>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing
>> event. It means that probing callback will skip this port. But this
>> assignment can not move to front of probing notification. See
>> commit be8cd210379a ("ethdev: fix port probing notification")
>>
>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev
>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and set it to
>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid if its
>> device state is 'ALLOCATED' or 'ATTACHED'.
> If you do that, changing the definition of eth_dev_find_free_port()
> you allow the application using a port before probing is finished.
Yes, it's not reasonable.

Thinking your comment twice, I feel that the root cause of this issue is 
application want to check if the port id is valid.
However, application just receive the new event from the device and the 
port id of this device must be valid when report new event.
So application can think the received new event is valid and don't need 
to check, right?
If so I think this series can be dropped.
> It is the same as changing the state to RTE_ETH_DEV_ATTACHED
> before calling the event callback.
>
> So this is a NACK.
>
> Why do you need drivers to check the state of a notified device?
> If it is RTE_ETH_EVENT_NEW, you know that's a new device,
> there is nothing else to check.
It just modified the verification about RTE_ETH_DEV_UNUSED in the device 
driver.
Driver not need to know the event.
>
>
> .

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Thomas Monjalon]

13/01/2025 10:35, lihuisong (C):
> 在 2025/1/13 16:16, Thomas Monjalon 写道:
> > 13/01/2025 03:55, Huisong Li:
> >> The event callback in application may use the macro RTE_ETH_FOREACH_DEV to
> >> iterate over all enabled ports to do something(like, verifying the port id
> >> validity) when receive a probing event. If the ethdev state of a port is
> >> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
> >>
> >> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing
> >> event. It means that probing callback will skip this port. But this
> >> assignment can not move to front of probing notification. See
> >> commit be8cd210379a ("ethdev: fix port probing notification")
> >>
> >> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev
> >> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and set it to
> >> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid if its
> >> device state is 'ALLOCATED' or 'ATTACHED'.
> > If you do that, changing the definition of eth_dev_find_free_port()
> > you allow the application using a port before probing is finished.
> 
> Yes, it's not reasonable.
> 
> Thinking your comment twice, I feel that the root cause of this issue is 
> application want to check if the port id is valid.
> However, application just receive the new event from the device and the 
> port id of this device must be valid when report new event.
> So application can think the received new event is valid and don't need 
> to check, right?

Yes
Do you think it should be highlighted in the API doc?

We currently have this:
	RTE_ETH_EVENT_NEW,      /**< port is probed */


> If so I think this series can be dropped.
> > It is the same as changing the state to RTE_ETH_DEV_ATTACHED
> > before calling the event callback.
> >
> > So this is a NACK.
> >
> > Why do you need drivers to check the state of a notified device?
> > If it is RTE_ETH_EVENT_NEW, you know that's a new device,
> > there is nothing else to check.
> 
> It just modified the verification about RTE_ETH_DEV_UNUSED in the device 
> driver.
> Driver not need to know the event.

Sorry I was not clear.
I said "drivers", but it should be "apps & drivers" because they can both
register to the event RTE_ETH_EVENT_NEW.
In some situations, it is convenient for a driver to listen to new ports
(it was done for failsafe driver).

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[lihuisong (C)]

在 2025/1/13 18:57, Thomas Monjalon 写道:
> 13/01/2025 10:35, lihuisong (C):
>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
>>> 13/01/2025 03:55, Huisong Li:
>>>> The event callback in application may use the macro RTE_ETH_FOREACH_DEV to
>>>> iterate over all enabled ports to do something(like, verifying the port id
>>>> validity) when receive a probing event. If the ethdev state of a port is
>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>>>
>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing
>>>> event. It means that probing callback will skip this port. But this
>>>> assignment can not move to front of probing notification. See
>>>> commit be8cd210379a ("ethdev: fix port probing notification")
>>>>
>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev
>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and set it to
>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid if its
>>>> device state is 'ALLOCATED' or 'ATTACHED'.
>>> If you do that, changing the definition of eth_dev_find_free_port()
>>> you allow the application using a port before probing is finished.
>> Yes, it's not reasonable.
>>
>> Thinking your comment twice, I feel that the root cause of this issue is
>> application want to check if the port id is valid.
>> However, application just receive the new event from the device and the
>> port id of this device must be valid when report new event.
>> So application can think the received new event is valid and don't need
>> to check, right?
> Yes
> Do you think it should be highlighted in the API doc?
Security detection is common and always good for application.
So I think it's better to highlight that in doc.

If it's ok to you, I will add some comments for RTE_ETH_EVENT_NEW.
>
> We currently have this:
> 	RTE_ETH_EVENT_NEW,      /**< port is probed */
>
>
>> If so I think this series can be dropped.
>>> It is the same as changing the state to RTE_ETH_DEV_ATTACHED
>>> before calling the event callback.
>>>
>>> So this is a NACK.
>>>
>>> Why do you need drivers to check the state of a notified device?
>>> If it is RTE_ETH_EVENT_NEW, you know that's a new device,
>>> there is nothing else to check.
>> It just modified the verification about RTE_ETH_DEV_UNUSED in the device
>> driver.
>> Driver not need to know the event.
> Sorry I was not clear.
> I said "drivers", but it should be "apps & drivers" because they can both
> register to the event RTE_ETH_EVENT_NEW.
> In some situations, it is convenient for a driver to listen to new ports
> (it was done for failsafe driver).
Yes. but it doesn't matter now.😁
>
>
> .

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[lihuisong (C)]

Hi Thomas,

在 2025/1/13 19:23, lihuisong (C) 写道:
>
> 在 2025/1/13 18:57, Thomas Monjalon 写道:
>> 13/01/2025 10:35, lihuisong (C):
>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
>>>> 13/01/2025 03:55, Huisong Li:
>>>>> The event callback in application may use the macro 
>>>>> RTE_ETH_FOREACH_DEV to
>>>>> iterate over all enabled ports to do something(like, verifying the 
>>>>> port id
>>>>> validity) when receive a probing event. If the ethdev state of a 
>>>>> port is
>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>>>>
>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing 
>>>>> probing
>>>>> event. It means that probing callback will skip this port. But this
>>>>> assignment can not move to front of probing notification. See
>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
>>>>>
>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set 
>>>>> the ethdev
>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and 
>>>>> set it to
>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is 
>>>>> valid if its
>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
>>>> If you do that, changing the definition of eth_dev_find_free_port()
>>>> you allow the application using a port before probing is finished.
>>> Yes, it's not reasonable.
>>>
>>> Thinking your comment twice, I feel that the root cause of this 
>>> issue is
>>> application want to check if the port id is valid.
>>> However, application just receive the new event from the device and the
>>> port id of this device must be valid when report new event.
>>> So application can think the received new event is valid and don't need
>>> to check, right?
>> Yes
>> Do you think it should be highlighted in the API doc?
> Security detection is common and always good for application.
> So I think it's better to highlight that in doc.
>
Now I remember why I have to put this patch into the patchset [1] that 
testpmd support multiple process attach and detach port.
Becase patch 4/5 in this series depands on this patch.
The setup_attached_port() have to move to eth_event_callback() in 
testpmd to update something.
And the setup_attached_port() would indirectyly check if this port is 
valid by rte_eth_dev_is_valid_port().
Their caller stack is as follows:
eth_event_callback
     -->setup_attached_port
         -->rte_eth_dev_socket_id
             -->rte_eth_dev_is_valid_port

 From the testpmd's modification, that is to say, it is possible for 
appllication to call some APIs like rte_eth_dev_socket_id() and 
indirectyly check if this port id is valid in event new callback.
So should we add this patch? I think there are many like these API in 
ethdev layer. I'm confused a bit now.


[1] https://mails.dpdk.org/archives/dev/2024-January/286026.html

/Huisong

>>
>> We currently have this:
>>     RTE_ETH_EVENT_NEW,      /**< port is probed */
>>
>>
>>> If so I think this series can be dropped.
>>>> It is the same as changing the state to RTE_ETH_DEV_ATTACHED
>>>> before calling the event callback.
>>>>
>>>> So this is a NACK.
>>>>
>>>> Why do you need drivers to check the state of a notified device?
>>>> If it is RTE_ETH_EVENT_NEW, you know that's a new device,
>>>> there is nothing else to check.
>>> It just modified the verification about RTE_ETH_DEV_UNUSED in the 
>>> device
>>> driver.
>>> Driver not need to know the event.
>> Sorry I was not clear.
>> I said "drivers", but it should be "apps & drivers" because they can 
>> both
>> register to the event RTE_ETH_EVENT_NEW.
>> In some situations, it is convenient for a driver to listen to new ports
>> (it was done for failsafe driver).
> Yes. but it doesn't matter now.😁
>>
>>
>> .

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Thomas Monjalon]

13/01/2025 13:05, lihuisong (C):
> 在 2025/1/13 19:23, lihuisong (C) 写道:
> > 在 2025/1/13 18:57, Thomas Monjalon 写道:
> >> 13/01/2025 10:35, lihuisong (C):
> >>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
> >>>> 13/01/2025 03:55, Huisong Li:
> >>>>> The event callback in application may use the macro 
> >>>>> RTE_ETH_FOREACH_DEV to
> >>>>> iterate over all enabled ports to do something(like, verifying the 
> >>>>> port id
> >>>>> validity) when receive a probing event. If the ethdev state of a 
> >>>>> port is
> >>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
> >>>>>
> >>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing 
> >>>>> probing
> >>>>> event. It means that probing callback will skip this port. But this
> >>>>> assignment can not move to front of probing notification. See
> >>>>> commit be8cd210379a ("ethdev: fix port probing notification")
> >>>>>
> >>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set 
> >>>>> the ethdev
> >>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and 
> >>>>> set it to
> >>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is 
> >>>>> valid if its
> >>>>> device state is 'ALLOCATED' or 'ATTACHED'.
> >>>> 
> >>>> If you do that, changing the definition of eth_dev_find_free_port()
> >>>> you allow the application using a port before probing is finished.
> >>> Yes, it's not reasonable.
> >>>
> >>> Thinking your comment twice, I feel that the root cause of this 
> >>> issue is
> >>> application want to check if the port id is valid.
> >>> However, application just receive the new event from the device and the
> >>> port id of this device must be valid when report new event.
> >>> So application can think the received new event is valid and don't need
> >>> to check, right?
> >> 
> >> Yes
> >> Do you think it should be highlighted in the API doc?
> > Security detection is common and always good for application.
> > So I think it's better to highlight that in doc.
> >
> Now I remember why I have to put this patch into the patchset [1] that 
> testpmd support multiple process attach and detach port.
> Becase patch 4/5 in this series depands on this patch.
> The setup_attached_port() have to move to eth_event_callback() in 
> testpmd to update something.
> And the setup_attached_port() would indirectyly check if this port is 
> valid by rte_eth_dev_is_valid_port().
> Their caller stack is as follows:
> eth_event_callback
>      -->setup_attached_port
>          -->rte_eth_dev_socket_id
>              -->rte_eth_dev_is_valid_port
> 
>  From the testpmd's modification, that is to say, it is possible for 
> appllication to call some APIs like rte_eth_dev_socket_id() and 
> indirectyly check if this port id is valid in event new callback.
> So should we add this patch? I think there are many like these API in 
> ethdev layer. I'm confused a bit now.

Yes rte_eth_dev_is_valid_port() is used in many API functions,
so that's a valid concern.
I would say we should not call much of these functions in the "new port"
event callback.
But the case of rte_eth_dev_socket_id() is concerning.

I suggest to update rte_eth_dev_socket_id() to make it work with
a newly allocated port.
I suppose we can use the function eth_dev_is_allocated().

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[lihuisong (C)]

在 2025/1/13 20:30, Thomas Monjalon 写道:
> 13/01/2025 13:05, lihuisong (C):
>> 在 2025/1/13 19:23, lihuisong (C) 写道:
>>> 在 2025/1/13 18:57, Thomas Monjalon 写道:
>>>> 13/01/2025 10:35, lihuisong (C):
>>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
>>>>>> 13/01/2025 03:55, Huisong Li:
>>>>>>> The event callback in application may use the macro
>>>>>>> RTE_ETH_FOREACH_DEV to
>>>>>>> iterate over all enabled ports to do something(like, verifying the
>>>>>>> port id
>>>>>>> validity) when receive a probing event. If the ethdev state of a
>>>>>>> port is
>>>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>>>>>>
>>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing
>>>>>>> probing
>>>>>>> event. It means that probing callback will skip this port. But this
>>>>>>> assignment can not move to front of probing notification. See
>>>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
>>>>>>>
>>>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set
>>>>>>> the ethdev
>>>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and
>>>>>>> set it to
>>>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is
>>>>>>> valid if its
>>>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
>>>>>> If you do that, changing the definition of eth_dev_find_free_port()
>>>>>> you allow the application using a port before probing is finished.
>>>>> Yes, it's not reasonable.
>>>>>
>>>>> Thinking your comment twice, I feel that the root cause of this
>>>>> issue is
>>>>> application want to check if the port id is valid.
>>>>> However, application just receive the new event from the device and the
>>>>> port id of this device must be valid when report new event.
>>>>> So application can think the received new event is valid and don't need
>>>>> to check, right?
>>>> Yes
>>>> Do you think it should be highlighted in the API doc?
>>> Security detection is common and always good for application.
>>> So I think it's better to highlight that in doc.
>>>
>> Now I remember why I have to put this patch into the patchset [1] that
>> testpmd support multiple process attach and detach port.
>> Becase patch 4/5 in this series depands on this patch.
>> The setup_attached_port() have to move to eth_event_callback() in
>> testpmd to update something.
>> And the setup_attached_port() would indirectyly check if this port is
>> valid by rte_eth_dev_is_valid_port().
>> Their caller stack is as follows:
>> eth_event_callback
>>       -->setup_attached_port
>>           -->rte_eth_dev_socket_id
>>               -->rte_eth_dev_is_valid_port
>>
>>   From the testpmd's modification, that is to say, it is possible for
>> appllication to call some APIs like rte_eth_dev_socket_id() and
>> indirectyly check if this port id is valid in event new callback.
>> So should we add this patch? I think there are many like these API in
>> ethdev layer. I'm confused a bit now.
> Yes rte_eth_dev_is_valid_port() is used in many API functions,
> so that's a valid concern.
> I would say we should not call much of these functions in the "new port"
> event callback.
> But the case of rte_eth_dev_socket_id() is concerning.
>
> I suggest to update rte_eth_dev_socket_id() to make it work with
> a newly allocated port.
> I suppose we can use the function eth_dev_is_allocated().
What you mean is doing it like the following code?
-->

--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -635,8 +635,10 @@ 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)) {
+       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;


>
>
> .

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Thomas Monjalon]

13/01/2025 13:47, lihuisong (C):
> 在 2025/1/13 20:30, Thomas Monjalon 写道:
> > 13/01/2025 13:05, lihuisong (C):
> >> 在 2025/1/13 19:23, lihuisong (C) 写道:
> >>> 在 2025/1/13 18:57, Thomas Monjalon 写道:
> >>>> 13/01/2025 10:35, lihuisong (C):
> >>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
> >>>>>> 13/01/2025 03:55, Huisong Li:
> >>>>>>> The event callback in application may use the macro
> >>>>>>> RTE_ETH_FOREACH_DEV to
> >>>>>>> iterate over all enabled ports to do something(like, verifying the
> >>>>>>> port id
> >>>>>>> validity) when receive a probing event. If the ethdev state of a
> >>>>>>> port is
> >>>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
> >>>>>>>
> >>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing
> >>>>>>> probing
> >>>>>>> event. It means that probing callback will skip this port. But this
> >>>>>>> assignment can not move to front of probing notification. See
> >>>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
> >>>>>>>
> >>>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set
> >>>>>>> the ethdev
> >>>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and
> >>>>>>> set it to
> >>>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is
> >>>>>>> valid if its
> >>>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
> >>>>>> If you do that, changing the definition of eth_dev_find_free_port()
> >>>>>> you allow the application using a port before probing is finished.
> >>>>> Yes, it's not reasonable.
> >>>>>
> >>>>> Thinking your comment twice, I feel that the root cause of this
> >>>>> issue is
> >>>>> application want to check if the port id is valid.
> >>>>> However, application just receive the new event from the device and the
> >>>>> port id of this device must be valid when report new event.
> >>>>> So application can think the received new event is valid and don't need
> >>>>> to check, right?
> >>>> Yes
> >>>> Do you think it should be highlighted in the API doc?
> >>> Security detection is common and always good for application.
> >>> So I think it's better to highlight that in doc.
> >>>
> >> Now I remember why I have to put this patch into the patchset [1] that
> >> testpmd support multiple process attach and detach port.
> >> Becase patch 4/5 in this series depands on this patch.
> >> The setup_attached_port() have to move to eth_event_callback() in
> >> testpmd to update something.
> >> And the setup_attached_port() would indirectyly check if this port is
> >> valid by rte_eth_dev_is_valid_port().
> >> Their caller stack is as follows:
> >> eth_event_callback
> >>       -->setup_attached_port
> >>           -->rte_eth_dev_socket_id
> >>               -->rte_eth_dev_is_valid_port
> >>
> >>   From the testpmd's modification, that is to say, it is possible for
> >> appllication to call some APIs like rte_eth_dev_socket_id() and
> >> indirectyly check if this port id is valid in event new callback.
> >> So should we add this patch? I think there are many like these API in
> >> ethdev layer. I'm confused a bit now.
> > Yes rte_eth_dev_is_valid_port() is used in many API functions,
> > so that's a valid concern.
> > I would say we should not call much of these functions in the "new port"
> > event callback.
> > But the case of rte_eth_dev_socket_id() is concerning.
> >
> > I suggest to update rte_eth_dev_socket_id() to make it work with
> > a newly allocated port.
> > I suppose we can use the function eth_dev_is_allocated().
> What you mean is doing it like the following code?
> -->
> 
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -635,8 +635,10 @@ 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)) {
> +       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;


Yes. Would it work?

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[lihuisong (C)]

在 2025/1/13 21:14, Thomas Monjalon 写道:
> 13/01/2025 13:47, lihuisong (C):
>> 在 2025/1/13 20:30, Thomas Monjalon 写道:
>>> 13/01/2025 13:05, lihuisong (C):
>>>> 在 2025/1/13 19:23, lihuisong (C) 写道:
>>>>> 在 2025/1/13 18:57, Thomas Monjalon 写道:
>>>>>> 13/01/2025 10:35, lihuisong (C):
>>>>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
>>>>>>>> 13/01/2025 03:55, Huisong Li:
>>>>>>>>> The event callback in application may use the macro
>>>>>>>>> RTE_ETH_FOREACH_DEV to
>>>>>>>>> iterate over all enabled ports to do something(like, verifying the
>>>>>>>>> port id
>>>>>>>>> validity) when receive a probing event. If the ethdev state of a
>>>>>>>>> port is
>>>>>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>>>>>>>>
>>>>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing
>>>>>>>>> probing
>>>>>>>>> event. It means that probing callback will skip this port. But this
>>>>>>>>> assignment can not move to front of probing notification. See
>>>>>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
>>>>>>>>>
>>>>>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set
>>>>>>>>> the ethdev
>>>>>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and
>>>>>>>>> set it to
>>>>>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is
>>>>>>>>> valid if its
>>>>>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
>>>>>>>> If you do that, changing the definition of eth_dev_find_free_port()
>>>>>>>> you allow the application using a port before probing is finished.
>>>>>>> Yes, it's not reasonable.
>>>>>>>
>>>>>>> Thinking your comment twice, I feel that the root cause of this
>>>>>>> issue is
>>>>>>> application want to check if the port id is valid.
>>>>>>> However, application just receive the new event from the device and the
>>>>>>> port id of this device must be valid when report new event.
>>>>>>> So application can think the received new event is valid and don't need
>>>>>>> to check, right?
>>>>>> Yes
>>>>>> Do you think it should be highlighted in the API doc?
>>>>> Security detection is common and always good for application.
>>>>> So I think it's better to highlight that in doc.
>>>>>
>>>> Now I remember why I have to put this patch into the patchset [1] that
>>>> testpmd support multiple process attach and detach port.
>>>> Becase patch 4/5 in this series depands on this patch.
>>>> The setup_attached_port() have to move to eth_event_callback() in
>>>> testpmd to update something.
>>>> And the setup_attached_port() would indirectyly check if this port is
>>>> valid by rte_eth_dev_is_valid_port().
>>>> Their caller stack is as follows:
>>>> eth_event_callback
>>>>        -->setup_attached_port
>>>>            -->rte_eth_dev_socket_id
>>>>                -->rte_eth_dev_is_valid_port
>>>>
>>>>    From the testpmd's modification, that is to say, it is possible for
>>>> appllication to call some APIs like rte_eth_dev_socket_id() and
>>>> indirectyly check if this port id is valid in event new callback.
>>>> So should we add this patch? I think there are many like these API in
>>>> ethdev layer. I'm confused a bit now.
>>> Yes rte_eth_dev_is_valid_port() is used in many API functions,
>>> so that's a valid concern.
>>> I would say we should not call much of these functions in the "new port"
>>> event callback.
>>> But the case of rte_eth_dev_socket_id() is concerning.
>>>
>>> I suggest to update rte_eth_dev_socket_id() to make it work with
>>> a newly allocated port.
>>> I suppose we can use the function eth_dev_is_allocated().
>> What you mean is doing it like the following code?
>> -->
>>
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -635,8 +635,10 @@ 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)) {
>> +       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;
>
> Yes. Would it work?
I think it can work for this API.

 From the disscussion for this patch, we've come to an aggreement that 
application can think port is valid in new event.
Now that the port id is valid, the new event callback of application may 
call other API, for example, rte_eth_dev_info_get().
(Apllication may call rte_eth_dev_info_get to get someting in new event 
callback)
Note: patch 4/5 modified in the series[1] also used this API.
-->
eth_event_callback
     -->setup_attached_port
         -->reconfig
             -->init_config_port_offloads
                 -->eth_dev_info_get_print_err
---

There is RTE_ETH_VALID_PORTID_OR_ERR_RET to check port_id is valid in 
rte_eth_dev_info_get.
Application also happen to this issue like rte_eth_dev_socket_id, right?
This macro is also widely used in ethdev layer. We probability need to 
filter out all these interfaces which can be used in new event callback.
And then handle the check for port_id in these interfaces like 
rte_eth_dev_socket_id.
What do you think?  Are there any other similar interfaces in ethdev layer?


>
>
>
>
> .

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Thomas Monjalon]

14/01/2025 02:50, lihuisong (C):
> 在 2025/1/13 21:14, Thomas Monjalon 写道:
> > 13/01/2025 13:47, lihuisong (C):
> >> 在 2025/1/13 20:30, Thomas Monjalon 写道:
> >>> 13/01/2025 13:05, lihuisong (C):
> >>>> 在 2025/1/13 19:23, lihuisong (C) 写道:
> >>>>> 在 2025/1/13 18:57, Thomas Monjalon 写道:
> >>>>>> 13/01/2025 10:35, lihuisong (C):
> >>>>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
> >>>>>>>> 13/01/2025 03:55, Huisong Li:
> >>>>>>>>> The event callback in application may use the macro
> >>>>>>>>> RTE_ETH_FOREACH_DEV to
> >>>>>>>>> iterate over all enabled ports to do something(like, verifying the
> >>>>>>>>> port id
> >>>>>>>>> validity) when receive a probing event. If the ethdev state of a
> >>>>>>>>> port is
> >>>>>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
> >>>>>>>>>
> >>>>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing
> >>>>>>>>> probing
> >>>>>>>>> event. It means that probing callback will skip this port. But this
> >>>>>>>>> assignment can not move to front of probing notification. See
> >>>>>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
> >>>>>>>>>
> >>>>>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set
> >>>>>>>>> the ethdev
> >>>>>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and
> >>>>>>>>> set it to
> >>>>>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is
> >>>>>>>>> valid if its
> >>>>>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
> >>>>>>>> If you do that, changing the definition of eth_dev_find_free_port()
> >>>>>>>> you allow the application using a port before probing is finished.
> >>>>>>> Yes, it's not reasonable.
> >>>>>>>
> >>>>>>> Thinking your comment twice, I feel that the root cause of this
> >>>>>>> issue is
> >>>>>>> application want to check if the port id is valid.
> >>>>>>> However, application just receive the new event from the device and the
> >>>>>>> port id of this device must be valid when report new event.
> >>>>>>> So application can think the received new event is valid and don't need
> >>>>>>> to check, right?
> >>>>>> Yes
> >>>>>> Do you think it should be highlighted in the API doc?
> >>>>> Security detection is common and always good for application.
> >>>>> So I think it's better to highlight that in doc.
> >>>>>
> >>>> Now I remember why I have to put this patch into the patchset [1] that
> >>>> testpmd support multiple process attach and detach port.
> >>>> Becase patch 4/5 in this series depands on this patch.
> >>>> The setup_attached_port() have to move to eth_event_callback() in
> >>>> testpmd to update something.
> >>>> And the setup_attached_port() would indirectyly check if this port is
> >>>> valid by rte_eth_dev_is_valid_port().
> >>>> Their caller stack is as follows:
> >>>> eth_event_callback
> >>>>        -->setup_attached_port
> >>>>            -->rte_eth_dev_socket_id
> >>>>                -->rte_eth_dev_is_valid_port
> >>>>
> >>>>    From the testpmd's modification, that is to say, it is possible for
> >>>> appllication to call some APIs like rte_eth_dev_socket_id() and
> >>>> indirectyly check if this port id is valid in event new callback.
> >>>> So should we add this patch? I think there are many like these API in
> >>>> ethdev layer. I'm confused a bit now.
> >>> Yes rte_eth_dev_is_valid_port() is used in many API functions,
> >>> so that's a valid concern.
> >>> I would say we should not call much of these functions in the "new port"
> >>> event callback.
> >>> But the case of rte_eth_dev_socket_id() is concerning.
> >>>
> >>> I suggest to update rte_eth_dev_socket_id() to make it work with
> >>> a newly allocated port.
> >>> I suppose we can use the function eth_dev_is_allocated().
> >> What you mean is doing it like the following code?
> >> -->
> >>
> >> --- a/lib/ethdev/rte_ethdev.c
> >> +++ b/lib/ethdev/rte_ethdev.c
> >> @@ -635,8 +635,10 @@ 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)) {
> >> +       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;
> >
> > Yes. Would it work?
> I think it can work for this API.
> 
>  From the disscussion for this patch, we've come to an aggreement that 
> application can think port is valid in new event.

We don't want an application to configure a port before probing is finished
(like still in the event processing).

> Now that the port id is valid, the new event callback of application may 
> call other API, for example, rte_eth_dev_info_get().
> (Apllication may call rte_eth_dev_info_get to get someting in new event 
> callback)
> Note: patch 4/5 modified in the series[1] also used this API.
> -->
> eth_event_callback
>      -->setup_attached_port
>          -->reconfig
>              -->init_config_port_offloads
>                  -->eth_dev_info_get_print_err
> ---

I don't agree with configuring a port which is not fully probed.

> There is RTE_ETH_VALID_PORTID_OR_ERR_RET to check port_id is valid in 
> rte_eth_dev_info_get.
> Application also happen to this issue like rte_eth_dev_socket_id, right?

Right, I think such application is abusing the new event.

testpmd set a flag when receiving an event, it should not do more:

    case RTE_ETH_EVENT_NEW:                                                                          
        ports[port_id].need_setup = 1;
        ports[port_id].port_status = RTE_PORT_HANDLING;
        break;      

> This macro is also widely used in ethdev layer. We probability need to 
> filter out all these interfaces which can be used in new event callback.
> And then handle the check for port_id in these interfaces like 
> rte_eth_dev_socket_id.
> What do you think?  Are there any other similar interfaces in ethdev layer?

As explained above, we should not do allow much API from RTE_ETH_EVENT_NEW.
rte_eth_dev_socket_id() is reasonnable.
Functions rte_eth_dev_owner_*() are fine.
Others functions should be called only after probing.

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[lihuisong (C)]

在 2025/1/14 19:13, Thomas Monjalon 写道:
> 14/01/2025 02:50, lihuisong (C):
>> 在 2025/1/13 21:14, Thomas Monjalon 写道:
>>> 13/01/2025 13:47, lihuisong (C):
>>>> 在 2025/1/13 20:30, Thomas Monjalon 写道:
>>>>> 13/01/2025 13:05, lihuisong (C):
>>>>>> 在 2025/1/13 19:23, lihuisong (C) 写道:
>>>>>>> 在 2025/1/13 18:57, Thomas Monjalon 写道:
>>>>>>>> 13/01/2025 10:35, lihuisong (C):
>>>>>>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
>>>>>>>>>> 13/01/2025 03:55, Huisong Li:
>>>>>>>>>>> The event callback in application may use the macro
>>>>>>>>>>> RTE_ETH_FOREACH_DEV to
>>>>>>>>>>> iterate over all enabled ports to do something(like, verifying the
>>>>>>>>>>> port id
>>>>>>>>>>> validity) when receive a probing event. If the ethdev state of a
>>>>>>>>>>> port is
>>>>>>>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>>>>>>>>>>
>>>>>>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing
>>>>>>>>>>> probing
>>>>>>>>>>> event. It means that probing callback will skip this port. But this
>>>>>>>>>>> assignment can not move to front of probing notification. See
>>>>>>>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
>>>>>>>>>>>
>>>>>>>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set
>>>>>>>>>>> the ethdev
>>>>>>>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and
>>>>>>>>>>> set it to
>>>>>>>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is
>>>>>>>>>>> valid if its
>>>>>>>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
>>>>>>>>>> If you do that, changing the definition of eth_dev_find_free_port()
>>>>>>>>>> you allow the application using a port before probing is finished.
>>>>>>>>> Yes, it's not reasonable.
>>>>>>>>>
>>>>>>>>> Thinking your comment twice, I feel that the root cause of this
>>>>>>>>> issue is
>>>>>>>>> application want to check if the port id is valid.
>>>>>>>>> However, application just receive the new event from the device and the
>>>>>>>>> port id of this device must be valid when report new event.
>>>>>>>>> So application can think the received new event is valid and don't need
>>>>>>>>> to check, right?
>>>>>>>> Yes
>>>>>>>> Do you think it should be highlighted in the API doc?
>>>>>>> Security detection is common and always good for application.
>>>>>>> So I think it's better to highlight that in doc.
>>>>>>>
>>>>>> Now I remember why I have to put this patch into the patchset [1] that
>>>>>> testpmd support multiple process attach and detach port.
>>>>>> Becase patch 4/5 in this series depands on this patch.
>>>>>> The setup_attached_port() have to move to eth_event_callback() in
>>>>>> testpmd to update something.
>>>>>> And the setup_attached_port() would indirectyly check if this port is
>>>>>> valid by rte_eth_dev_is_valid_port().
>>>>>> Their caller stack is as follows:
>>>>>> eth_event_callback
>>>>>>         -->setup_attached_port
>>>>>>             -->rte_eth_dev_socket_id
>>>>>>                 -->rte_eth_dev_is_valid_port
>>>>>>
>>>>>>     From the testpmd's modification, that is to say, it is possible for
>>>>>> appllication to call some APIs like rte_eth_dev_socket_id() and
>>>>>> indirectyly check if this port id is valid in event new callback.
>>>>>> So should we add this patch? I think there are many like these API in
>>>>>> ethdev layer. I'm confused a bit now.
>>>>> Yes rte_eth_dev_is_valid_port() is used in many API functions,
>>>>> so that's a valid concern.
>>>>> I would say we should not call much of these functions in the "new port"
>>>>> event callback.
>>>>> But the case of rte_eth_dev_socket_id() is concerning.
>>>>>
>>>>> I suggest to update rte_eth_dev_socket_id() to make it work with
>>>>> a newly allocated port.
>>>>> I suppose we can use the function eth_dev_is_allocated().
>>>> What you mean is doing it like the following code?
>>>> -->
>>>>
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -635,8 +635,10 @@ 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)) {
>>>> +       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;
>>> Yes. Would it work?
>> I think it can work for this API.
>>
>>   From the disscussion for this patch, we've come to an aggreement that
>> application can think port is valid in new event.
> We don't want an application to configure a port before probing is finished
> (like still in the event processing).
Ok
>
>> Now that the port id is valid, the new event callback of application may
>> call other API, for example, rte_eth_dev_info_get().
>> (Apllication may call rte_eth_dev_info_get to get someting in new event
>> callback)
>> Note: patch 4/5 modified in the series[1] also used this API.
>> -->
>> eth_event_callback
>>       -->setup_attached_port
>>           -->reconfig
>>               -->init_config_port_offloads
>>                   -->eth_dev_info_get_print_err
>> ---
> I don't agree with configuring a port which is not fully probed.
Got it.
>
>> There is RTE_ETH_VALID_PORTID_OR_ERR_RET to check port_id is valid in
>> rte_eth_dev_info_get.
>> Application also happen to this issue like rte_eth_dev_socket_id, right?
> Right, I think such application is abusing the new event.
>
> testpmd set a flag when receiving an event, it should not do more:
>
>      case RTE_ETH_EVENT_NEW:
>          ports[port_id].need_setup = 1;
>          ports[port_id].port_status = RTE_PORT_HANDLING;
>          break;
ok I know what you mean.
>> This macro is also widely used in ethdev layer. We probability need to
>> filter out all these interfaces which can be used in new event callback.
>> And then handle the check for port_id in these interfaces like
>> rte_eth_dev_socket_id.
>> What do you think?  Are there any other similar interfaces in ethdev layer?
> As explained above, we should not do allow much API from RTE_ETH_EVENT_NEW.
> rte_eth_dev_socket_id() is reasonnable.
> Functions rte_eth_dev_owner_*() are fine.
> Others functions should be called only after probing.
All right, will fix it in new patch set.
And I'll also add these comments like above you said for RTE_ETH_EVENT_NEW.
>
>
>
> .

Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

[Thomas Monjalon]

14/01/2025 13:13, lihuisong (C):
> 在 2025/1/14 19:13, Thomas Monjalon 写道:
> > As explained above, we should not do allow much API from RTE_ETH_EVENT_NEW.
> > rte_eth_dev_socket_id() is reasonnable.
> > Functions rte_eth_dev_owner_*() are fine.
> > Others functions should be called only after probing.
> 
> All right, will fix it in new patch set.
> And I'll also add these comments like above you said for RTE_ETH_EVENT_NEW.

Thank you