Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----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