Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Jan 29, 2025 at 10:54:16AM +0200, Shani Peretz wrote:
> DPDK provides two formats for specifying PCI device numbers:
> a full version ("0000:08:00.0") and a short version ("08:00.0").
> Issues can occur when an application uses one format (e.g., short)
> while running testpmd, then attempts to use the other format
> (e.g., full) in a later command, resulting in a failure.
> 
> The issue is that find_device goes over the list of devices and
> compares the user-provided string to the rte_device structure's
> device->name (device->name is just the string received from devargs
> (i.e "08:00.0" or "0000:08:00.0")).
> Notice that there's another field that represents the device name,
> but this one is in the rte_pci_bus struct. This name is actually the result
> of the PCI parse function ("0000:08:00.0").
> 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.
> 
> To make the cmp_dev_name function applicable to all buses—not just PCI—
> 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.
> This will allow consistent comparisons between different representations
> of same devices.
> 
> Also, the pci_common_set function has been modified to improve naming
> consistency for PCI buses.
> Now, the name stored in rte_device for PCI buses will match the parsed
> name that is also stored in rte_pci_device name,
> rather than using the user-provided string from devargs.
> As a result, when a new PCI device is registered, the name displayed in
> the device list will be the parsed version.
> 
> Added tests that compare and find devices in various forms of names
> under test_devargs.
> 
> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> 
> Signed-off-by: Shani Peretz <shperetz@nvidia.com>
> ---

Thanks, I believe this fixes issues that I previously encountered.

However, there are a lot of changes in this one patch, so review would be
easier if it could be split a bit. Could you please look to split it up.
For example, one possible split might be:

* have one patch to adjust the bus parse function to add the extra
  parameter there. That should clear most/all of the non-pci driver changes.
* have a separate patch at the end for testing code.
* the middle patch can therefore be focused on rest of the problem
  statement and especially on the PCI bus and EAL changes.

Would such a split work?

Thanks,
/Bruce

>  app/test/test_devargs.c                  | 123 ++++++++++++++++++++++-
>  app/test/test_vdev.c                     |  10 +-
>  drivers/bus/auxiliary/auxiliary_common.c |  17 +++-
>  drivers/bus/cdx/cdx.c                    |  13 ++-
>  drivers/bus/dpaa/dpaa_bus.c              |   7 +-
>  drivers/bus/fslmc/fslmc_bus.c            |   9 +-
>  drivers/bus/ifpga/ifpga_bus.c            |  14 ++-
>  drivers/bus/pci/pci_common.c             |  22 ++--
>  drivers/bus/platform/platform.c          |  15 ++-
>  drivers/bus/uacce/uacce.c                |  14 ++-
>  drivers/bus/vdev/vdev.c                  |  23 ++++-
>  drivers/bus/vmbus/vmbus_common.c         |   9 +-
>  drivers/dma/idxd/idxd_bus.c              |   9 +-
>  drivers/raw/ifpga/ifpga_rawdev.c         |   8 +-
>  lib/eal/common/eal_common_bus.c          |   2 +-
>  lib/eal/common/eal_common_dev.c          |  41 +++++++-
>  lib/eal/common/hotplug_mp.c              |  11 +-
>  lib/eal/include/bus_driver.h             |  24 ++++-
>  lib/eal/include/rte_dev.h                |  15 +++
>  lib/eal/linux/eal_dev.c                  |  10 +-
>  lib/eal/version.map                      |   1 +
>  21 files changed, 305 insertions(+), 92 deletions(-)
>