RE: [PATCH v7 2/4] lib: fix comparison between devices

[Shani Peretz <shperetz@nvidia.com>]

Hey Stephan thanks for the quick reply, 
I checked the code and also tested and it seems that it won't work because rte_devargs_parse() saves the user provided name in devargs.name without changing it to canonical form.
so in the end find_device still receives the short name.
We could save it in devargs.name but then we will still need another function that converts the short name to its canonical form.

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, 30 April 2025 19:00
> To: Shani Peretz <shperetz@nvidia.com>
> Cc: dev@dpdk.org; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav Pandit
> <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> Richardson <bruce.richardson@intel.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> Subject: Re: [PATCH v7 2/4] lib: fix comparison between devices
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 30 Apr 2025 11:12:51 +0000
> Shani Peretz <shperetz@nvidia.com> wrote:
> 
> > Hey Stephan,
> > Apologize for the delayed response and appreciate your assistance.
> > I prepared a document outlining the bug and its flow, along with the two
> solutions we discussed.
> > I hope this document provides a comprehensive overview. Can you take a
> look?
> >
> > Thank you!
> >
> >
> https://docs.google.com/document/d/1LmMlJ31P1G77K0TGkBfXWz0DEj6zdprP
> 0b
> > G5p_wu77w/edit?usp=sharing
> >
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Monday, 17 March 2025 16:11
> > > To: Shani Peretz <shperetz@nvidia.com>
> > > Cc: dev@dpdk.org; Tyler Retzlaff <roretzla@linux.microsoft.com>;
> > > Parav Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>;
> > > Nipun Gupta <nipun.gupta@amd.com>; Nikhil Agarwal
> > > <nikhil.agarwal@amd.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> > > Sachin Saxena <sachin.saxena@nxp.com>; Rosen Xu
> > > <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>; Tomasz
> > > Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > Subject: Re: [PATCH v7 2/4] lib: fix comparison between devices
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Thu, 6 Mar 2025 16:26:50 +0000
> > > Shani Peretz <shperetz@nvidia.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > > Sent: Thursday, 20 February 2025 20:33
> > > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > > Cc: dev@dpdk.org; Tyler Retzlaff <roretzla@linux.microsoft.com>;
> > > > > Parav Pandit <parav@nvidia.com>; Xueming Li
> > > > > <xuemingl@nvidia.com>; Nipun Gupta <nipun.gupta@amd.com>; Nikhil
> > > > > Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>;
> > > > > Bruce Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > > Subject: Re: [PATCH v7 2/4] lib: fix comparison between devices
> > > > >
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > On Wed, 12 Feb 2025 18:38:33 +0200 Shani Peretz
> > > > > <shperetz@nvidia.com> wrote:
> > > > >
> > > > > > DPDK supports multiple formats for specifying buses, (such as
> > > > > > "0000:08:00.0" and "08:00.0" for PCI).
> > > > > > This flexibility can lead to inconsistencies when using one
> > > > > > format while running testpmd, then attempts to use the other
> > > > > > format in a later command, resulting in a failure.
> > > > > >
> > > > > > The issue arises from the find_device function, which compares
> > > > > > the user-provided string directly with the device->name in the
> > > > > > rte_device structure.
> > > > > > If we want to accurately compare these names, we'll need to
> > > > > > bring both sides to the same representation by invoking the
> > > > > > parse function on the user input.
> > > > >
> > > > > Could you give an example where this happens please?
> > > > > Shouldn't find_device string always be changed into canonical
> > > > > form in find_device handler?
> > > >
> > > > The flow I was dealing with was attach_port -> rte_dev_probe - >
> > > local_dev_probe -> find_device.
> > > > The string passed to attach_port was the short version, directly from the
> user.
> > > >
> > > > So, to clarify - you're saying that find_device simply need to
> > > > accept the string
> > > in its canonical form? Which means we'll only need to fix
> > > local_dev_probe to bring it to the canonical form before calling find_device?
> > > > I tried it but then I noticed that there's no function that gets
> > > > the user-
> > > provided string and returns it's string canonical form. The closest
> > > to this is parse, but what it eventually returns is not necessarily
> > > a string - it can be anything - for instance pci_parse will give you back a
> struct rte_pci_addr.
> > >
> > > Not sure at this point. There are two options. One would be fixup in
> > > attach_port the other would be allowing short form in PCI part of
> > > find_device. Since the strings from command line are put in
> > > canonical form for devargs, it seems logical to do it in attach_port path.
> 
> What about if testpmd just did it first?
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> b5f0c02261..a324225e26 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3413,17 +3413,18 @@ void
>  attach_port(char *identifier)
>  {
>         portid_t pi;
> +       struct rte_devargs devargs = { 0 };
>         struct rte_dev_iterator iterator;
> 
>         printf("Attaching a new port...\n");
> 
> -       if (identifier == NULL) {
> +       if (rte_devargs_parse(&devargs, identifier) != 0) {
>                 fprintf(stderr, "Invalid parameters are specified\n");
>                 return;
>         }
> 
> -       if (rte_dev_probe(identifier) < 0) {
> -               TESTPMD_LOG(ERR, "Failed to attach port %s\n", identifier);
> +       if (rte_dev_probe(devargs.name) < 0) {
> +               TESTPMD_LOG(ERR, "Failed to attach port %s\n",
> + devargs.name);
>                 return;
>         }
> 
> @@ -3435,14 +3436,14 @@ attach_port(char *identifier)
>         }
> 
>         /* second attach mode: iterator */
> -       RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
> +       RTE_ETH_FOREACH_MATCHING_DEV(pi, devargs.name, &iterator) {
>                 /* setup ports matching the devargs used for probing */
>                 if (port_is_forwarding(pi))
>                         continue; /* port was already attached before */
>                 setup_attached_port((void *)(intptr_t)pi);
>         }
>  out:
> -       printf("Port %s is attached.\n", identifier);
> +       printf("Port %s is attached.\n", devargs.name);
>         printf("Done\n");
>  }
>