From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>, dev@dpdk.org
Cc: Ferruh Yigit <ferruh.yigit@amd.com>,
Ivan Malov <ivan.malov@arknetworks.am>,
Andy Moreton <amoreton@xilinx.com>
Subject: Re: [PATCH 3/3] net/sfc: support FEC feature
Date: Fri, 2 Jun 2023 12:09:58 +0300 [thread overview]
Message-ID: <075dbd95-940c-869b-fa15-10050fd07faa@oktetlabs.ru> (raw)
In-Reply-To: <20230601222349.28965-4-denis.pryazhennikov@arknetworks.am>
On 6/2/23 01:23, Denis Pryazhennikov wrote:
> Support ethdev methods to query and set FEC information.
> Limitations: ignoring rte_eth_fec_get_capability() results
> can lead to NOFEC if the device is not strated.
>
> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
> Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
> doc/guides/nics/features/sfc.ini | 1 +
> drivers/net/sfc/sfc.h | 2 +
> drivers/net/sfc/sfc_ethdev.c | 337 +++++++++++++++++++++++++++++++
> drivers/net/sfc/sfc_port.c | 24 ++-
> 4 files changed, 353 insertions(+), 11 deletions(-)
Let's advertise it in release notes.
>
> diff --git a/doc/guides/nics/features/sfc.ini b/doc/guides/nics/features/sfc.ini
> index f5ac644278ae..e0b9bfb7f7bf 100644
> --- a/doc/guides/nics/features/sfc.ini
> +++ b/doc/guides/nics/features/sfc.ini
> @@ -24,6 +24,7 @@ RSS reta update = Y
> SR-IOV = Y
> Flow control = Y
> VLAN offload = P
> +FEC = Y
> L3 checksum offload = Y
> L4 checksum offload = Y
> Inner L3 checksum = Y
> diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
> index 730d054aea74..e42abe42cb8a 100644
> --- a/drivers/net/sfc/sfc.h
> +++ b/drivers/net/sfc/sfc.h
> @@ -68,6 +68,8 @@ struct sfc_port {
>
> uint32_t phy_adv_cap_mask;
> uint32_t phy_adv_cap;
> + uint32_t fec_cfg;
> + bool fec_auto;
>
> unsigned int flow_ctrl;
> boolean_t flow_ctrl_autoneg;
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index 6d41eb000345..05fc70541b10 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -2343,6 +2343,340 @@ sfc_rx_metadata_negotiate(struct rte_eth_dev *dev, uint64_t *features)
> return 0;
> }
>
> +static unsigned int
> +sfc_fec_get_capa_speed_to_fec(uint32_t supported_caps,
> + struct rte_eth_fec_capa *speed_fec_capa)
> +{
> + int num = 0;
> +
> + if (supported_caps & (1u << EFX_PHY_CAP_10000FDX)) {
> + if (speed_fec_capa) {
Compare vs NULL as DPDK coding style says. Here and in similar cases below.
> + speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_10G;
> + speed_fec_capa[num].capa =
> + RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
I'd like to see a comment which explicilty says that supported
FEC depends on supported link speed only. So, there is no
special capability bits.
> + }
> + num++;
> + }
> + if (supported_caps & (1u << EFX_PHY_CAP_25000FDX)) {
> + if (speed_fec_capa) {
> + speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;
> + speed_fec_capa[num].capa =
> + RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> + }
> + num++;
> + }
> + if (supported_caps & (1u << EFX_PHY_CAP_40000FDX)) {
> + if (speed_fec_capa) {
> + speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_40G;
> + speed_fec_capa[num].capa =
> + RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> + }
> + num++;
> + }
> + if (supported_caps & (1u << EFX_PHY_CAP_50000FDX)) {
> + if (speed_fec_capa) {
> + speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_50G;
> + speed_fec_capa[num].capa =
> + RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> + }
> + num++;
> + }
> + if (supported_caps & (1u << EFX_PHY_CAP_100000FDX)) {
> + if (speed_fec_capa) {
> + speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100G;
> + speed_fec_capa[num].capa =
> + RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> + }
> + num++;
> + }
> +
> + return num;
> +}
> +
> +static int
> +sfc_fec_get_capability(struct rte_eth_dev *dev,
> + struct rte_eth_fec_capa *speed_fec_capa,
> + unsigned int num)
> +{
> + struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> + unsigned int num_entries;
> + uint32_t supported_caps;
> +
> + sfc_adapter_lock(sa);
> +
> + efx_phy_adv_cap_get(sa->nic, EFX_PHY_CAP_PERM, &supported_caps);
> +
> + num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps, NULL);
> + if (!speed_fec_capa || num < num_entries)
Compare vs NULL as DPDK coding style says.
> + goto adapter_unlock;
> +
> + num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps,
> + speed_fec_capa);
> +
> +adapter_unlock:
> + sfc_adapter_unlock(sa);
> +
> + return num_entries;
> +}
> +
> +static uint32_t
> +sfc_efx_caps_to_fec(uint32_t caps, bool is_25g)
> +{
> + bool rs_req = caps & EFX_PHY_CAP_FEC_BIT(RS_FEC_REQUESTED);
> + bool rs = caps & EFX_PHY_CAP_FEC_BIT(RS_FEC);
> + uint32_t fec_capa = 0;
> + bool baser_req;
> + bool baser;
> +
> + if (is_25g) {
> + baser = caps & EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC);
> + baser_req = caps & EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC_REQUESTED);
> + } else {
> + baser = caps & EFX_PHY_CAP_FEC_BIT(BASER_FEC);
> + baser_req = caps & EFX_PHY_CAP_FEC_BIT(BASER_FEC_REQUESTED);
> + }
> +
> + if (!baser && !rs)
> + return RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +
> + if (rs_req)
> + fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +
> + if (baser_req)
> + fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +
> +
two many empty lines, just one is sufficient
> + return fec_capa;
> +}
> +
> +static int
> +sfc_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> + struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> + struct sfc_port *port = &sa->port;
> + struct rte_eth_link current_link;
> + efx_phy_fec_type_t active_fec;
> + bool is_25g = false;
> + int rc = 0;
> +
> + sfc_adapter_lock(sa);
> +
> + sfc_dev_get_rte_link(dev, 1, ¤t_link);
> +
> + if (current_link.link_status == RTE_ETH_LINK_DOWN) {
> + uint32_t speed = current_link.link_speed;
> +
> + if (port->fec_auto) {
> + *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
> + goto adapter_unlock;
> + }
> +
> + is_25g = (speed == RTE_ETH_SPEED_NUM_25G ||
> + speed == RTE_ETH_SPEED_NUM_50G);
> +
> + *fec_capa = sfc_efx_caps_to_fec(port->fec_cfg, is_25g);
> +
> + goto adapter_unlock;
> + }
> +
> + rc = efx_phy_fec_type_get(sa->nic, &active_fec);
> + if (rc != 0)
> + goto adapter_unlock;
> +
> + switch (active_fec) {
> + case EFX_PHY_FEC_NONE:
> + *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> + break;
> + case EFX_PHY_FEC_BASER:
> + *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> + break;
> + case EFX_PHY_FEC_RS:
> + *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> + break;
> + default:
Don't we need an error message here and return failure?
> + break;
> + }
> +
> +adapter_unlock:
> + sfc_adapter_unlock(sa);
> +
> + return rc;
> +}
> +
> +static int
> +sfc_fec_capa_check(struct rte_eth_dev *dev, uint32_t fec_capa,
> + uint32_t supported_caps)
> +{
> + struct rte_eth_fec_capa *speed_fec_capa;
> + struct rte_eth_link current_link;
> + bool is_supported = false;
> + unsigned int num_entries;
> + bool auto_fec = false;
> + unsigned int i;
> +
> + struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> +
> + if (sa->state != SFC_ETHDEV_STARTED)
> + return 0;
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO)) {
> + auto_fec = true;
> + fec_capa &= ~RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
> + }
> +
> + /*
> + * If only the AUTO bit is set, the decision on which FEC
> + * mode to use will be made by HW/FW or driver.
> + */
> + if (auto_fec && fec_capa == 0)
> + return 0;
> +
> + sfc_dev_get_rte_link(dev, 1, ¤t_link);
> +
> + num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps, NULL);
> + if (num_entries == 0)
> + return -ENOTSUP;
> +
> + speed_fec_capa = rte_calloc("fec_capa", num_entries,
> + sizeof(*speed_fec_capa), 0);
> + num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps,
> + speed_fec_capa);
> +
> + for (i = 0; i < num_entries; i++) {
> + if (speed_fec_capa[i].speed == current_link.link_speed) {
> + if ((fec_capa & speed_fec_capa[i].capa) != 0)
> + is_supported = true;
> +
> + break;
> + }
> + }
> +
> + rte_free(speed_fec_capa);
> +
> + if (is_supported)
> + return 0;
> +
> + return -ENOTSUP;
> +}
> +
> +static int
> +sfc_fec_capa_to_efx(uint32_t supported_caps, uint32_t fec_capa,
> + uint32_t *efx_fec_caps)
> +{
> + bool fec_is_set = false;
> + bool auto_fec = false;
> + bool nofec = false;
> + uint32_t ret = 0;
> +
> + if (efx_fec_caps == NULL)
> + return -EINVAL;
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO))
> + auto_fec = true;
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC))
> + nofec = true;
> +
> + if (fec_capa == RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO)) {
> + ret |= (EFX_PHY_CAP_FEC_BIT(BASER_FEC) |
> + EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC) |
> + EFX_PHY_CAP_FEC_BIT(RS_FEC)) & supported_caps;
> + goto done;
> + }
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS)) {
> + fec_is_set = true;
> +
> + if (supported_caps & EFX_PHY_CAP_FEC_BIT(RS_FEC)) {
> + ret |= EFX_PHY_CAP_FEC_BIT(RS_FEC) |
> + EFX_PHY_CAP_FEC_BIT(RS_FEC_REQUESTED);
> + }
> + }
> + if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER)) {
> + if (!auto_fec && fec_is_set)
> + return -EINVAL;
> +
> + if (supported_caps & EFX_PHY_CAP_FEC_BIT(BASER_FEC)) {
> + ret |= EFX_PHY_CAP_FEC_BIT(BASER_FEC) |
> + EFX_PHY_CAP_FEC_BIT(BASER_FEC_REQUESTED);
> + }
> + if (supported_caps & EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC)) {
> + ret |= EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC) |
> + EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC_REQUESTED);
> + }
> + }
> +
> + if (ret == 0 && !nofec)
> + return -ENOTSUP;
> +
> +done:
> + *efx_fec_caps = ret;
> + return 0;
> +}
> +
> +static int
> +sfc_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> + struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> + struct sfc_port *port = &sa->port;
> + uint32_t supported_caps;
> + uint32_t efx_fec_caps;
> + uint32_t updated_caps;
> + int rc = 0;
> +
> + sfc_adapter_lock(sa);
> +
> + efx_phy_adv_cap_get(sa->nic, EFX_PHY_CAP_PERM, &supported_caps);
> +
> + rc = sfc_fec_capa_check(dev, fec_capa, supported_caps);
> + if (rc != 0)
> + goto adapter_unlock;
> +
> + rc = sfc_fec_capa_to_efx(supported_caps, fec_capa, &efx_fec_caps);
> + if (rc != 0)
> + goto adapter_unlock;
> +
> + if (sa->state == SFC_ETHDEV_STARTED) {
> + efx_phy_adv_cap_get(sa->nic, EFX_PHY_CAP_CURRENT,
> + &updated_caps);
> + updated_caps = updated_caps & ~EFX_PHY_CAP_FEC_MASK;
> + updated_caps |= efx_fec_caps;
> +
> + rc = efx_phy_adv_cap_set(sa->nic, updated_caps);
> + if (rc != 0)
> + goto adapter_unlock;
> + }
> +
> + port->fec_cfg = efx_fec_caps;
> + /*
> + * There is no chance to recognize AUTO mode from the
> + * saved FEC capabilities as AUTO mode can have the same
> + * set of bits as any other mode from the EFX point of view.
> + * Save it in the proper variable.
> + */
> + if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO))
> + port->fec_auto = true;
> + else
> + port->fec_auto = false;
> +
> +adapter_unlock:
> + sfc_adapter_unlock(sa);
> +
> + return rc;
> +}
> +
> static const struct eth_dev_ops sfc_eth_dev_ops = {
> .dev_configure = sfc_dev_configure,
> .dev_start = sfc_dev_start,
> @@ -2392,6 +2726,9 @@ static const struct eth_dev_ops sfc_eth_dev_ops = {
> .pool_ops_supported = sfc_pool_ops_supported,
> .representor_info_get = sfc_representor_info_get,
> .rx_metadata_negotiate = sfc_rx_metadata_negotiate,
> + .fec_get_capability = sfc_fec_get_capability,
> + .fec_get = sfc_fec_get,
> + .fec_set = sfc_fec_set,
> };
>
> struct sfc_ethdev_init_data {
[snip]
next prev parent reply other threads:[~2023-06-02 9:10 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-01 22:23 [PATCH 0/3] " Denis Pryazhennikov
2023-06-01 22:23 ` [PATCH 1/3] net/sfc: split link update function Denis Pryazhennikov
2023-06-02 8:52 ` Andrew Rybchenko
2023-06-20 11:25 ` Ferruh Yigit
2023-06-20 11:43 ` Andrew Rybchenko
2023-06-01 22:23 ` [PATCH 2/3] common/sfc_efx/base: add FEC related macros Denis Pryazhennikov
2023-06-02 8:53 ` Andrew Rybchenko
2023-06-01 22:23 ` [PATCH 3/3] net/sfc: support FEC feature Denis Pryazhennikov
2023-06-02 9:09 ` Andrew Rybchenko [this message]
2023-06-15 8:38 ` [PATCH v2 0/3] " Denis Pryazhennikov
2023-06-15 8:38 ` [PATCH v2 1/3] net/sfc: split link update function Denis Pryazhennikov
2023-06-15 8:38 ` [PATCH v2 2/3] common/sfc_efx/base: add FEC related macros Denis Pryazhennikov
2023-06-20 11:34 ` Ferruh Yigit
2023-06-15 8:38 ` [PATCH v2 3/3] net/sfc: support FEC feature Denis Pryazhennikov
2023-06-19 9:39 ` Andrew Rybchenko
2023-06-20 11:33 ` Ferruh Yigit
2023-06-20 11:47 ` [PATCH v2 0/3] " Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=075dbd95-940c-869b-fa15-10050fd07faa@oktetlabs.ru \
--to=andrew.rybchenko@oktetlabs.ru \
--cc=amoreton@xilinx.com \
--cc=denis.pryazhennikov@arknetworks.am \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=ivan.malov@arknetworks.am \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).