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

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

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