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