DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
@ 2018-10-25  3:30 Qi Zhang
  2018-10-25  9:51 ` Gaëtan Rivet
  2018-10-28 17:32 ` Thomas Monjalon
  0 siblings, 2 replies; 7+ messages in thread
From: Qi Zhang @ 2018-10-25  3:30 UTC (permalink / raw)
  To: thomas, gaetan.rivet; +Cc: dev, Qi Zhang, stable

It's not necessary to insert device argment to devargs_list
during bus scan, but this happens when we try to attach a
device on secondary process. The patch fix the issue.

Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/bus/vdev/vdev.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 688e31c21..818a2bfc2 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char *args)
 }
 
 static int
-insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
+insert_vdev(const char *name, const char *args,
+		struct rte_vdev_device **p_dev,
+		bool init)
 {
 	struct rte_vdev_device *dev;
 	struct rte_devargs *devargs;
@@ -237,7 +239,8 @@ insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
 	}
 
 	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
-	rte_devargs_insert(devargs);
+	if (init)
+		rte_devargs_insert(devargs);
 
 	if (p_dev)
 		*p_dev = dev;
@@ -257,7 +260,7 @@ rte_vdev_init(const char *name, const char *args)
 	int ret;
 
 	rte_spinlock_recursive_lock(&vdev_device_list_lock);
