From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Boccassi Subject: Re: [PATCH 1/2] net/virtio: do not re-enter clean up routines Date: Fri, 02 Nov 2018 11:14:18 +0000 Message-ID: <1541157258.4849.17.camel@debian.org> References: <1500332723-10812-1-git-send-email-ciwillia@brocade.com> <1500332723-10812-2-git-send-email-ciwillia@brocade.com> <1541083532.4849.8.camel@debian.org> <4cb5c58b-b0e8-b00c-b302-70f4cb6cb2ee@redhat.com> <54c2ea74-17ec-ee3a-4b32-a55599f43413@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: ferruh.yigit@intel.com, 3chas3@gmail.com To: Maxime Coquelin , dev@dpdk.org Return-path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id 4CA8D5B32 for ; Fri, 2 Nov 2018 12:14:22 +0100 (CET) Received: by mail-wr1-f66.google.com with SMTP id y16so1596692wrw.3 for ; Fri, 02 Nov 2018 04:14:22 -0700 (PDT) In-Reply-To: <54c2ea74-17ec-ee3a-4b32-a55599f43413@redhat.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, 2018-11-02 at 12:03 +0100, Maxime Coquelin wrote: >=20 > On 11/2/18 10:57 AM, Maxime Coquelin wrote: > >=20 > >=20 > > On 11/1/18 3:45 PM, Luca Boccassi wrote: > > > On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote: > > > > .dev_uninit calls .dev_stop and .dev_close.=C2=A0=C2=A0The work tha= t is > > > > done in > > > > those routines doesn't need repeated.=C2=A0=C2=A0Use started and op= ened > > > > to > > > > track > > > > the adapter's status. > > > >=20 > > > > Signed-off-by: Chas Williams > > > > --- > > > > =C2=A0=C2=A0drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++--- > > > > =C2=A0=C2=A0drivers/net/virtio/virtio_pci.h=C2=A0=C2=A0=C2=A0=C2=A0= |=C2=A0=C2=A04 +++- > > > > =C2=A0=C2=A02 files changed, 15 insertions(+), 4 deletions(-) > > > >=20 > > > > diff --git a/drivers/net/virtio/virtio_ethdev.c > > > > b/drivers/net/virtio/virtio_ethdev.c > > > > index 00a3122..eff0545 100644 > > > > --- a/drivers/net/virtio/virtio_ethdev.c > > > > +++ b/drivers/net/virtio/virtio_ethdev.c > > > > @@ -609,6 +609,10 @@ virtio_dev_close(struct rte_eth_dev *dev) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 PMD_INIT_LOG(DEBUG, "virtio_dev_clos= e"); > > > > +=C2=A0=C2=A0=C2=A0 if (!hw->opened) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > > > +=C2=A0=C2=A0=C2=A0 hw->opened =3D false; > > > > + > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* reset the NIC */ > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (dev->data->dev_flags & RTE_ETH_D= EV_INTR_LSC) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 VTPCI_OPS(hw= )->set_config_irq(hw, > > > > VIRTIO_MSI_NO_VECTOR); > > > > @@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev > > > > *dev) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 return -EBUSY; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > > > +=C2=A0=C2=A0=C2=A0 hw->opened =3D true; > > > > + > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; > > > > =C2=A0=C2=A0} > > > > @@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 VIRTQUEUE_DU= MP(txvq->vq); > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > > > -=C2=A0=C2=A0=C2=A0 hw->started =3D 1; > > > > +=C2=A0=C2=A0=C2=A0 hw->started =3D true; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Initialize Link state */ > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 virtio_dev_link_update(dev, 0); > > > > @@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev > > > > *dev) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 PMD_INIT_LOG(DEBUG, "stop"); > > > > +=C2=A0=C2=A0=C2=A0 if (!hw->started) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > > > +=C2=A0=C2=A0=C2=A0 hw->started =3D false; > > > > + > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (intr_conf->lsc || intr_conf->rxq= ) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rte_intr_dis= able(dev->intr_handle); > > > > -=C2=A0=C2=A0=C2=A0 hw->started =3D 0; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 memset(&link, 0, sizeof(link)); > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 virtio_dev_atomic_write_link_status(= dev, &link); > > > > =C2=A0=C2=A0} > > > > @@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev > > > > *dev, > > > > __rte_unused int wait_to_complet > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 link.link_duplex =3D ETH_LINK_FULL_D= UPLEX; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 link.link_speed=C2=A0=C2=A0=3D SPEED= _10G; > > > > -=C2=A0=C2=A0=C2=A0 if (hw->started =3D=3D 0) { > > > > +=C2=A0=C2=A0=C2=A0 if (!hw->started) { > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 link.link_st= atus =3D ETH_LINK_DOWN; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else if (vtpci_with_feature(hw, VI= RTIO_NET_F_STATUS)) { > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 PMD_INIT_LOG= (DEBUG, "Get link status from hw"); > > > > diff --git a/drivers/net/virtio/virtio_pci.h > > > > b/drivers/net/virtio/virtio_pci.h > > > > index 18caebd..65bad2d 100644 > > > > --- a/drivers/net/virtio/virtio_pci.h > > > > +++ b/drivers/net/virtio/virtio_pci.h > > > > @@ -35,6 +35,7 @@ > > > > =C2=A0=C2=A0#define _VIRTIO_PCI_H_ > > > > =C2=A0=C2=A0#include > > > > +#include > > > > =C2=A0=C2=A0#include > > > > =C2=A0=C2=A0#include > > > > @@ -253,7 +254,7 @@ struct virtio_hw { > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint64_t=C2=A0=C2=A0=C2=A0=C2=A0req_= guest_features; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint64_t=C2=A0=C2=A0=C2=A0=C2=A0gues= t_features; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint32_t=C2=A0=C2=A0=C2=A0=C2=A0max_= queue_pairs; > > > > -=C2=A0=C2=A0=C2=A0 uint16_t=C2=A0=C2=A0=C2=A0=C2=A0started; > > > > +=C2=A0=C2=A0=C2=A0 bool=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0started; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint16_t=C2=A0=C2=A0=C2=A0 max_mtu; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint16_t=C2=A0=C2=A0=C2=A0=C2=A0vtne= t_hdr_size; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint8_t=C2=A0=C2=A0=C2=A0 =C2=A0=C2= =A0=C2=A0=C2=A0vlan_strip; > > > > @@ -268,6 +269,7 @@ struct virtio_hw { > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct virtio_pci_common_cfg *common= _cfg; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct virtio_net_config *dev_cfg; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 void=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0= =C2=A0=C2=A0*virtio_user_dev; > > > > +=C2=A0=C2=A0=C2=A0 bool=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0opened; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct virtqueue **vqs; > > > > =C2=A0=C2=A0}; > > >=20 > > > Hi Maxime, > > >=20 > > > Any chance this could be reviewed and taken care of, please? It's > > > been > > > forgotten in the queue for a year, but we still use it. > > > I have sent 2/2 separately so ignore that (I didn't notice this > > > series > > > was still pending review). > > >=20 > > > Thanks! > > >=20 > >=20 > >=20 > > Reviewed-by: Maxime Coquelin > >=20 > > I'll certainly have to rebase it, but it should be trivial. > > And as discussed on IRC, it fixes a real issue, so will have to be > > backported to stable. I'll take care of it. > >=20 > > Regards, > > Maxime >=20 > Applied to dpdk-next-virtio/master >=20 > If you have some time, please have a look at the branch to ensure the > rebase is correct. >=20 > Thanks, > Maxime Looks good to me, thank you very much! --=20 Kind regards, Luca Boccassi