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.😁
.
