[v2] eal: fix to set the rte_device ptr's device args before hotplug
Checks
Commit Message
As per the comments in this code section, since there is a matching device,
it is now its responsibility to manage the devargs we've just inserted.
But the matching device ptr's devargs is still uninitialized or not pointing
to the newest dev_args that were passed as a parameter to local_dev_probe().
This is needed particularly in the case when *probe is called again* on an
already probed device as part of adding a representor port to an OVS switch(OVS-DPDK)
Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
v1->v2: Incorporated suggestions from Gaetan Rivet
drivers/bus/pci/linux/pci.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
On 14/02/2020 07:43, Somnath Kotur wrote:
> As per the comments in this code section, since there is a matching device,
> it is now its responsibility to manage the devargs we've just inserted.
> But the matching device ptr's devargs is still uninitialized or not pointing
> to the newest dev_args that were passed as a parameter to local_dev_probe().
> This is needed particularly in the case when *probe is called again* on an
> already probed device as part of adding a representor port to an OVS switch(OVS-DPDK)
>
> Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> ---
> v1->v2: Incorporated suggestions from Gaetan Rivet
> drivers/bus/pci/linux/pci.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> index 740a2cd..71b0a30 100644
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -377,6 +377,11 @@
> */
> RTE_LOG(ERR, EAL, "Unexpected device scan at %s!\n",
> filename);
> + else if (dev2->device.devargs !=
> + dev->device.devargs) {
> + rte_devargs_remove(dev2->device.devargs);
> + pci_name_set(dev2);
> + }
> }
> free(dev);
> }
>
Hi Somnath,
I see that this is already pretty similar in BSD (minus the rte_devargs_remove()),
so if you have tested and validated that this works properly I'm fine with this patch.
This might miss a Cc: stable@dpdk.org, otherwise,
Acked-by: Gaetan Rivet <grive@u256.net>
On Mon, Feb 17, 2020 at 3:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 17/02/2020 11:02, Ferruh Yigit:
> > On 2/17/2020 3:18 AM, Somnath Kotur wrote:
> > > On Fri, Feb 14, 2020 at 1:54 PM Gaetan Rivet <grive@u256.net> wrote:
> > >>
> > >> On 14/02/2020 07:43, Somnath Kotur wrote:
> > >>> As per the comments in this code section, since there is a matching device,
> > >>> it is now its responsibility to manage the devargs we've just inserted.
> > >>> But the matching device ptr's devargs is still uninitialized or not pointing
> > >>> to the newest dev_args that were passed as a parameter to local_dev_probe().
> > >>> This is needed particularly in the case when *probe is called again* on an
> > >>> already probed device as part of adding a representor port to an OVS switch(OVS-DPDK)
> > >>>
> > >>> Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
> > >>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > >>> ---
> > >>> v1->v2: Incorporated suggestions from Gaetan Rivet
> > >>> drivers/bus/pci/linux/pci.c | 5 +++++
> > >>> 1 file changed, 5 insertions(+)
> > >>>
> > >>> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> > >>> index 740a2cd..71b0a30 100644
> > >>> --- a/drivers/bus/pci/linux/pci.c
> > >>> +++ b/drivers/bus/pci/linux/pci.c
> > >>> @@ -377,6 +377,11 @@
> > >>> */
> > >>> RTE_LOG(ERR, EAL, "Unexpected device scan at %s!\n",
> > >>> filename);
> > >>> + else if (dev2->device.devargs !=
> > >>> + dev->device.devargs) {
> > >>> + rte_devargs_remove(dev2->device.devargs);
> > >>> + pci_name_set(dev2);
> > >>> + }
> > >>> }
> > >>> free(dev);
> > >>> }
> > >>>
> > >>
> > >> Hi Somnath,
> > >>
> > >> I see that this is already pretty similar in BSD (minus the rte_devargs_remove()),
> > >> so if you have tested and validated that this works properly I'm fine with this patch.
> > >>
> > >> This might miss a Cc: stable@dpdk.org, otherwise,
> > >>
> > >> Acked-by: Gaetan Rivet <grive@u256.net>
> > >
> > > Off the list.
> > > Thanks Gaetan. Ferruh : Anything else you waiting from my side or is
> > > this done ?
> >
> > Hi Somnath,
> >
> > The patch is for the main repo, it is Thomas who will merge it, cc'ed.
>
> I won't take any risk changing this critical code in the last days of 20.02.
> I will take time to review it carefully post-20.02.
>
Thomas,
Now that 20.02 is out and we are already in the 20.05
window, could you please merge this in or pls give me an ETA by when
you think you'll be able to do it?
Thank you so much!
Som
14/02/2020 09:24, Gaetan Rivet:
> On 14/02/2020 07:43, Somnath Kotur wrote:
> > As per the comments in this code section, since there is a matching device,
> > it is now its responsibility to manage the devargs we've just inserted.
> > But the matching device ptr's devargs is still uninitialized or not pointing
> > to the newest dev_args that were passed as a parameter to local_dev_probe().
> > This is needed particularly in the case when *probe is called again* on an
> > already probed device as part of adding a representor port to an OVS switch(OVS-DPDK)
> >
> > Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > ---
> > --- a/drivers/bus/pci/linux/pci.c
> > +++ b/drivers/bus/pci/linux/pci.c
> > + else if (dev2->device.devargs !=
> > + dev->device.devargs) {
> > + rte_devargs_remove(dev2->device.devargs);
> > + pci_name_set(dev2);
> > + }
>
> I see that this is already pretty similar in BSD (minus the rte_devargs_remove()),
We really need to review this kind of code for Linux and FreeBSD,
and share the common code.
> so if you have tested and validated that this works properly I'm fine with this patch.
>
> This might miss a Cc: stable@dpdk.org, otherwise,
>
> Acked-by: Gaetan Rivet <grive@u256.net>
I don't like how complicate this function is becoming,
but because it's tested and acked,
Applied, thanks
Title updated: bus/pci: fix devargs on probing again
@@ -377,6 +377,11 @@
*/
RTE_LOG(ERR, EAL, "Unexpected device scan at %s!\n",
filename);
+ else if (dev2->device.devargs !=
+ dev->device.devargs) {
+ rte_devargs_remove(dev2->device.devargs);
+ pci_name_set(dev2);
+ }
}
free(dev);
}