Re: [PATCH v8 3/4] ethdev: redesign link speed config API

From: Matej Vido <vido@cesnet.cz>
To: Marc Sune <marcdevel@gmail.com>,
	dev@dpdk.org, Wenzhuo Lu <wenzhuo.lu@intel.com>,
	Helin Zhang <helin.zhang@intel.com>,
	Harish Patil <harish.patil@qlogic.com>,
	Jing Chen <jing.d.chen@intel.com>
Subject: Re: [PATCH v8 3/4] ethdev: redesign link speed config API
Date: Tue, 16 Feb 2016 11:28:00 +0100	[thread overview]
Message-ID: <56C2F9B0.5080204@cesnet.cz> (raw)
In-Reply-To: <1455488259-1000-4-git-send-email-marcdevel@gmail.com>

Dňa 14.02.2016 o 23:17 Marc Sune napísal(a):
> This patch redesigns the API to set the link speed/s configure
> for an ethernet port. Specifically:
>
> - it allows to define a set of advertised speeds for
>    auto-negociation.
> - it allows to disable link auto-negociation (single fixed speed).
> - default: auto-negociate all supported speeds.
>
> Other changes:
>
> * Added utility MACROs ETH_SPEED_NUM_XXX with the numeric
>    values of all supported link speeds, in Mbps.
> * Converted link_speed to uint64_t to accomodate 100G speeds
>    and beyond (bug).
> * Added autoneg flag in struct rte_eth_link to indicate if
>    link speed was a result of auto-negociation or was fixed
>    by configuration.
> * Added utility function to convert numeric speeds to bitmap
>    fields.
> * Added rte_eth_speed_to_bm_flag() to version map.
>
> Signed-off-by: Marc Sune <marcdevel@gmail.com>
> ---
>   app/test-pipeline/init.c                  |   2 +-
>   app/test-pmd/cmdline.c                    | 124 +++++++++++++++---------------
>   app/test-pmd/config.c                     |   4 +-
>   app/test/virtual_pmd.c                    |   4 +-
>   drivers/net/af_packet/rte_eth_af_packet.c |   5 +-
>   drivers/net/bnx2x/bnx2x_ethdev.c          |   8 +-
>   drivers/net/bonding/rte_eth_bond_8023ad.c |  14 ++--
>   drivers/net/cxgbe/base/t4_hw.c            |   8 +-
>   drivers/net/cxgbe/cxgbe_ethdev.c          |   2 +-
>   drivers/net/e1000/em_ethdev.c             | 116 ++++++++++++++--------------
>   drivers/net/e1000/igb_ethdev.c            | 111 +++++++++++++-------------
>   drivers/net/fm10k/fm10k_ethdev.c          |   8 +-
>   drivers/net/i40e/i40e_ethdev.c            |  73 +++++++++---------
>   drivers/net/i40e/i40e_ethdev_vf.c         |  11 +--
>   drivers/net/ixgbe/ixgbe_ethdev.c          |  78 ++++++++-----------
>   drivers/net/mlx4/mlx4.c                   |   6 +-
>   drivers/net/mpipe/mpipe_tilegx.c          |   6 +-
>   drivers/net/nfp/nfp_net.c                 |   4 +-
>   drivers/net/null/rte_eth_null.c           |   5 +-
>   drivers/net/pcap/rte_eth_pcap.c           |   9 ++-
>   drivers/net/ring/rte_eth_ring.c           |   5 +-
>   drivers/net/virtio/virtio_ethdev.c        |   2 +-
>   drivers/net/virtio/virtio_ethdev.h        |   2 -
>   drivers/net/vmxnet3/vmxnet3_ethdev.c      |   5 +-
>   drivers/net/xenvirt/rte_eth_xenvirt.c     |   5 +-
>   examples/ip_pipeline/config_parse.c       |   3 +-
>   lib/librte_ether/rte_ethdev.c             |  49 ++++++++++++
>   lib/librte_ether/rte_ethdev.h             | 113 +++++++++++++++++----------
>   lib/librte_ether/rte_ether_version.map    |   6 ++
>   29 files changed, 443 insertions(+), 345 deletions(-)
>
> [...]

Hi,

some drivers (at least: e1000, i40e, ixgbe, mpipe, nfp, virtio, vmxnet3) 
use rte_atomic64_cmpset() to read and write link state like:

static inline int
rte_em_dev_atomic_read_link_status(struct rte_eth_dev *dev,
                                 struct rte_eth_link *link)
{
         struct rte_eth_link *dst = link;
         struct rte_eth_link *src = &(dev->data->dev_link);

         if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
                                         *(uint64_t *)src) == 0)
                 return -1;

         return 0;
}

static inline int
rte_em_dev_atomic_write_link_status(struct rte_eth_dev *dev,
                                 struct rte_eth_link *link)
{
         struct rte_eth_link *dst = &(dev->data->dev_link);
         struct rte_eth_link *src = link;

         if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
                                         *(uint64_t *)src) == 0)
                 return -1;

         return 0;
}

When link_speed is changed to uint64_t, struct rte_eth_link exceeds 64 
bits. Shouldn't these functions be adapted in this patch series?

Szedata2 PMD will also use rte_atomic64_cmpset() after patch [1].
I can take care of this change in szedata2 PMD when patch [1] is 
accepted together with adjusting speeds in szedata2 PMD.

[1] http://dpdk.org/ml/archives/dev/2016-January/032281.html

Regards,
Matej