[v2] eal: fix to set the rte_device ptr's device args before hotplug
diff mbox series

Message ID 20200214064353.31022-1-somnath.kotur@broadcom.com
State Accepted
Delegated to: Thomas Monjalon
Headers show
Series
  • [v2] eal: fix to set the rte_device ptr's device args before hotplug
Related show

Checks

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

Commit Message

Somnath Kotur Feb. 14, 2020, 6:43 a.m. UTC
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

Gaetan Rivet Feb. 14, 2020, 8:24 a.m. UTC | #1
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>
Somnath Kotur March 20, 2020, 4:21 a.m. UTC | #2
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
Thomas Monjalon March 31, 2020, 12:52 a.m. UTC | #3
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

Patch
diff mbox series

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);
 			}