diff mbox series

[v2] pci/linux: copy new id for inserted device

Message ID 20200824211038.48932-1-james.r.harris@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] pci/linux: copy new id for inserted device |

Checks

Context Check Description
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/checkpatch success coding style OK

Commit Message

Harris, James R Aug. 24, 2020, 9:10 p.m. UTC 
  When a device is inserted into an existing BDF slot
that has not been probed, we must overwrite the old
PCI ID with the ID of the new function. Otherwise
we may not probe the function with the correct driver,
if at all.

Signed-off-by: Jim Harris <james.r.harris@intel.com>
---
 drivers/bus/pci/linux/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon Oct. 7, 2020, 3:05 p.m. UTC | #1 
Hi Jim,

Sorry I see nobody reviewed your patch.

Jim Harris <james.r.harris@intel.com> wrote:
> + memcpy(&dev2->id, &dev->id, sizeof(dev2->id));
[...]
> + memcmp(&dev2->id, &dev->id, sizeof(dev2->id)))

Why using memcpy and memcmp instead of simple assignment and comparison?
Harris, James R Oct. 12, 2020, 8:58 p.m. UTC | #2 
On 10/7/20, 8:06 AM, "Thomas Monjalon" <thomas@monjalon.net> wrote:

    Hi Jim,

    Sorry I see nobody reviewed your patch.

    Jim Harris <james.r.harris@intel.com> wrote:
    > + memcpy(&dev2->id, &dev->id, sizeof(dev2->id));
    [...]
    > + memcmp(&dev2->id, &dev->id, sizeof(dev2->id)))

    Why using memcpy and memcmp instead of simple assignment and comparison?

Direct assignment and comparison would work too.  I did see some similar cases though using memcpy for rte_pci_addr (which is similar to rte_pci_id) in linux/pci_uio.c and linux/pci_vfio.c.  It wasn't clear to me if direct assignment/comparison for structures was the norm for DPDK.

I'm happy to send a v2 with a direct assignment/comparison though if that is preferred.

-Jim
Thomas Monjalon Oct. 12, 2020, 9:54 p.m. UTC | #3 
12/10/2020 22:58, Harris, James R:
> 
> On 10/7/20, 8:06 AM, "Thomas Monjalon" <thomas@monjalon.net> wrote:
> 
>     Hi Jim,
> 
>     Sorry I see nobody reviewed your patch.
> 
>     Jim Harris <james.r.harris@intel.com> wrote:
>     > + memcpy(&dev2->id, &dev->id, sizeof(dev2->id));
>     [...]
>     > + memcmp(&dev2->id, &dev->id, sizeof(dev2->id)))
> 
>     Why using memcpy and memcmp instead of simple assignment and comparison?
> 
> Direct assignment and comparison would work too.  I did see some similar cases though using memcpy for rte_pci_addr (which is similar to rte_pci_id) in linux/pci_uio.c and linux/pci_vfio.c.  It wasn't clear to me if direct assignment/comparison for structures was the norm for DPDK.
> 
> I'm happy to send a v2 with a direct assignment/comparison though if that is preferred.

Yes please, direct assignment is preferred.
Thanks
diff mbox series

Patch

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index a2198abf4..d8fd973b2 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -352,6 +352,7 @@  pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 				if (!rte_dev_is_probed(&dev2->device)) {
 					dev2->kdrv = dev->kdrv;
 					dev2->max_vfs = dev->max_vfs;
+					memcpy(&dev2->id, &dev->id, sizeof(dev2->id));
 					pci_name_set(dev2);
 					memmove(dev2->mem_resource,
 						dev->mem_resource,
@@ -365,7 +366,8 @@  pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 					 * need to do anything here unless...
 					 **/
 					if (dev2->kdrv != dev->kdrv ||
-						dev2->max_vfs != dev->max_vfs)
+						dev2->max_vfs != dev->max_vfs ||
+						memcmp(&dev2->id, &dev->id, sizeof(dev2->id)))
 						/*
 						 * This should not happens.
 						 * But it is still possible if