-	ret = insert_vdev(name, args, &dev);
+	ret = insert_vdev(name, args, &dev, true);
 	if (ret == 0) {
 		ret = vdev_probe_all_drivers(dev);
 		if (ret) {
@@ -383,7 +386,7 @@ vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
 		break;
 	case VDEV_SCAN_ONE:
 		VDEV_LOG(INFO, "receive vdev, %s", in->name);
-		ret = insert_vdev(in->name, NULL, NULL);
+		ret = insert_vdev(in->name, NULL, NULL, false);
 		if (ret == -EEXIST)
 			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
 		else if (ret < 0)
-- 
2.13.6

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
  2018-10-25  3:30 [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan Qi Zhang
@ 2018-10-25  9:51 ` Gaëtan Rivet
  2018-10-25 14:56   ` Zhang, Qi Z
  2018-10-28 17:32 ` Thomas Monjalon
  1 sibling, 1 reply; 7+ messages in thread
From: Gaëtan Rivet @ 2018-10-25  9:51 UTC (permalink / raw)
  To: Qi Zhang; +Cc: thomas, dev, stable

On Thu, Oct 25, 2018 at 11:30:36AM +0800, Qi Zhang wrote:
> It's not necessary to insert device argment to devargs_list
> during bus scan, but this happens when we try to attach a
> device on secondary process. The patch fix the issue.
> 
> Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index 688e31c21..818a2bfc2 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char *args)
>  }
>  
>  static int
> -insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
> +insert_vdev(const char *name, const char *args,
> +		struct rte_vdev_device **p_dev,
> +		bool init)

Why is vdev the only bus not using hotplug API itself and reimplementing
it on its own?

It should not have to insert devargs at all, not even in the primary
process. If it called rte_dev_probe(), this would normally already be
properly handled I think.

>  {
>  	struct rte_vdev_device *dev;
>  	struct rte_devargs *devargs;
> @@ -237,7 +239,8 @@ insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
>  	}
>  
>  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
> -	rte_devargs_insert(devargs);
> +	if (init)
> +		rte_devargs_insert(devargs);
>  
>  	if (p_dev)
>  		*p_dev = dev;
> @@ -257,7 +260,7 @@ rte_vdev_init(const char *name, const char *args)
>  	int ret;
>  
>  	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> -	ret = insert_vdev(name, args, &dev);
> +	ret = insert_vdev(name, args, &dev, true);
>  	if (ret == 0) {
>  		ret = vdev_probe_all_drivers(dev);
>  		if (ret) {
> @@ -383,7 +386,7 @@ vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
>  		break;
>  	case VDEV_SCAN_ONE:
>  		VDEV_LOG(INFO, "receive vdev, %s", in->name);
> -		ret = insert_vdev(in->name, NULL, NULL);
> +		ret = insert_vdev(in->name, NULL, NULL, false);
>  		if (ret == -EEXIST)
>  			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
>  		else if (ret < 0)
> -- 
> 2.13.6
> 

-- 
Gaëtan Rivet
6WIND

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
  2018-10-25  9:51 ` Gaëtan Rivet
@ 2018-10-25 14:56   ` Zhang, Qi Z
  2018-10-25 15:03     ` Gaëtan Rivet
  0 siblings, 1 reply; 7+ messages in thread
From: Zhang, Qi Z @ 2018-10-25 14:56 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: thomas, dev, stable



> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Thursday, October 25, 2018 4:51 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus scan
> 
> On Thu, Oct 25, 2018 at 11:30:36AM +0800, Qi Zhang wrote:
> > It's not necessary to insert device argment to devargs_list during bus
> > scan, but this happens when we try to attach a device on secondary
> > process. The patch fix the issue.
> >
> > Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  drivers/bus/vdev/vdev.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index
> > 688e31c21..818a2bfc2 100644
> > --- a/drivers/bus/vdev/vdev.c
> > +++ b/drivers/bus/vdev/vdev.c
> > @@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char *args)
> > }
> >
> >  static int
> > -insert_vdev(const char *name, const char *args, struct
> > rte_vdev_device **p_dev)
> > +insert_vdev(const char *name, const char *args,
> > +		struct rte_vdev_device **p_dev,
> > +		bool init)
> 
> Why is vdev the only bus not using hotplug API itself and reimplementing it
> on its own?

I don't know the history, 
but replace insert_vdev with hotplug API will not solve all the problem (see my below comments)

> 
> It should not have to insert devargs at all, not even in the primary process. If
> it called rte_dev_probe(), this would normally already be properly handled I
> think.

Currently insert_vdev is shared by two scenarios
1. rte_vdev_init, which is called by application to attach a new device, I agree it's better to have rte_dev_probe here so, no need to have insert_vdev here.
2. during secondary's vdev->scan when it receive a SCAN_ONE event from primary, it should not call rte_devargs_insert, 
The patch is going to fix the issue on secondary scenario and we can do the cleanup for first issue in a separate patch



> 
> >  {
> >  	struct rte_vdev_device *dev;
> >  	struct rte_devargs *devargs;
> > @@ -237,7 +239,8 @@ insert_vdev(const char *name, const char *args,
> struct rte_vdev_device **p_dev)
> >  	}
> >
> >  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
> > -	rte_devargs_insert(devargs);
> > +	if (init)
> > +		rte_devargs_insert(devargs);
> >
> >  	if (p_dev)
> >  		*p_dev = dev;
> > @@ -257,7 +260,7 @@ rte_vdev_init(const char *name, const char *args)
> >  	int ret;
> >
> >  	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> > -	ret = insert_vdev(name, args, &dev);
> > +	ret = insert_vdev(name, args, &dev, true);
> >  	if (ret == 0) {
> >  		ret = vdev_probe_all_drivers(dev);
> >  		if (ret) {
> > @@ -383,7 +386,7 @@ vdev_action(const struct rte_mp_msg *mp_msg,
> const void *peer)
> >  		break;
> >  	case VDEV_SCAN_ONE:
> >  		VDEV_LOG(INFO, "receive vdev, %s", in->name);
> > -		ret = insert_vdev(in->name, NULL, NULL);
> > +		ret = insert_vdev(in->name, NULL, NULL, false);
> >  		if (ret == -EEXIST)
> >  			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
> >  		else if (ret < 0)
> > --
> > 2.13.6
> >
> 
> --
> Gaëtan Rivet
> 6WIND

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
  2018-10-25 14:56   ` Zhang, Qi Z
@ 2018-10-25 15:03     ` Gaëtan Rivet
  2018-10-25 15:18       ` Zhang, Qi Z
  0 siblings, 1 reply; 7+ messages in thread
From: Gaëtan Rivet @ 2018-10-25 15:03 UTC (permalink / raw)
  To: Zhang, Qi Z; +Cc: thomas, dev, stable

On Thu, Oct 25, 2018 at 02:56:55PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Thursday, October 25, 2018 4:51 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus scan
> > 
> > On Thu, Oct 25, 2018 at 11:30:36AM +0800, Qi Zhang wrote:
> > > It's not necessary to insert device argment to devargs_list during bus
> > > scan, but this happens when we try to attach a device on secondary
> > > process. The patch fix the issue.
> > >
> > > Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > ---
> > >  drivers/bus/vdev/vdev.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index
> > > 688e31c21..818a2bfc2 100644
> > > --- a/drivers/bus/vdev/vdev.c
> > > +++ b/drivers/bus/vdev/vdev.c
> > > @@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char *args)
> > > }
> > >
> > >  static int
> > > -insert_vdev(const char *name, const char *args, struct
> > > rte_vdev_device **p_dev)
> > > +insert_vdev(const char *name, const char *args,
> > > +		struct rte_vdev_device **p_dev,
> > > +		bool init)
> > 
> > Why is vdev the only bus not using hotplug API itself and reimplementing it
> > on its own?
> 
> I don't know the history, 
> but replace insert_vdev with hotplug API will not solve all the problem (see my below comments)
> 
> > 
> > It should not have to insert devargs at all, not even in the primary process. If
> > it called rte_dev_probe(), this would normally already be properly handled I
> > think.
> 
> Currently insert_vdev is shared by two scenarios
> 1. rte_vdev_init, which is called by application to attach a new device, I agree it's better to have rte_dev_probe here so, no need to have insert_vdev here.
> 2. during secondary's vdev->scan when it receive a SCAN_ONE event from primary, it should not call rte_devargs_insert, 
> The patch is going to fix the issue on secondary scenario and we can do the cleanup for first issue in a separate patch

In 2., won't dev_probe() check for secondary process context and in this
case, send the request to primary, which will see that the device is
already probed, which would thus fix your issue?

My take on it is that it seems to be fixing both your issue and cleaning
up history.

> 
> 
> 
> > 
> > >  {
> > >  	struct rte_vdev_device *dev;
> > >  	struct rte_devargs *devargs;
> > > @@ -237,7 +239,8 @@ insert_vdev(const char *name, const char *args,
> > struct rte_vdev_device **p_dev)
> > >  	}
> > >
> > >  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
> > > -	rte_devargs_insert(devargs);
> > > +	if (init)
> > > +		rte_devargs_insert(devargs);
> > >
> > >  	if (p_dev)
> > >  		*p_dev = dev;
> > > @@ -257,7 +260,7 @@ rte_vdev_init(const char *name, const char *args)
> > >  	int ret;
> > >
> > >  	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> > > -	ret = insert_vdev(name, args, &dev);
> > > +	ret = insert_vdev(name, args, &dev, true);
> > >  	if (ret == 0) {
> > >  		ret = vdev_probe_all_drivers(dev);
> > >  		if (ret) {
> > > @@ -383,7 +386,7 @@ vdev_action(const struct rte_mp_msg *mp_msg,
> > const void *peer)
> > >  		break;
> > >  	case VDEV_SCAN_ONE:
> > >  		VDEV_LOG(INFO, "receive vdev, %s", in->name);
> > > -		ret = insert_vdev(in->name, NULL, NULL);
> > > +		ret = insert_vdev(in->name, NULL, NULL, false);
> > >  		if (ret == -EEXIST)
> > >  			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
> > >  		else if (ret < 0)
> > > --
> > > 2.13.6
> > >
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
  2018-10-25 15:03     ` Gaëtan Rivet
@ 2018-10-25 15:18       ` Zhang, Qi Z
  2018-10-25 15:26         ` Gaëtan Rivet
  0 siblings, 1 reply; 7+ messages in thread
From: Zhang, Qi Z @ 2018-10-25 15:18 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: thomas, dev, stable



> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Thursday, October 25, 2018 10:03 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus scan
> 
> On Thu, Oct 25, 2018 at 02:56:55PM +0000, Zhang, Qi Z wrote:
> >
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Thursday, October 25, 2018 4:51 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> > > Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus
> > > scan
> > >
> > > On Thu, Oct 25, 2018 at 11:30:36AM +0800, Qi Zhang wrote:
> > > > It's not necessary to insert device argment to devargs_list during
> > > > bus scan, but this happens when we try to attach a device on
> > > > secondary process. The patch fix the issue.
> > > >
> > > > Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > ---
> > > >  drivers/bus/vdev/vdev.c | 11 +++++++----
> > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > > > index
> > > > 688e31c21..818a2bfc2 100644
> > > > --- a/drivers/bus/vdev/vdev.c
> > > > +++ b/drivers/bus/vdev/vdev.c
> > > > @@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char
> > > > *args) }
> > > >
> > > >  static int
> > > > -insert_vdev(const char *name, const char *args, struct
> > > > rte_vdev_device **p_dev)
> > > > +insert_vdev(const char *name, const char *args,
> > > > +		struct rte_vdev_device **p_dev,
> > > > +		bool init)
> > >
> > > Why is vdev the only bus not using hotplug API itself and
> > > reimplementing it on its own?
> >
> > I don't know the history,
> > but replace insert_vdev with hotplug API will not solve all the
> > problem (see my below comments)
> >
> > >
> > > It should not have to insert devargs at all, not even in the primary
> > > process. If it called rte_dev_probe(), this would normally already
> > > be properly handled I think.
> >
> > Currently insert_vdev is shared by two scenarios 1. rte_vdev_init,
> > which is called by application to attach a new device, I agree it's better to
> have rte_dev_probe here so, no need to have insert_vdev here.
> > 2. during secondary's vdev->scan when it receive a SCAN_ONE event from
> > primary, it should not call rte_devargs_insert, The patch is going to
> > fix the issue on secondary scenario and we can do the cleanup for
> > first issue in a separate patch
> 
> In 2., won't dev_probe() check for secondary process context and in this case,
> send the request to primary, which will see that the device is already probed,
> which would thus fix your issue?

No, It's not the case that we issue a device attaching from secondary, but the case that we try to sync with primary for a device already be attached 
So we are in the context of dev->scan, we should not do dev_probe in dev->scan, since dev_probe will call dev->scan again, then it go dead loop.
> 
> My take on it is that it seems to be fixing both your issue and cleaning up
> history.
> 
> >
> >
> >
> > >
> > > >  {
> > > >  	struct rte_vdev_device *dev;
> > > >  	struct rte_devargs *devargs;
> > > > @@ -237,7 +239,8 @@ insert_vdev(const char *name, const char
> > > > *args,
> > > struct rte_vdev_device **p_dev)
> > > >  	}
> > > >
> > > >  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
> > > > -	rte_devargs_insert(devargs);
> > > > +	if (init)
> > > > +		rte_devargs_insert(devargs);
> > > >
> > > >  	if (p_dev)
> > > >  		*p_dev = dev;
> > > > @@ -257,7 +260,7 @@ rte_vdev_init(const char *name, const char
> *args)
> > > >  	int ret;
> > > >
> > > >  	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> > > > -	ret = insert_vdev(name, args, &dev);
> > > > +	ret = insert_vdev(name, args, &dev, true);
> > > >  	if (ret == 0) {
> > > >  		ret = vdev_probe_all_drivers(dev);
> > > >  		if (ret) {
> > > > @@ -383,7 +386,7 @@ vdev_action(const struct rte_mp_msg
> *mp_msg,
> > > const void *peer)
> > > >  		break;
> > > >  	case VDEV_SCAN_ONE:
> > > >  		VDEV_LOG(INFO, "receive vdev, %s", in->name);
> > > > -		ret = insert_vdev(in->name, NULL, NULL);
> > > > +		ret = insert_vdev(in->name, NULL, NULL, false);
> > > >  		if (ret == -EEXIST)
> > > >  			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
> > > >  		else if (ret < 0)
> > > > --
> > > > 2.13.6
> > > >
> > >
> > > --
> > > Gaëtan Rivet
> > > 6WIND
> 
> --
> Gaëtan Rivet
> 6WIND

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
  2018-10-25 15:18       ` Zhang, Qi Z
@ 2018-10-25 15:26         ` Gaëtan Rivet
  0 siblings, 0 replies; 7+ messages in thread
From: Gaëtan Rivet @ 2018-10-25 15:26 UTC (permalink / raw)
  To: Zhang, Qi Z; +Cc: thomas, dev, stable

On Thu, Oct 25, 2018 at 03:18:20PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Thursday, October 25, 2018 10:03 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus scan
> > 
> > On Thu, Oct 25, 2018 at 02:56:55PM +0000, Zhang, Qi Z wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > Sent: Thursday, October 25, 2018 4:51 AM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> > > > Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus
> > > > scan
> > > >
> > > > On Thu, Oct 25, 2018 at 11:30:36AM +0800, Qi Zhang wrote:
> > > > > It's not necessary to insert device argment to devargs_list during
> > > > > bus scan, but this happens when we try to attach a device on
> > > > > secondary process. The patch fix the issue.
> > > > >
> > > > > Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > ---
> > > > >  drivers/bus/vdev/vdev.c | 11 +++++++----
> > > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > > > > index
> > > > > 688e31c21..818a2bfc2 100644
> > > > > --- a/drivers/bus/vdev/vdev.c
> > > > > +++ b/drivers/bus/vdev/vdev.c
> > > > > @@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char
> > > > > *args) }
> > > > >
> > > > >  static int
> > > > > -insert_vdev(const char *name, const char *args, struct
> > > > > rte_vdev_device **p_dev)
> > > > > +insert_vdev(const char *name, const char *args,
> > > > > +		struct rte_vdev_device **p_dev,
> > > > > +		bool init)
> > > >
> > > > Why is vdev the only bus not using hotplug API itself and
> > > > reimplementing it on its own?
> > >
> > > I don't know the history,
> > > but replace insert_vdev with hotplug API will not solve all the
> > > problem (see my below comments)
> > >
> > > >
> > > > It should not have to insert devargs at all, not even in the primary
> > > > process. If it called rte_dev_probe(), this would normally already
> > > > be properly handled I think.
> > >
> > > Currently insert_vdev is shared by two scenarios 1. rte_vdev_init,
> > > which is called by application to attach a new device, I agree it's better to
> > have rte_dev_probe here so, no need to have insert_vdev here.
> > > 2. during secondary's vdev->scan when it receive a SCAN_ONE event from
> > > primary, it should not call rte_devargs_insert, The patch is going to
> > > fix the issue on secondary scenario and we can do the cleanup for
> > > first issue in a separate patch
> > 
> > In 2., won't dev_probe() check for secondary process context and in this case,
> > send the request to primary, which will see that the device is already probed,
> > which would thus fix your issue?
> 
> No, It's not the case that we issue a device attaching from secondary, but the case that we try to sync with primary for a device already be attached 
> So we are in the context of dev->scan, we should not do dev_probe in dev->scan, since dev_probe will call dev->scan again, then it go dead loop.
> > 
> > My take on it is that it seems to be fixing both your issue and cleaning up
> > history.
> > 

Ok!
As long as you are confident this is the simplest way to write this, while
taking into consideration the future rework of vdev_init(), all good :)

-- 
Gaëtan Rivet
6WIND

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
  2018-10-25  3:30 [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan Qi Zhang
  2018-10-25  9:51 ` Gaëtan Rivet
@ 2018-10-28 17:32 ` Thomas Monjalon
  1 sibling, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2018-10-28 17:32 UTC (permalink / raw)
  To: Qi Zhang; +Cc: dev, gaetan.rivet, stable

25/10/2018 05:30, Qi Zhang:
> It's not necessary to insert device argment to devargs_list
> during bus scan, but this happens when we try to attach a
> device on secondary process. The patch fix the issue.
> 
> Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-10-28 17:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25  3:30 [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan Qi Zhang
2018-10-25  9:51 ` Gaëtan Rivet
2018-10-25 14:56   ` Zhang, Qi Z
2018-10-25 15:03     ` Gaëtan Rivet
2018-10-25 15:18       ` Zhang, Qi Z
2018-10-25 15:26         ` Gaëtan Rivet
2018-10-28 17:32 ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).