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 <shper...@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(-)
>
