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

[Stephen Hemminger]

On Wed, 30 Apr 2025 11:12:51 +0000
Shani Peretz <shperetz at 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/1LmMlJ31P1G77K0TGkBfXWz0DEj6zdprP0bG5p_wu77w/edit?usp=sharing
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen at networkplumber.org>
> > Sent: Monday, 17 March 2025 16:11
> > To: Shani Peretz <shperetz at nvidia.com>
> > Cc: dev at dpdk.org; Tyler Retzlaff <roretzla at linux.microsoft.com>; Parav Pandit
> > <parav at nvidia.com>; Xueming Li <xuemingl at nvidia.com>; Nipun Gupta
> > <nipun.gupta at amd.com>; Nikhil Agarwal <nikhil.agarwal at amd.com>; Hemant
> > Agrawal <hemant.agrawal at nxp.com>; Sachin Saxena
> > <sachin.saxena at nxp.com>; Rosen Xu <rosen.xu at intel.com>; Chenbo Xia
> > <chenbox at nvidia.com>; Tomasz Duszynski <tduszynski at marvell.com>;
> > Chengwen Feng <fengchengwen at huawei.com>; NBU-Contact-longli
> > (EXTERNAL) <longli at microsoft.com>; Wei Hu <weh at microsoft.com>; Bruce
> > Richardson <bruce.richardson at intel.com>; Kevin Laatz
> > <kevin.laatz at intel.com>; Jan Blunck <jblunck at 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 at nvidia.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Stephen Hemminger <stephen at networkplumber.org>
> > > > Sent: Thursday, 20 February 2025 20:33
> > > > To: Shani Peretz <shperetz at nvidia.com>
> > > > Cc: dev at dpdk.org; Tyler Retzlaff <roretzla at linux.microsoft.com>;
> > > > Parav Pandit <parav at nvidia.com>; Xueming Li <xuemingl at nvidia.com>;
> > > > Nipun Gupta <nipun.gupta at amd.com>; Nikhil Agarwal
> > > > <nikhil.agarwal at amd.com>; Hemant Agrawal <hemant.agrawal at nxp.com>;
> > > > Sachin Saxena <sachin.saxena at nxp.com>; Rosen Xu
> > > > <rosen.xu at intel.com>; Chenbo Xia <chenbox at nvidia.com>; Tomasz
> > > > Duszynski <tduszynski at marvell.com>; Chengwen Feng
> > > > <fengchengwen at huawei.com>; NBU-Contact-longli
> > > > (EXTERNAL) <longli at microsoft.com>; Wei Hu <weh at microsoft.com>; Bruce
> > > > Richardson <bruce.richardson at intel.com>; Kevin Laatz
> > > > <kevin.laatz at intel.com>; Jan Blunck <jblunck at 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 at 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");
 }