From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2A94AA04B1; Thu, 24 Sep 2020 13:08:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D35BE1DDE5; Thu, 24 Sep 2020 13:08:01 +0200 (CEST) Received: from huawei.com (szxga07-in.huawei.com [45.249.212.35]) by dpdk.org (Postfix) with ESMTP id E53C21DDD0 for ; Thu, 24 Sep 2020 13:08:00 +0200 (CEST) Received: from DGGEMS414-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id AA0C5BC1D3279027404B; Thu, 24 Sep 2020 19:07:59 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS414-HUB.china.huawei.com (10.3.19.214) with Microsoft SMTP Server id 14.3.487.0; Thu, 24 Sep 2020 19:07:52 +0800 To: Andrew Rybchenko , CC: , , , References: <1599534347-20430-1-git-send-email-humin29@huawei.com> <1600668838-31498-1-git-send-email-humin29@huawei.com> <1600668838-31498-2-git-send-email-humin29@huawei.com> <879dfbdb-d706-f4c9-2d5a-6c6eccbc4ab9@huawei.com> <5c8ba025-3c55-637c-0a0c-e99644f33d83@solarflare.com> From: "Min Hu (Connor)" Message-ID: Date: Thu, 24 Sep 2020 19:07:52 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <5c8ba025-3c55-637c-0a0c-e99644f33d83@solarflare.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH V9 1/3] ethdev: introduce FEC API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Hi, Andrew, I have fixed it about FEC in V11 according to your suggestion. could your please check it out. thanks. 在 2020/9/22 20:18, Andrew Rybchenko 写道: > On 9/22/20 2:06 PM, Min Hu (Connor) wrote: >> >> >> 在 2020/9/22 16:02, Andrew Rybchenko 写道: >>> On 9/22/20 7:58 AM, Min Hu (Connor) wrote: >>>> >>>> >>>> 在 2020/9/21 21:39, Andrew Rybchenko 写道: >>>>> On 9/21/20 9:13 AM, Min Hu (Connor) wrote: >>>>>> This patch adds Forward error correction(FEC) support for ethdev. >>>>>> Introduce APIs which support query and config FEC information in >>>>>> hardware. >>>>>> >>>>>> Signed-off-by: Min Hu (Connor) >>>>>> Reviewed-by: Wei Hu (Xavier) >>>>>> Reviewed-by: Chengwen Feng >>>>>> Reviewed-by: Chengchang Tang >>>>>> Reviewed-by: Ajit Khaparde >>>>>> Acked-by: Konstantin Ananyev > > [snip] > >>>>>> @@ -3328,6 +3349,70 @@ int  rte_eth_led_on(uint16_t port_id); >>>>>>    int  rte_eth_led_off(uint16_t port_id); >>>>>>      /** >>>>>> + * @warning >>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without >>>>>> prior notice >>>>>> + * >>>>>> + * Get Forward Error Correction(FEC) capability. >>>>>> + * >>>>>> + * @param port_id >>>>>> + *   The port identifier of the Ethernet device. >>>>>> + * @param fec_cap >>>>>> + *   returns the FEC capability from the device, as follows: >>>>>> + *   RTE_ETH_FEC_CAPA_NOFEC >>>>>> + *   RTE_ETH_FEC_CAPA_AUTO >>>>>> + *   RTE_ETH_FEC_CAPA_BASER >>>>>> + *   RTE_ETH_FEC_CAPA_RS >>>>>> + * @return >>>>>> + *   - (0) if successful. >>>>>> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support. >>>>>> + *     that operation. >>>>>> + *   - (-EIO) if device is removed. >>>>>> + *   - (-ENODEV)  if *port_id* invalid. >>>>>> + */ >>>>>> +__rte_experimental >>>>>> +int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *fec_cap); >>>>> >>>>> The API does not allow to report capabilities per link speed: >>>>> which FEC mode is supported at which link speed? >>>>> >>>>> What about something like: >>>>> >>>>> struct rte_eth_fec_capa { >>>>>     uint32_t speed; /**< Link speed (see ETH_SPEED_NUM_*) */ >>>>>     uint32_t capa;  /**< FEC capabilities bitmask (see >>>>> RTE_FEC_CAPA_*) */ >>>>> }; >>>>> >>>>> __rte_experimental >>>>> int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *num, struct >>>>> rte_eth_fec_capa *speed_capa); >>>>> >>>>> where: >>>>>    - num is in/out with a number of elements in an array >>>>>    - speed_capa is out only with per-speed capabilities >>>>> >>>> There is no need to report capabilities per link speed, because >>>> relastionship between the link speed and fec mode is fixed. The >>>> infomations can be referred to in official documents or internet. >>> >>> Should an application download documents and search for it? :) >> >> OK， I will report capabilities per link speed in V11. >> By the way, >>>>> where: >>>>>     - num is in/out with a number of elements in an array >> >> could you describe "num" more detailedly, how to use this value? > > On input, num should specify a number of elements in speed_capa > array provided by the caller to get FEC capabilities. > If the number is insufficient to, error should be returned and > the number should contain required number of elements. > If sufficient, on output driver should return a number of > filled in array elements. > >>> >>>> >>>> A ethernet port may have various link speed in diffrent situations(for >>>> example, optical module with different speed is used). But we do not >>>> care about capabilities per link speed. We only care about FEC capa of >>>> the ethernet device at a specific moment, because set FEC mode also >>>> depend on the current FEC capa. >>> >>> Capabilities should not be for a specific moment. Capabilities >>> should be fixed and stable (if transceiver is not replaced). >>> Capabilities should not depend on a link speed or link status. >>> Otherwise an application can't use it in a reliable way. >>> >>>> >>>> By the way, we can also get link speed of the device by API >>>> "rte_eth_link_get" in the same time. >>>> >>>> thanks. >>>> >>>>>> + >>>>>> +/** >>>>>> + * @warning >>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without >>>>>> prior notice >>>>>> + * >>>>>> + * Get current Forward Error Correction(FEC) mode. >>>>>> + * >>>>>> + * @param port_id >>>>>> + *   The port identifier of the Ethernet device. >>>>>> + * @param mode >>>>>> + *   returns the FEC mode from the device. >>>>>> + * @return >>>>>> + *   - (0) if successful. >>>>>> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support. >>>>>> + *     that operation. >>>>>> + *   - (-EIO) if device is removed. >>>>>> + *   - (-ENODEV)  if *port_id* invalid. >>>>>> + */ >>>>>> +__rte_experimental >>>>>> +int rte_eth_fec_get(uint16_t port_id, enum rte_eth_fec_mode *mode); >>>>> >>>>> Please, specify what should be reported if link is down. >>>>> E.g. if set to RS, but link is down. >>>>> >>>>> Does AUTO make sense here? >>>>> >>>> OK, I will add the information in the function header comment: >>>> when link down，None AUTO mode(RS, BASER. NOFEC) keep as it is when link >>>> up, AUTO mode will change from rs，baser to nofec when quering the mode. >>> >>> I'll take a look at the patch, above text is hardly readable. > >> (1). If the current mode of device is one of these modes: >> RS, BASER. NOFEC. >> when link up, for example, the mode is RS. when the device is linked >> down, the mode is always RS. > >> (2). If the current mode of device is AUTO: >> when the device is linked down, the mode varies in this order: >> rs->baser->nofec, until Auto-negotiation success (link shoud be >> up first). >> >> But this is defined in our hardware. I think the first feature(1) are >> common and can be adopted. > > Sorry, I don't understand. > What abgot if link is down and AUTO is enabled, AUTO is > returned, otherwise, configured FEC mode is returned. > If link is up, current FEC mode is returned (i.e. not AUTO, > either NOFEC, or RS, or BASER). > >>>>>> + >>>>>> +/** >>>>>> + * @warning >>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without >>>>>> prior notice >>>>>> + * >>>>>> + * Set Forward Error Correction(FEC) mode. >>>>>> + * >>>>>> + * @param port_id >>>>>> + *   The port identifier of the Ethernet device. >>>>>> + * @param mode >>>>>> + *   the FEC mode. >>>>>> + * @return >>>>>> + *   - (0) if successful. >>>>>> + *   - (-EINVAL) if the FEC mode is not valid. >>>>>> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support. >>>>>> + *   - (-EIO) if device is removed. >>>>>> + *   - (-ENODEV)  if *port_id* invalid. >>>>>> + */ >>>>>> +__rte_experimental >>>>>> +int rte_eth_fec_set(uint16_t port_id, enum rte_eth_fec_mode mode); >>>>> >>>>> It does not allow to tweak autoneg facilities. >>>>> E.g. "I know that RS is buggy, so I want to exclude it from >>>>> auto-negotiation". >>>>> So, I suggest to change mode to capa bitmask. >>>>> If AUTO is set, other bits may be set and specify allowed >>>>> options. E.g. AUTO|RS|BASER will require FEC, i.e. NOFEC is >>>> The two FEC modes cannot be configured for hardware at the same time, >>>> including AUTO and other FEC modes. This is determined by Hardware >>>> itself. >>> >>> Which HW? Yours? If so, it does not matter. The patch adds >>> generic API. My comments are not abstract thoughts. There >>> are requirements and capabilities behind. >> >> yes, it is in my HW. But I think the feature of FEC will exist in other >> HW: the two FEC modes cannot be configured for hardware at the same time. >> By the way, if set two FEC mode in our HW, the result will be unknown. >> I also test X710 nic device, it does not support that feature. >> I do not support that solutions. thanks. > > I'm not trying to say that two FEC modes could be running > simultaneously. I'm trying to say that in the future a PHY > could support more than one FEC mode and autonegotiation > could make a choice which FEC mode to use. > E.g. > NOFEC, FOO and BAR modes supported > set (AUTO|FOO|BAR) will require either FOO or BAR to be > negotiated and does not allow NOFEC. > >>>> Thanks. >>>> >>>>> not allowed. If just RS, it means that auto-negotiation is >>>>> disabled and RS must be used. >>>>> If AUTO is unset, only one bit may be set in capabilities. >>>>> Since we don't do it per speed, I think it is safe to ignore >>>>> unsupported mode bits. I.e. do not return error if unsupported >>>>> capa is requested to together with AUTO, however it could be >> Why? if the mode is unsupported (not in capa),why we can configure >> the the mode to the device? Because this is unreaonable. Also, >> the configure will not be ineffective, and the hardware will return >> error back to the driver. > > I agree that requested mode should be in capabilities for at > least some speed, but not required to be applicable to > currently negotiated and running speed. May be it is obvious. > I.e. if I set AUTO|NOFEC|RS when capabilities are > 25G(NOFEC,AUTO,BASER) and 100G(NOFEC,AUTO,RS), it will > enforce NOFEC for 25G (since BASER is disabled) and allow > either NOFEC or RS for 100G. > >>>>> a problem if no modes are allowed for negotiated link speed. >>>>> Thoughts are welcome. >>>>> >>>>>> + >>>>>> +/** >>>>>>     * Get current status of the Ethernet link flow control for >>>>>> Ethernet device >>>>>>     * >>>>>>     * @param port_id >>>>> >>>>> [snip] >>>>> >>>>> . >>>>> >>> >>> . >>> > > . >