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

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

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