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