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

[Thomas Monjalon <thomas@monjalon.net>]

13/01/2025 13:47, lihuisong (C):
> 在 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;


Yes. Would it work?