On Thu, Oct 25, 2018 at 03:18:20PM +0000, Zhang, Qi Z wrote: > > > > -----Original Message----- > > From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com] > > Sent: Thursday, October 25, 2018 10:03 AM > > To: Zhang, Qi Z <qi.z.zh...@intel.com> > > Cc: tho...@monjalon.net; dev@dpdk.org; sta...@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.ri...@6wind.com] > > > > Sent: Thursday, October 25, 2018 4:51 AM > > > > To: Zhang, Qi Z <qi.z.zh...@intel.com> > > > > Cc: tho...@monjalon.net; dev@dpdk.org; sta...@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: sta...@dpdk.org > > > > > > > > > > Signed-off-by: Qi Zhang <qi.z.zh...@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
