[1/2] ethdev: deduplicate functions to get link infos
Checks
Commit Message
There are two function to retrieve link informations.
The only small difference is a boolean timeout parameter.
Adding a new static function, with an additional parameter,
removes the code redundancy.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
lib/librte_ethdev/rte_ethdev.c | 52 ++++++++++++++--------------------
1 file changed, 22 insertions(+), 30 deletions(-)
Comments
Thank you, Thomas, for taking care of this.
PSB.
Regards,
Asaf Penso
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Wednesday, April 8, 2020 1:27 AM
> To: dev@dpdk.org
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link
> infos
>
> There are two function to retrieve link informations.
> The only small difference is a boolean timeout parameter.
> Adding a new static function, with an additional parameter,
> removes the code redundancy.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> lib/librte_ethdev/rte_ethdev.c | 52 ++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 0854ef8832..0df39dff97 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -2332,44 +2332,36 @@ rte_eth_allmulticast_get(uint16_t port_id)
> return dev->data->all_multicast;
> }
>
> +static int
> +get_link_infos(uint16_t port_id, struct rte_eth_link *eth_link, int wait)
I would recommend renaming to link_get_infos, to have the same naming convention as rte_eth_*link_get* and rte_eth_*link_get*_nowait
> +{
> + struct rte_eth_dev *dev;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + dev = &rte_eth_devices[port_id];
> +
> + if (dev->data->dev_conf.intr_conf.lsc &&
> + dev->data->dev_started)
> + rte_eth_linkstatus_get(dev, eth_link);
> + else {
> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >link_update, -ENOTSUP);
> + (*dev->dev_ops->link_update)(dev, wait);
> + *eth_link = dev->data->dev_link;
> + }
> +
> + return 0;
Since it's a static function, I think it can return void, and the calling functions can decide what to return, but it's a matter of taste.
Do we want to check that the return value for eth_link is not NULL and return -1 in case it is?
> +}
> +
> int
> rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
> {
> - struct rte_eth_dev *dev;
> -
> - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> - dev = &rte_eth_devices[port_id];
> -
> - if (dev->data->dev_conf.intr_conf.lsc &&
> - dev->data->dev_started)
> - rte_eth_linkstatus_get(dev, eth_link);
> - else {
> - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >link_update, -ENOTSUP);
> - (*dev->dev_ops->link_update)(dev, 1);
> - *eth_link = dev->data->dev_link;
> - }
> -
> - return 0;
> + return get_link_infos(port_id, eth_link, 1);
> }
>
> int
> rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
> {
> - struct rte_eth_dev *dev;
> -
> - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> - dev = &rte_eth_devices[port_id];
> -
> - if (dev->data->dev_conf.intr_conf.lsc &&
> - dev->data->dev_started)
> - rte_eth_linkstatus_get(dev, eth_link);
> - else {
> - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >link_update, -ENOTSUP);
> - (*dev->dev_ops->link_update)(dev, 0);
> - *eth_link = dev->data->dev_link;
> - }
> -
> - return 0;
> + return get_link_infos(port_id, eth_link, 0);
> }
>
> int
> --
> 2.26.0
08/04/2020 07:21, Asaf Penso:
> From: Thomas Monjalon
> > +static int
> > +get_link_infos(uint16_t port_id, struct rte_eth_link *eth_link, int wait)
>
> I would recommend renaming to link_get_infos, to have the same naming
> convention as rte_eth_*link_get* and rte_eth_*link_get*_nowait
No strong opinion.
get_link_infos looks more natural english.
If others prefer to have a sort of consistency, fine.
> > +{
> > + struct rte_eth_dev *dev;
> > +
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > + dev = &rte_eth_devices[port_id];
> > +
> > + if (dev->data->dev_conf.intr_conf.lsc &&
> > + dev->data->dev_started)
> > + rte_eth_linkstatus_get(dev, eth_link);
> > + else {
> > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> > >link_update, -ENOTSUP);
> > + (*dev->dev_ops->link_update)(dev, wait);
> > + *eth_link = dev->data->dev_link;
> > + }
> > +
> > + return 0;
>
> Since it's a static function, I think it can return void,
No, it cannot return void because some errors may be returned
with the macros RTE_ETH_VALID_PORTID_OR_ERR_RET and
RTE_FUNC_PTR_OR_ERR_RET.
> and the calling functions can decide what to return,
> but it's a matter of taste.
>
> Do we want to check that the return value for eth_link
> is not NULL and return -1 in case it is?
eth_link must not be NULL. It is allocated by the caller.
On 4/8/20 1:26 AM, Thomas Monjalon wrote:
> There are two function to retrieve link informations.
> The only small difference is a boolean timeout parameter.
> Adding a new static function, with an additional parameter,
> removes the code redundancy.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
and I'm fine with get_link_infos() function name since
there a number of ^get_*() static functions in the file.
@@ -2332,44 +2332,36 @@ rte_eth_allmulticast_get(uint16_t port_id)
return dev->data->all_multicast;
}
+static int
+get_link_infos(uint16_t port_id, struct rte_eth_link *eth_link, int wait)
+{
+ struct rte_eth_dev *dev;
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ dev = &rte_eth_devices[port_id];
+
+ if (dev->data->dev_conf.intr_conf.lsc &&
+ dev->data->dev_started)
+ rte_eth_linkstatus_get(dev, eth_link);
+ else {
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
+ (*dev->dev_ops->link_update)(dev, wait);
+ *eth_link = dev->data->dev_link;
+ }
+
+ return 0;
+}
+
int
rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
{
- struct rte_eth_dev *dev;
-
- RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
- dev = &rte_eth_devices[port_id];
-
- if (dev->data->dev_conf.intr_conf.lsc &&
- dev->data->dev_started)
- rte_eth_linkstatus_get(dev, eth_link);
- else {
- RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
- (*dev->dev_ops->link_update)(dev, 1);
- *eth_link = dev->data->dev_link;
- }
-
- return 0;
+ return get_link_infos(port_id, eth_link, 1);
}
int
rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
{
- struct rte_eth_dev *dev;
-
- RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
- dev = &rte_eth_devices[port_id];
-
- if (dev->data->dev_conf.intr_conf.lsc &&
- dev->data->dev_started)
- rte_eth_linkstatus_get(dev, eth_link);
- else {
- RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
- (*dev->dev_ops->link_update)(dev, 0);
- *eth_link = dev->data->dev_link;
- }
-
- return 0;
+ return get_link_infos(port_id, eth_link, 0);
}
int