[RFC,3/3] ethdev: rename parameter in API to get FEC
Checks
Commit Message
Only one valid FEC mode can be get by rte_eth_fec_get().
The previous name implied that more than one FEC mode
can be obtained.
Documentation was updated accordingly.
Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
Acked-by: Ivan Malov <ivan.malov@arknetworks.am>
Acked-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
---
lib/ethdev/rte_ethdev.c | 8 ++++----
lib/ethdev/rte_ethdev.h | 9 +++------
2 files changed, 7 insertions(+), 10 deletions(-)
Comments
On 4/28/2023 11:27 AM, Denis Pryazhennikov wrote:
> Only one valid FEC mode can be get by rte_eth_fec_get().
> The previous name implied that more than one FEC mode
> can be obtained.
+1 and patch looks good.
But isn't this valid for 'rte_eth_fec_set()', it gets 'fec_mode'. FEC
capability has its own type "struct rte_eth_fec_capa".
Independent from being single FEC mode or not, I think both
'rte_eth_fec_get()' & 'rte_eth_fec_set()' should get 'fec_mode' as
param, what do you think?
> Documentation was updated accordingly.
>
> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
> Acked-by: Ivan Malov <ivan.malov@arknetworks.am>
> Acked-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
<...>
I think my patch isn't correct.
I would say that
- fec_capa is a set of bits produced by 'RTE_ETH_FEC_MODE_TO_CAPA()';
- fec_mode is an element of 'enum rte_eth_fec_mode'.
Based on this definition, replacing 'fec_capa' with 'fec_mode' should
involve changing the parameter type.
Probably I shouldn't change the name, but I should add a more detailed
comment.
> Independent from being single FEC mode or not, I think both
'rte_eth_fec_get()' & 'rte_eth_fec_set()' should get 'fec_mode' as
param, what do you think?
Andrew Rybchenko asked to replace 'mode' with 'fec_capa' for
'rte_eth_fec_set()' in
https://inbox.dpdk.org/dev/aa745bd1-a564-fa8c-c77b-2d99c97690aa@solarflare.com/
I don't think we need to change it for rte_eth_fec_set().
On 5/2/23 7:02 PM, Ferruh Yigit wrote:
> On 4/28/2023 11:27 AM, Denis Pryazhennikov wrote:
>> Only one valid FEC mode can be get by rte_eth_fec_get().
>> The previous name implied that more than one FEC mode
>> can be obtained.
> +1 and patch looks good.
>
> But isn't this valid for 'rte_eth_fec_set()', it gets 'fec_mode'. FEC
> capability has its own type "struct rte_eth_fec_capa".
>
> Independent from being single FEC mode or not, I think both
> 'rte_eth_fec_get()' & 'rte_eth_fec_set()' should get 'fec_mode' as
> param, what do you think?
>
>> Documentation was updated accordingly.
>>
>> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
>> Acked-by: Ivan Malov <ivan.malov@arknetworks.am>
>> Acked-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
> <...>
>
On 5/4/2023 8:13 AM, Denis Pryazhennikov wrote:
> I think my patch isn't correct.
> I would say that
> - fec_capa is a set of bits produced by 'RTE_ETH_FEC_MODE_TO_CAPA()';
> - fec_mode is an element of 'enum rte_eth_fec_mode'.
>
> Based on this definition, replacing 'fec_capa' with 'fec_mode' should
> involve changing the parameter type.
> Probably I shouldn't change the name, but I should add a more detailed
> comment.
>
>> Independent from being single FEC mode or not, I think both
> 'rte_eth_fec_get()' & 'rte_eth_fec_set()' should get 'fec_mode' as
> param, what do you think?
>
> Andrew Rybchenko asked to replace 'mode' with 'fec_capa' for
> 'rte_eth_fec_set()' in
> https://inbox.dpdk.org/dev/aa745bd1-a564-fa8c-c77b-2d99c97690aa@solarflare.com/
> I don't think we need to change it for rte_eth_fec_set().
>
OK
> On 5/2/23 7:02 PM, Ferruh Yigit wrote:
>> On 4/28/2023 11:27 AM, Denis Pryazhennikov wrote:
>>> Only one valid FEC mode can be get by rte_eth_fec_get().
>>> The previous name implied that more than one FEC mode
>>> can be obtained.
>> +1 and patch looks good.
>>
>> But isn't this valid for 'rte_eth_fec_set()', it gets 'fec_mode'. FEC
>> capability has its own type "struct rte_eth_fec_capa".
>>
>> Independent from being single FEC mode or not, I think both
>> 'rte_eth_fec_get()' & 'rte_eth_fec_set()' should get 'fec_mode' as
>> param, what do you think?
>>
>>> Documentation was updated accordingly.
>>>
>>> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
>>> Acked-by: Ivan Malov <ivan.malov@arknetworks.am>
>>> Acked-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
>> <...>
>>
@@ -4718,7 +4718,7 @@ rte_eth_fec_get_capability(uint16_t port_id,
}
int
-rte_eth_fec_get(uint16_t port_id, uint32_t *fec_capa)
+rte_eth_fec_get(uint16_t port_id, uint32_t *fec_mode)
{
struct rte_eth_dev *dev;
int ret;
@@ -4726,7 +4726,7 @@ rte_eth_fec_get(uint16_t port_id, uint32_t *fec_capa)
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
dev = &rte_eth_devices[port_id];
- if (fec_capa == NULL) {
+ if (fec_mode == NULL) {
RTE_ETHDEV_LOG(ERR,
"Cannot get ethdev port %u current FEC mode to NULL\n",
port_id);
@@ -4735,9 +4735,9 @@ rte_eth_fec_get(uint16_t port_id, uint32_t *fec_capa)
if (*dev->dev_ops->fec_get == NULL)
return -ENOTSUP;
- ret = eth_err(port_id, (*dev->dev_ops->fec_get)(dev, fec_capa));
+ ret = eth_err(port_id, (*dev->dev_ops->fec_get)(dev, fec_mode));
- rte_eth_trace_fec_get(port_id, fec_capa, ret);
+ rte_eth_trace_fec_get(port_id, fec_mode, ret);
return ret;
}
@@ -4203,11 +4203,8 @@ int rte_eth_fec_get_capability(uint16_t port_id,
*
* @param port_id
* The port identifier of the Ethernet device.
- * @param fec_capa
- * A bitmask of enabled FEC modes. If AUTO bit is set, other
- * bits specify FEC modes which may be negotiated. If AUTO
- * bit is clear, specify FEC modes to be used (only one valid
- * mode per speed may be set).
+ * @param fec_mode
+ * The current FEC mode.
* @return
* - (0) if successful.
* - (-ENOTSUP) if underlying hardware OR driver doesn't support.
@@ -4216,7 +4213,7 @@ int rte_eth_fec_get_capability(uint16_t port_id,
* - (-ENODEV) if *port_id* invalid.
*/
__rte_experimental
-int rte_eth_fec_get(uint16_t port_id, uint32_t *fec_capa);
+int rte_eth_fec_get(uint16_t port_id, uint32_t *fec_mode);
/**
* @warning