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

[Thomas Monjalon <thomas@monjalon.net>]

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().