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

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

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