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

[Shani Peretz <shperetz@nvidia.com>]

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

> 
> > The proposed solution is to utilize the parse function implemented by
> > each bus. When comparing names, we will call parse on the supplied
> > string as well as on the device name itself and compare the results.
> > As part of the change the parse function will now recive the address
> > to write to and the size of the pointer, in addition it will return
> > the size of the parsed address.
> 
> This leads to more complexity than needed, the layering here is a bit of a mess
> already. Way too complex as it is.
> There is complex nesting between generic bus code, pci bus code, kvargs
> processing and drivers.
> 
> Why does the PCI code need to be calling generic code for parse.
> 
> 
> > This will allow consistent comparisons between different
> > representations of same devices.
> 
> Not a fan of how wide this change ends up being. Would like to keep it just to
> PCI.


I agree it became too wide.

As I see it we have 3 options:
	1. Add a function that returns the canonical form string of a device and call it before find_device is called (in local_dev_probe for example)
	2. Add a comparison function, pci bus will call this specific comparison (something like what I did in here https://patches.dpdk.org/project/dpdk/patch/20240708165145.1405107-1-shperetz@nvidia.com/) 
	3. The current proposal

What do you think?

Thanks for your help, and sorry for the late response