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 3CBD15911 for ; Fri, 2 Nov 2018 12:03:35 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 946AB34CB; Fri, 2 Nov 2018 11:03:34 +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 6337160BE8; Fri, 2 Nov 2018 11:03:32 +0000 (UTC) From: Maxime Coquelin 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> <4cb5c58b-b0e8-b00c-b302-70f4cb6cb2ee@redhat.com> Message-ID: <54c2ea74-17ec-ee3a-4b32-a55599f43413@redhat.com> Date: Fri, 2 Nov 2018 12:03:30 +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: <4cb5c58b-b0e8-b00c-b302-70f4cb6cb2ee@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 02 Nov 2018 11:03:34 +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 11:03:35 -0000 On 11/2/18 10:57 AM, Maxime Coquelin wrote: > > > 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 Applied to dpdk-next-virtio/master If you have some time, please have a look at the branch to ensure the rebase is correct. Thanks, Maxime