Re: [PATCH 2/5] net/virtio-user: add rxq interrupt mode support

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, March 17, 2017 2:48 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; david.marchand@6wind.com
> Subject: Re: [PATCH 2/5] net/virtio-user: add rxq interrupt mode support
> 
> On Fri, Mar 03, 2017 at 05:56:40PM +0000, Jianfeng Tan wrote:
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> I don't see a single word to explain how this patch works :/

Sorry, will add that in next version.

> 
> > ---
> >  drivers/net/virtio/virtio_ethdev.c               | 17 ++++++++++++++--
> >  drivers/net/virtio/virtio_user/virtio_user_dev.c | 25
> +++++++++++++++++++++++-
> >  drivers/net/virtio/virtio_user/virtio_user_dev.h |  2 +-
> >  drivers/net/virtio/virtio_user_ethdev.c          | 12 +++++++++++-
> >  4 files changed, 51 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> > index 4dc03b9..5d80d1a 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1264,6 +1264,10 @@ virtio_configure_intr(struct rte_eth_dev *dev)
> >  {
> >  	struct virtio_hw *hw = dev->data->dev_private;
> >
> > +
> > +#ifdef RTE_VIRTIO_USER
> > +        if (!hw->virtio_user_dev) {
> > +#endif
> 
> No need to put the #ifdef block here. virtio_user_dev is defined even
> when RTE_VIRTIO_USER is not configured.

Correct. I'll remove that.

> 
> ...
> 
> > +	vtpci_reinit_complete(hw);
> > +
> >  	if (eth_dev->data->dev_conf.intr_conf.rxq) {
> >  		if (virtio_configure_intr(eth_dev) < 0) {
> >  			PMD_INIT_LOG(ERR, "failed to configure interrupt");
> > @@ -1416,8 +1431,6 @@ virtio_init_device(struct rte_eth_dev *eth_dev,
> uint64_t req_features)
> >  		}
> >  	}
> >
> > -	vtpci_reinit_complete(hw);
> 
> 
> Hmm, why ...? Such stealthy change definitely need an explanation.
> And it's more likely it needs a single patch.

I should have described it more. The reason of above change is to make sure intr_handle is filled in virtio_user_start_device(). But reconsidering this, this could have effect on virtio pci device as it requires interrupts are set up before setting DRIVER_OK. So how about:

1. configure intr for PCI devices;
2. vtpci_reinit_complete();
3. configure intr for vdev;

Thanks,
Jianfeng