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 <[email protected]> > Sent: Wednesday, 30 April 2025 19:00 > To: Shani Peretz <[email protected]> > Cc: [email protected]; Tyler Retzlaff <[email protected]>; Parav Pandit > <[email protected]>; Xueming Li <[email protected]>; Nipun Gupta > <[email protected]>; Nikhil Agarwal <[email protected]>; Hemant > Agrawal <[email protected]>; Sachin Saxena > <[email protected]>; Rosen Xu <[email protected]>; Chenbo Xia > <[email protected]>; Tomasz Duszynski <[email protected]>; > Chengwen Feng <[email protected]>; NBU-Contact-longli > (EXTERNAL) <[email protected]>; Wei Hu <[email protected]>; Bruce > Richardson <[email protected]>; Kevin Laatz > <[email protected]>; Jan Blunck <[email protected]> > 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 <[email protected]> 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 <[email protected]> > > > Sent: Monday, 17 March 2025 16:11 > > > To: Shani Peretz <[email protected]> > > > Cc: [email protected]; Tyler Retzlaff <[email protected]>; > > > Parav Pandit <[email protected]>; Xueming Li <[email protected]>; > > > Nipun Gupta <[email protected]>; Nikhil Agarwal > > > <[email protected]>; Hemant Agrawal <[email protected]>; > > > Sachin Saxena <[email protected]>; Rosen Xu > > > <[email protected]>; Chenbo Xia <[email protected]>; Tomasz > > > Duszynski <[email protected]>; Chengwen Feng > > > <[email protected]>; NBU-Contact-longli > > > (EXTERNAL) <[email protected]>; Wei Hu <[email protected]>; Bruce > > > Richardson <[email protected]>; Kevin Laatz > > > <[email protected]>; Jan Blunck <[email protected]> > > > 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 <[email protected]> wrote: > > > > > > > > -----Original Message----- > > > > > From: Stephen Hemminger <[email protected]> > > > > > Sent: Thursday, 20 February 2025 20:33 > > > > > To: Shani Peretz <[email protected]> > > > > > Cc: [email protected]; Tyler Retzlaff <[email protected]>; > > > > > Parav Pandit <[email protected]>; Xueming Li > > > > > <[email protected]>; Nipun Gupta <[email protected]>; Nikhil > > > > > Agarwal <[email protected]>; Hemant Agrawal > > > > > <[email protected]>; Sachin Saxena <[email protected]>; > > > > > Rosen Xu <[email protected]>; Chenbo Xia <[email protected]>; > > > > > Tomasz Duszynski <[email protected]>; Chengwen Feng > > > > > <[email protected]>; NBU-Contact-longli > > > > > (EXTERNAL) <[email protected]>; Wei Hu <[email protected]>; > > > > > Bruce Richardson <[email protected]>; Kevin Laatz > > > > > <[email protected]>; Jan Blunck <[email protected]> > > > > > 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 > > > > > <[email protected]> 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"); > } >
