[1/2] ethdev: deduplicate functions to get link infos
diff mbox series

Message ID 20200407222637.55289-2-thomas@monjalon.net
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • ethdev link speed
Related show

Checks

Context Check Description
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Thomas Monjalon April 7, 2020, 10:26 p.m. UTC
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

Asaf Penso April 8, 2020, 5:21 a.m. UTC | #1
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
Thomas Monjalon April 8, 2020, 12:49 p.m. UTC | #2
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.
Andrew Rybchenko April 13, 2020, 2:14 p.m. UTC | #3
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.

Patch
diff mbox series

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)
+{
+	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