From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <arybchenko@solarflare.com>, <dev@dpdk.org>
CC: <konstantin.ananyev@intel.com>, <thomas@monjalon.net>,
<ferruh.yigit@intel.com>, <linuxarm@huawei.com>
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>
<c2654fcf-8ce9-e480-2f18-e6daea200350@solarflare.com>
<fcd5718a-bdec-6027-76de-545d01d53fdc@huawei.com>
<f8bcf4f5-b54a-2155-045b-6d81c6e0701e@solarflare.com>
<879dfbdb-d706-f4c9-2d5a-6c6eccbc4ab9@huawei.com>
<5c8ba025-3c55-637c-0a0c-e99644f33d83@solarflare.com>
From: "Min Hu (Connor)" <humin29@huawei.com>
Message-ID: <dc391949-661e-2317-21b2-a39c5af837a7@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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) <humin29@huawei.com>
>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>> Reviewed-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>>>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> [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]
>>>>>
>>>>> .
>>>>>
>>>
>>> .
>>>
>
> .
>