From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C39C8A0032; Fri, 29 Oct 2021 12:20:11 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 495CB410E1; Fri, 29 Oct 2021 12:20:11 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 1B298410E0; Fri, 29 Oct 2021 12:20:10 +0200 (CEST) Received: from [192.168.1.71] (unknown [188.170.83.209]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 1BB5C7F4FE; Fri, 29 Oct 2021 13:20:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 1BB5C7F4FE DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1635502809; bh=GgDwxTgsXE+04263lK7/oavHY/2RFyc1zPto0ySlsvg=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=GncZNpDpKQVGDoCJmVTeXIWONCw1eshzogmK3Do3FvOqLsEgAboQOwRtqtPC/fh8T JmgxuNckGQ8kZYcoEhQSNjO2W80+9UBw3jUgoWkEoB7SAItNwV/zYSk9c/R4Kklftm 4dMZgiNwPNgLrKMsDhUREG9EfNHG1LdS4LGgBBUM= To: Maxime Coquelin , Chenbo Xia , Ivan Dyukov Cc: dev@dpdk.org, Ivan Ilchenko , stable@dpdk.org References: <20211022131755.932304-1-andrew.rybchenko@oktetlabs.ru> <6b1ce80a-25cd-4d6b-18a2-ccc3b51be7ad@redhat.com> From: Andrew Rybchenko Message-ID: <9f36c830-1e98-7c6a-feff-1c8c0b4b3902@oktetlabs.ru> Date: Fri, 29 Oct 2021 13:20:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <6b1ce80a-25cd-4d6b-18a2-ccc3b51be7ad@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix link update in speed feature case X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list 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 10/29/21 12:42 PM, Maxime Coquelin wrote: > > > On 10/22/21 15:17, Andrew Rybchenko wrote: >> From: Ivan Ilchenko >> >> Link update callback reports speed/duplex based on data >> filled on device initialization. This is wrong in case of >> VIRTIO_NET_F_SPEED_DUPLEX is negotiated since link could >> be down at this time. Fix this function to actually >> update the HW data in this case with respect to the fact >> that specifying speed via devarg is a highest priority. >> >> Fixes: 1357b4b36246 ("net/virtio: support Virtio link speed feature") >> Cc: stable@dpdk.org >> >> Signed-off-by: Ivan Ilchenko >> Signed-off-by: Andrew Rybchenko >> --- >>   drivers/net/virtio/virtio.h        |  5 ++++ >>   drivers/net/virtio/virtio_ethdev.c | 47 +++++++++++++++++++++--------- >>   2 files changed, 39 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio.h b/drivers/net/virtio/virtio.h >> index e78b2e429e..4fd1427375 100644 >> --- a/drivers/net/virtio/virtio.h >> +++ b/drivers/net/virtio/virtio.h >> @@ -178,6 +178,11 @@ struct virtio_hw { >>       uint16_t port_id; >>       uint8_t mac_addr[RTE_ETHER_ADDR_LEN]; >>       uint32_t speed;  /* link speed in MB */ >> +    /* >> +     * Speed is specified via 'speed' devarg or >> +     * negotiated via VIRTIO_NET_F_SPEED_DUPLEX >> +     */ >> +    bool get_speed_via_feat; > > For better packing of this struct, it may be better to place it just > before the speed field as it already has a 2 bytes hole above. > >>       uint8_t duplex; >>       uint8_t intr_lsc; >>       uint16_t max_mtu; >> diff --git a/drivers/net/virtio/virtio_ethdev.c >> b/drivers/net/virtio/virtio_ethdev.c >> index 4001368bc4..3d80f664b3 100644 >> --- a/drivers/net/virtio/virtio_ethdev.c >> +++ b/drivers/net/virtio/virtio_ethdev.c >> @@ -1777,6 +1777,32 @@ virtio_configure_intr(struct rte_eth_dev *dev) >>       return 0; >>   } >> + >> +static void >> +virtio_get_speed_duplex(struct rte_eth_dev *eth_dev, >> +            struct rte_eth_link *link) >> +{ >> +    struct virtio_hw *hw = eth_dev->data->dev_private; >> +    struct virtio_net_config *config; >> +    struct virtio_net_config local_config; >> + >> +    config = &local_config; >> +    virtio_read_dev_config(hw, >> +        offsetof(struct virtio_net_config, speed), >> +        &config->speed, sizeof(config->speed)); >> +    virtio_read_dev_config(hw, >> +        offsetof(struct virtio_net_config, duplex), >> +        &config->duplex, sizeof(config->duplex)); >> +    hw->speed = config->speed; >> +    hw->duplex = config->duplex; >> +    if (link != NULL) { >> +        link->link_duplex = hw->duplex; >> +        link->link_speed  = hw->speed; >> +    } >> +    PMD_INIT_LOG(DEBUG, "link speed = %d, duplex = %d", >> +             hw->speed, hw->duplex); >> +} >> + >>   #define DUPLEX_UNKNOWN   0xff >>   /* reset device and renegotiate features if needed */ >>   static int >> @@ -1830,19 +1856,10 @@ virtio_init_device(struct rte_eth_dev >> *eth_dev, uint64_t req_features) >>                hw->mac_addr[0], hw->mac_addr[1], hw->mac_addr[2], >>                hw->mac_addr[3], hw->mac_addr[4], hw->mac_addr[5]); >> -    if (hw->speed == ETH_SPEED_NUM_UNKNOWN) { >> -        if (virtio_with_feature(hw, VIRTIO_NET_F_SPEED_DUPLEX)) { >> -            config = &local_config; >> -            virtio_read_dev_config(hw, >> -                offsetof(struct virtio_net_config, speed), >> -                &config->speed, sizeof(config->speed)); >> -            virtio_read_dev_config(hw, >> -                offsetof(struct virtio_net_config, duplex), >> -                &config->duplex, sizeof(config->duplex)); >> -            hw->speed = config->speed; >> -            hw->duplex = config->duplex; >> -        } >> -    } >> +    hw->get_speed_via_feat = hw->speed == ETH_SPEED_NUM_UNKNOWN && >> +                 virtio_with_feature(hw, VIRTIO_NET_F_SPEED_DUPLEX); >> +    if (hw->get_speed_via_feat) >> +        virtio_get_speed_duplex(eth_dev, NULL); >>       if (hw->duplex == DUPLEX_UNKNOWN) >>           hw->duplex = ETH_LINK_FULL_DUPLEX; >>       PMD_INIT_LOG(DEBUG, "link speed = %d, duplex = %d", >> @@ -2554,11 +2571,15 @@ virtio_dev_link_update(struct rte_eth_dev >> *dev, __rte_unused int wait_to_complet >>                        dev->data->port_id); >>           } else { >>               link.link_status = ETH_LINK_UP; >> +            if (hw->get_speed_via_feat) >> +                virtio_get_speed_duplex(dev, &link); >>               PMD_INIT_LOG(DEBUG, "Port %d is up", >>                        dev->data->port_id); >>           } >>       } else { >>           link.link_status = ETH_LINK_UP; >> +        if (hw->get_speed_via_feat) >> +            virtio_get_speed_duplex(dev, &link); >>       } >>       return rte_eth_linkstatus_set(dev, &link); >> > > If you agree with the change, I can do it while applying. Yes, please, do. > > Other than that: > Reviewed-by: Maxime Coquelin > > Thanks, > Maxime Many thanks, Andrew.