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 C127642C10; Fri, 2 Jun 2023 11:10:03 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9983240ED8; Fri, 2 Jun 2023 11:10:03 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 84311406B8 for ; Fri, 2 Jun 2023 11:10:02 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id AEAB850; Fri, 2 Jun 2023 12:09:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru AEAB850 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1685696999; bh=CDCnTi/o3sOlkvvQK2Oc//3DD2jV2posLuwQ5YrJxXo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=bTE48245Mtz6gAq3eZzHhQoTJor/cDUkzJEwNwYT0sXIMcDbYCLc5lSQKk6OkmGpj PJkg9NjCgxGKKAf31WxWklcEsAoBjLsGv/oz8N2uXpCdV+QQtpRB46zjyvgXd3h5Le QPo5Wo+EUsJi87IUS9hy05ugIVzjJvmpTdbBS9I4= Message-ID: <075dbd95-940c-869b-fa15-10050fd07faa@oktetlabs.ru> Date: Fri, 2 Jun 2023 12:09:58 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH 3/3] net/sfc: support FEC feature Content-Language: en-US To: Denis Pryazhennikov , dev@dpdk.org Cc: Ferruh Yigit , Ivan Malov , Andy Moreton References: <20230601222349.28965-1-denis.pryazhennikov@arknetworks.am> <20230601222349.28965-4-denis.pryazhennikov@arknetworks.am> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20230601222349.28965-4-denis.pryazhennikov@arknetworks.am> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 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 > Reviewed-by: Ivan Malov > Reviewed-by: Andy Moreton > Reviewed-by: Ferruh Yigit > --- > 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]