[v2] bus/pci: update device devargs on each rescan
Checks
Commit Message
From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Bus rescan is done e.g. during the device hotplug,
where devargs are re-allocated. By not updating the
rte_device->devargs pointer we potentially make it
a dangling one, as previous devargs could have been
(or will be soon) freed.
Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
Cc: qi.z.zhang@intel.com
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
Changes since v1:
* introduce additional pci_devargs_lookup() check to make
the code slightly more readable (Gaetan Rivet)
drivers/bus/pci/linux/pci.c | 5 +++++
drivers/bus/pci/pci_common.c | 3 ++-
drivers/bus/pci/private.h | 11 +++++++++++
3 files changed, 18 insertions(+), 1 deletion(-)
Comments
> -----Original Message-----
> From: Dariusz Stojaczyk [mailto:darek.stojaczyk@gmail.com]
> Sent: Monday, November 5, 2018 10:40 PM
> To: dev@dpdk.org; gaetan.rivet@6wind.com
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: [PATCH v2] bus/pci: update device devargs on each rescan
>
> From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
>
> Bus rescan is done e.g. during the device hotplug, where devargs are
> re-allocated. By not updating the rte_device->devargs pointer we potentially
> make it a dangling one, as previous devargs could have been (or will be soon)
> freed.
Yes, this is the similar issue we met on vdev.
The key problem is we have rte_devargs_insert will destroy a devargs which is still referenced by a rte_device
I'm thinking , why not just keep always keep a snapshot of devargs in rte_device to make things simple?
We could introduce a API like rte_dev_assign_devargs(dev, devargs) which handled the clone and destroy stuff and can be used for all buses.
If that idea is acceptable, I would prefer the issue in this patch could be fixed in pci_name_set by clone a new copy as workaround.
>
> Fixes: 55e411b301c3 ("bus/pci: fix resource mapping override")
> Cc: qi.z.zhang@intel.com
>
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
> Changes since v1:
> * introduce additional pci_devargs_lookup() check to make
> the code slightly more readable (Gaetan Rivet)
>
> drivers/bus/pci/linux/pci.c | 5 +++++ drivers/bus/pci/pci_common.c |
> 3 ++-
> drivers/bus/pci/private.h | 11 +++++++++++
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c index
> c99d523f0..a32d4848e 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -379,6 +379,11 @@ pci_scan_one(const char *dirname, const struct
> rte_pci_addr *addr)
> RTE_LOG(ERR, EAL, "Unexpected device scan
> at %s!\n",
> filename);
> }
> +
> + if (pci_devargs_lookup(dev2) !=
> + dev2->device.devargs)
> + pci_name_set(dev2);
> +
> free(dev);
> }
> return 0;
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index 6276e5d69..fcaf76ed2 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -42,7 +42,8 @@ const char *rte_pci_get_sysfs_path(void)
> return path;
> }
>
> -static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
> +struct rte_devargs *
> +pci_devargs_lookup(struct rte_pci_device *dev)
> {
> struct rte_devargs *devargs;
> struct rte_pci_addr addr;
> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h index
> 13c3324bb..8b2e904f0 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -43,6 +43,17 @@ void
> pci_name_set(struct rte_pci_device *dev);
>
> /**
> + * Find matching up-to-date devargs for a given device.
> + *
> + * @param pci_dev
> + * PCI device
> + * @return
> + * A devargs pointer or NULL
> + */
> +struct rte_devargs *
> +pci_devargs_lookup(struct rte_pci_device *dev);
> +
> +/**
> * Add a PCI device to the PCI Bus (append to PCI Device list). This function
> * also updates the bus references of the PCI Device (and the generic device
> * object embedded within.
> --
> 2.11.0
On Tue, Nov 06, 2018 at 10:21:38PM +0000, Zhang, Qi Z wrote:
>
>
> > -----Original Message-----
> > From: Dariusz Stojaczyk [mailto:darek.stojaczyk@gmail.com]
> > Sent: Monday, November 5, 2018 10:40 PM
> > To: dev@dpdk.org; gaetan.rivet@6wind.com
> > Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: [PATCH v2] bus/pci: update device devargs on each rescan
> >
> > From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> >
> > Bus rescan is done e.g. during the device hotplug, where devargs are
> > re-allocated. By not updating the rte_device->devargs pointer we potentially
> > make it a dangling one, as previous devargs could have been (or will be soon)
> > freed.
>
> Yes, this is the similar issue we met on vdev.
>
> The key problem is we have rte_devargs_insert will destroy a devargs which is still referenced by a rte_device
> I'm thinking , why not just keep always keep a snapshot of devargs in rte_device to make things simple?
> We could introduce a API like rte_dev_assign_devargs(dev, devargs) which handled the clone and destroy stuff and can be used for all buses.
> If that idea is acceptable, I would prefer the issue in this patch could be fixed in pci_name_set by clone a new copy as workaround.
>
I agree that it would be useful to have
rte_devargs_map(devargs, device);
That will link safely a devargs to a device, but cloning is overkill.
I disagree that dangling pointers and loose references is fixed by
introducing more cloning and copies of stuff here and there.
We must tighten the device -> devargs coupling, not loosen it.
07/11/2018 00:40, Gaëtan Rivet:
> On Tue, Nov 06, 2018 at 10:21:38PM +0000, Zhang, Qi Z wrote:
> > From: Dariusz Stojaczyk [mailto:darek.stojaczyk@gmail.com]
> > > From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > >
> > > Bus rescan is done e.g. during the device hotplug, where devargs are
> > > re-allocated. By not updating the rte_device->devargs pointer we potentially
> > > make it a dangling one, as previous devargs could have been (or will be soon)
> > > freed.
> >
> > Yes, this is the similar issue we met on vdev.
> >
> > The key problem is we have rte_devargs_insert will destroy a devargs which is still referenced by a rte_device
> > I'm thinking , why not just keep always keep a snapshot of devargs in rte_device to make things simple?
> > We could introduce a API like rte_dev_assign_devargs(dev, devargs) which handled the clone and destroy stuff and can be used for all buses.
> > If that idea is acceptable, I would prefer the issue in this patch could be fixed in pci_name_set by clone a new copy as workaround.
> >
>
> I agree that it would be useful to have
>
> rte_devargs_map(devargs, device);
>
> That will link safely a devargs to a device, but cloning is overkill.
>
> I disagree that dangling pointers and loose references is fixed by
> introducing more cloning and copies of stuff here and there.
>
> We must tighten the device -> devargs coupling, not loosen it.
This issue is fixed with a different approach:
http://git.dpdk.org/dpdk/commit/?id=c7ad7754
devargs: do not replace already inserted device
@@ -379,6 +379,11 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
RTE_LOG(ERR, EAL, "Unexpected device scan at %s!\n",
filename);
}
+
+ if (pci_devargs_lookup(dev2) !=
+ dev2->device.devargs)
+ pci_name_set(dev2);
+
free(dev);
}
return 0;
@@ -42,7 +42,8 @@ const char *rte_pci_get_sysfs_path(void)
return path;
}
-static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
+struct rte_devargs *
+pci_devargs_lookup(struct rte_pci_device *dev)
{
struct rte_devargs *devargs;
struct rte_pci_addr addr;
@@ -43,6 +43,17 @@ void
pci_name_set(struct rte_pci_device *dev);
/**
+ * Find matching up-to-date devargs for a given device.
+ *
+ * @param pci_dev
+ * PCI device
+ * @return
+ * A devargs pointer or NULL
+ */
+struct rte_devargs *
+pci_devargs_lookup(struct rte_pci_device *dev);
+
+/**
* Add a PCI device to the PCI Bus (append to PCI Device list). This function
* also updates the bus references of the PCI Device (and the generic device
* object embedded within.