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

["lihuisong (C)" <lihuisong@huawei.com>]

在 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;


>
>
> .