From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 4704C1B114 for ; Fri, 2 Nov 2018 10:57:33 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 72D97356E5; Fri, 2 Nov 2018 09:57:32 +0000 (UTC) Received: from [10.36.112.50] (ovpn-112-50.ams2.redhat.com [10.36.112.50]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CED48194AA; Fri, 2 Nov 2018 09:57:30 +0000 (UTC) To: Luca Boccassi , dev@dpdk.org Cc: ferruh.yigit@intel.com, 3chas3@gmail.com 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> From: Maxime Coquelin Message-ID: <4cb5c58b-b0e8-b00c-b302-70f4cb6cb2ee@redhat.com> Date: Fri, 2 Nov 2018 10:57:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <1541083532.4849.8.camel@debian.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 02 Nov 2018 09:57:32 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Nov 2018 09:57:33 -0000 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.  The work that is done in >> those routines doesn't need repeated.  Use started and opened to >> track >> the adapter's status. >> >> Signed-off-by: Chas Williams >> --- >>  drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++--- >>  drivers/net/virtio/virtio_pci.h    |  4 +++- >>  2 files changed, 15 insertions(+), 4 deletions(-) >> >> 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) >> >>   PMD_INIT_LOG(DEBUG, "virtio_dev_close"); >> >> + if (!hw->opened) >> + return; >> + hw->opened = false; >> + >>   /* reset the NIC */ >>   if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) >>   VTPCI_OPS(hw)->set_config_irq(hw, >> VIRTIO_MSI_NO_VECTOR); >> @@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev *dev) >>   return -EBUSY; >>   } >> >> + hw->opened = true; >> + >>   return 0; >>  } >> >> @@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev) >>   VIRTQUEUE_DUMP(txvq->vq); >>   } >> >> - hw->started = 1; >> + hw->started = true; >> >>   /* Initialize Link state */ >>   virtio_dev_link_update(dev, 0); >> @@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev *dev) >> >>   PMD_INIT_LOG(DEBUG, "stop"); >> >> + if (!hw->started) >> + return; >> + hw->started = false; >> + >>   if (intr_conf->lsc || intr_conf->rxq) >>   rte_intr_disable(dev->intr_handle); >> >> - hw->started = 0; >>   memset(&link, 0, sizeof(link)); >>   virtio_dev_atomic_write_link_status(dev, &link); >>  } >> @@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, >> __rte_unused int wait_to_complet >>   link.link_duplex = ETH_LINK_FULL_DUPLEX; >>   link.link_speed  = SPEED_10G; >> >> - if (hw->started == 0) { >> + if (!hw->started) { >>   link.link_status = ETH_LINK_DOWN; >>   } else if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) { >>   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 @@ >>  #define _VIRTIO_PCI_H_ >> >>  #include >> +#include >> >>  #include >>  #include >> @@ -253,7 +254,7 @@ struct virtio_hw { >>   uint64_t    req_guest_features; >>   uint64_t    guest_features; >>   uint32_t    max_queue_pairs; >> - uint16_t    started; >> + bool        started; >>   uint16_t max_mtu; >>   uint16_t    vtnet_hdr_size; >>   uint8_t     vlan_strip; >> @@ -268,6 +269,7 @@ struct virtio_hw { >>   struct virtio_pci_common_cfg *common_cfg; >>   struct virtio_net_config *dev_cfg; >>   void     *virtio_user_dev; >> + bool        opened; >> >>   struct virtqueue **vqs; >>  }; > > Hi Maxime, > > 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). > > Thanks! > Reviewed-by: Maxime Coquelin 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. Regards, Maxime