* [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case
@ 2021-10-22 13:17 Andrew Rybchenko
2021-10-29 9:42 ` Maxime Coquelin
2021-10-29 10:33 ` Maxime Coquelin
0 siblings, 2 replies; 4+ messages in thread
From: Andrew Rybchenko @ 2021-10-22 13:17 UTC (permalink / raw)
To: Maxime Coquelin, Chenbo Xia, Ivan Dyukov; +Cc: dev, Ivan Ilchenko, stable
From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
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 <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
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;
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);
--
2.30.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case
2021-10-22 13:17 [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case Andrew Rybchenko
@ 2021-10-29 9:42 ` Maxime Coquelin
2021-10-29 10:20 ` Andrew Rybchenko
2021-10-29 10:33 ` Maxime Coquelin
1 sibling, 1 reply; 4+ messages in thread
From: Maxime Coquelin @ 2021-10-29 9:42 UTC (permalink / raw)
To: Andrew Rybchenko, Chenbo Xia, Ivan Dyukov; +Cc: dev, Ivan Ilchenko, stable
On 10/22/21 15:17, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>
> 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 <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> 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.
Other than that:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case
2021-10-29 9:42 ` Maxime Coquelin
@ 2021-10-29 10:20 ` Andrew Rybchenko
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Rybchenko @ 2021-10-29 10:20 UTC (permalink / raw)
To: Maxime Coquelin, Chenbo Xia, Ivan Dyukov; +Cc: dev, Ivan Ilchenko, stable
On 10/29/21 12:42 PM, Maxime Coquelin wrote:
>
>
> On 10/22/21 15:17, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> 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 <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>> 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 <maxime.coquelin@redhat.com>
>
> Thanks,
> Maxime
Many thanks,
Andrew.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case
2021-10-22 13:17 [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case Andrew Rybchenko
2021-10-29 9:42 ` Maxime Coquelin
@ 2021-10-29 10:33 ` Maxime Coquelin
1 sibling, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2021-10-29 10:33 UTC (permalink / raw)
To: Andrew Rybchenko, Chenbo Xia, Ivan Dyukov; +Cc: dev, Ivan Ilchenko, stable
On 10/22/21 15:17, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>
> 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 <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> drivers/net/virtio/virtio.h | 5 ++++
> drivers/net/virtio/virtio_ethdev.c | 47 +++++++++++++++++++++---------
> 2 files changed, 39 insertions(+), 13 deletions(-)
>
Applied to dpdk-next-virtio/main with suggested change.
Thanks,
Maxime
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-29 10:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 13:17 [dpdk-stable] [PATCH] net/virtio: fix link update in speed feature case Andrew Rybchenko
2021-10-29 9:42 ` Maxime Coquelin
2021-10-29 10:20 ` Andrew Rybchenko
2021-10-29 10:33 ` Maxime Coquelin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).