Re: [dpdk-dev] [PATCH v2 6/8] bus/dpaa: enhance link status support

[Hemant Agrawal <hemant.agrawal@nxp.com>]

Hi ,

> -----Original Message-----
> From: Sachin Saxena (OSS) <sachin.saxena@oss.nxp.com>
> Sent: Friday, September 11, 2020 2:05 PM
> To: dev@dpdk.org; ferruh.yigit@intel.com
> Subject: [PATCH v2 6/8] bus/dpaa: enhance link status support
> 
> From: Rohit Raj <rohit.raj@nxp.com>
> 
> This patch adds support to get/set link speed, duplex and autoneg mode
> status from/to PHY. Enahnce device capabilities to advertise all supported
> link speeds according to mac type.
> 
> Also enables fallback support for get link status api where kernel support is
> missing.
> 
> Signed-off-by: Sachin Saxena <sachin.saxena@oss.nxp.com>
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
> ---
>  drivers/bus/dpaa/base/qbman/process.c     | 123 +++++++++++++++++++---
>  drivers/bus/dpaa/include/process.h        |  34 +++++-
>  drivers/bus/dpaa/rte_bus_dpaa_version.map |   3 +
>  drivers/net/dpaa/dpaa_ethdev.c            | 113 ++++++++++++++++----
>  4 files changed, 241 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/bus/dpaa/base/qbman/process.c
> b/drivers/bus/dpaa/base/qbman/process.c
> index 6f7e37957..9bc92681c 100644
> --- a/drivers/bus/dpaa/base/qbman/process.c
> +++ b/drivers/bus/dpaa/base/qbman/process.c
> @@ -8,6 +8,7 @@
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <sys/ioctl.h>
> +#include <rte_ethdev.h>
> 
>  #include "process.h"
> 
> @@ -341,30 +342,78 @@ int dpaa_intr_disable(char *if_name)
>  	return 0;
>  }
> 
> +#define DPAA_IOCTL_GET_IOCTL_VERSION \
> +	_IOR(DPAA_IOCTL_MAGIC, 0x14, int)
> +
[Hemant] This patch has dependency on dpaa kernel driver. If these are new changes, please also change the minimal kernel dependency in dpaa platform and dpaa nic documentation?
Are these changes already part of kernel tree? 
Is the kernel dependency documented? 

> +int dpaa_get_ioctl_version_number(void)
> +{
> +	int version_num, ret = check_fd();
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = ioctl(fd, DPAA_IOCTL_GET_IOCTL_VERSION, &version_num);
> +	if (ret) {
> +		if (errno == EINVAL) {
> +			version_num = 1;
> +		} else {
> +			printf("Failed to get ioctl version number\n");
> +			version_num = -1;
> +		}
> +	}
> +
> +	return version_num;
> +}
> +
>  #define DPAA_IOCTL_GET_LINK_STATUS \
>  	_IOWR(DPAA_IOCTL_MAGIC, 0x10, struct
> usdpaa_ioctl_link_status_args)
> 
> -int dpaa_get_link_status(char *if_name)
> +#define DPAA_IOCTL_GET_LINK_STATUS_OLD \
> +	_IOWR(DPAA_IOCTL_MAGIC, 0x10, struct
> +usdpaa_ioctl_link_status_args_old)
> +
> +
> +int dpaa_get_link_status(char *if_name, struct rte_eth_link *link)
>  {
> -	int ret = check_fd();
> -	struct usdpaa_ioctl_link_status_args args;
> +	int ioctl_version, ret = check_fd();
> 
>  	if (ret)
>  		return ret;
> 
> -	strcpy(args.if_name, if_name);
> -	args.link_status = 0;
> +	ioctl_version = dpaa_get_ioctl_version_number();
> 
> -	ret = ioctl(fd, DPAA_IOCTL_GET_LINK_STATUS, &args);
> -	if (ret) {
> -		if (errno == EINVAL)
> -			printf("Failed to get link status: Not Supported\n");
> -		else
> +	if (ioctl_version == 2) {
> +		struct usdpaa_ioctl_link_status_args args;
> +
> +		strcpy(args.if_name, if_name);
> +
> +		ret = ioctl(fd, DPAA_IOCTL_GET_LINK_STATUS, &args);
> +		if (ret) {
>  			printf("Failed to get link status\n");
> -		return ret;
> +			return ret;
> +		}
> +
> +		link->link_status = args.link_status;
> +		link->link_speed = args.link_speed;
> +		link->link_duplex = args.link_duplex;
> +		link->link_autoneg = args.link_autoneg;
> +	} else {
> +		struct usdpaa_ioctl_link_status_args_old args;
> +
> +		strcpy(args.if_name, if_name);
> +
> +		ret = ioctl(fd, DPAA_IOCTL_GET_LINK_STATUS_OLD, &args);
> +		if (ret) {
> +			if (errno == EINVAL)
> +				printf("Get link status: Not Supported\n");
> +			else
> +				printf("Failed to get link status\n");
> +			return ret;
> +		}
> +
> +		link->link_status = args.link_status;
>  	}
> 
> -	return args.link_status;
> +	return 0;
>  }
> 
>  #define DPAA_IOCTL_UPDATE_LINK_STATUS \ @@ -393,3 +442,53 @@ int
> dpaa_update_link_status(char *if_name, int link_status)
> 
>  	return 0;
>  }
> +
> +#define DPAA_IOCTL_UPDATE_LINK_SPEED \
> +	_IOW(DPAA_IOCTL_MAGIC, 0x12, struct
> usdpaa_ioctl_update_link_speed)
> +
> +int dpaa_update_link_speed(char *if_name, int link_speed, int
> +link_duplex) {
> +	struct usdpaa_ioctl_update_link_speed args;
> +	int ret;
> +
> +	ret = check_fd();
> +	if (ret)
> +		return ret;
> +
> +	strcpy(args.if_name, if_name);
> +	args.link_speed = link_speed;
> +	args.link_duplex = link_duplex;
> +
> +	ret = ioctl(fd, DPAA_IOCTL_UPDATE_LINK_SPEED, &args);
> +	if (ret) {
> +		if (errno == EINVAL)
> +			printf("Failed to set link speed: Not Supported\n");
> +		else
> +			printf("Failed to set link speed\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +#define DPAA_IOCTL_RESTART_LINK_AUTONEG \
> +	_IOW(DPAA_IOCTL_MAGIC, 0x13, char *)
> +
> +int dpaa_restart_link_autoneg(char *if_name) {
> +	int ret = check_fd();
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = ioctl(fd, DPAA_IOCTL_RESTART_LINK_AUTONEG, &if_name);
> +	if (ret) {
> +		if (errno == EINVAL)
> +			printf("Failed to restart autoneg: Not Supported\n");
> +		else
> +			printf("Failed to restart autoneg\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> diff --git a/drivers/bus/dpaa/include/process.h
> b/drivers/bus/dpaa/include/process.h
> index f52ea1635..be52e6f72 100644
> --- a/drivers/bus/dpaa/include/process.h
> +++ b/drivers/bus/dpaa/include/process.h
> @@ -10,6 +10,7 @@
>  #define	__PROCESS_H
> 
>  #include <compat.h>
> +#include <rte_ethdev.h>
> 
>  /* The process device underlies process-wide user/kernel interactions, such
> as
>   * mapping dma_mem memory and providing accompanying ioctl()s. (This
> isn't used @@ -86,10 +87,25 @@ int dpaa_intr_enable(char *if_name, int
> efd);  __rte_internal  int dpaa_intr_disable(char *if_name);
> 
> +struct usdpaa_ioctl_link_status_args_old {
> +	/* network device node name */
> +	char    if_name[IF_NAME_MAX_LEN];
> +	/* link status(ETH_LINK_UP/DOWN) */
> +	int     link_status;
> +};
> +
>  struct usdpaa_ioctl_link_status_args {
>  	/* network device node name */
>  	char    if_name[IF_NAME_MAX_LEN];
> +	/* link status(ETH_LINK_UP/DOWN) */
>  	int     link_status;
> +	/* link speed (ETH_SPEED_NUM_)*/
> +	int     link_speed;
> +	/* link duplex (ETH_LINK_[HALF/FULL]_DUPLEX)*/
> +	int     link_duplex;
> +	/* link autoneg (ETH_LINK_AUTONEG/FIXED)*/
> +	int     link_autoneg;
> +
>  };
> 
>  struct usdpaa_ioctl_update_link_status_args { @@ -99,10 +115,24 @@
> struct usdpaa_ioctl_update_link_status_args {
>  	int     link_status;
>  };
> 
> -__rte_internal
> -int dpaa_get_link_status(char *if_name);
> +struct usdpaa_ioctl_update_link_speed {
> +	/* network device node name*/
> +	char    if_name[IF_NAME_MAX_LEN];
> +	/* link speed (ETH_SPEED_NUM_)*/
> +	int     link_speed;
> +	/* link duplex (ETH_LINK_[HALF/FULL]_DUPLEX)*/
> +	int     link_duplex;
> +};
> 
> +__rte_internal
> +int dpaa_get_link_status(char *if_name, struct rte_eth_link *link);
>  __rte_internal
>  int dpaa_update_link_status(char *if_name, int link_status);
> +__rte_internal
> +int dpaa_update_link_speed(char *if_name, int speed, int duplex);
> +__rte_internal int dpaa_restart_link_autoneg(char *if_name);
> +__rte_internal int dpaa_get_ioctl_version_number(void);
> 
>  #endif	/*  __PROCESS_H */
> diff --git a/drivers/bus/dpaa/rte_bus_dpaa_version.map
> b/drivers/bus/dpaa/rte_bus_dpaa_version.map
> index 5b97b6eb2..26436e9fe 100644
> --- a/drivers/bus/dpaa/rte_bus_dpaa_version.map
> +++ b/drivers/bus/dpaa/rte_bus_dpaa_version.map
> @@ -8,10 +8,13 @@ INTERNAL {
>  	bman_query_free_buffers;
>  	bman_release;
>  	bman_thread_irq;
> +	dpaa_get_ioctl_version_number;
>  	dpaa_get_eth_port_cfg;
>  	dpaa_get_qm_channel_caam;
>  	dpaa_get_qm_channel_pool;
>  	dpaa_get_link_status;
> +	dpaa_restart_link_autoneg;
> +	dpaa_update_link_speed;
>  	dpaa_intr_disable;
>  	dpaa_intr_enable;
>  	dpaa_svr_family;
> diff --git a/drivers/net/dpaa/dpaa_ethdev.c
> b/drivers/net/dpaa/dpaa_ethdev.c index d5eeb280a..66bb6eb00 100644
> --- a/drivers/net/dpaa/dpaa_ethdev.c
> +++ b/drivers/net/dpaa/dpaa_ethdev.c
> @@ -205,10 +205,12 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
>  	uint64_t rx_offloads = eth_conf->rxmode.offloads;
>  	uint64_t tx_offloads = eth_conf->txmode.offloads;
>  	struct rte_device *rdev = dev->device;
> +	struct rte_eth_link *link = &dev->data->dev_link;
>  	struct rte_dpaa_device *dpaa_dev;
>  	struct fman_if *fif = dev->process_private;
>  	struct __fman_if *__fif;
>  	struct rte_intr_handle *intr_handle;
> +	int speed, duplex;
>  	int ret;
> 
>  	PMD_INIT_FUNC_TRACE();
> @@ -292,6 +294,60 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
>  			dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
>  		}
>  	}
> +
> +	/* Wait for link status to get updated */
> +	if (!link->link_status)
> +		sleep(1);
> +
> +	/* Configure link only if link is UP*/
> +	if (link->link_status) {
> +		if (eth_conf->link_speeds == ETH_LINK_SPEED_AUTONEG) {
> +			/* Start autoneg only if link is not in autoneg mode
> */
> +			if (!link->link_autoneg)
> +				dpaa_restart_link_autoneg(__fif-
> >node_name);
> +		} else if (eth_conf->link_speeds & ETH_LINK_SPEED_FIXED) {
> +			switch (eth_conf->link_speeds &
> ~ETH_LINK_SPEED_FIXED) {
> +			case ETH_LINK_SPEED_10M_HD:
> +				speed = ETH_SPEED_NUM_10M;
> +				duplex = ETH_LINK_HALF_DUPLEX;
> +				break;
> +			case ETH_LINK_SPEED_10M:
> +				speed = ETH_SPEED_NUM_10M;
> +				duplex = ETH_LINK_FULL_DUPLEX;
> +				break;
> +			case ETH_LINK_SPEED_100M_HD:
> +				speed = ETH_SPEED_NUM_100M;
> +				duplex = ETH_LINK_HALF_DUPLEX;
> +				break;
> +			case ETH_LINK_SPEED_100M:
> +				speed = ETH_SPEED_NUM_100M;
> +				duplex = ETH_LINK_FULL_DUPLEX;
> +				break;
> +			case ETH_LINK_SPEED_1G:
> +				speed = ETH_SPEED_NUM_1G;
> +				duplex = ETH_LINK_FULL_DUPLEX;
> +				break;
> +			case ETH_LINK_SPEED_2_5G:
> +				speed = ETH_SPEED_NUM_2_5G;
> +				duplex = ETH_LINK_FULL_DUPLEX;
> +				break;
> +			case ETH_LINK_SPEED_10G:
> +				speed = ETH_SPEED_NUM_10G;
> +				duplex = ETH_LINK_FULL_DUPLEX;
> +				break;
> +			default:
> +				speed = ETH_SPEED_NUM_NONE;
> +				duplex = ETH_LINK_FULL_DUPLEX;
> +				break;
> +			}
> +			/* Set link speed */
> +			dpaa_update_link_speed(__fif->node_name, speed,
> duplex);
> +		} else {
> +			/* Manual autoneg - custom advertisement speed.
> */
> +			printf("Custom Advertisement speeds not
> supported\n");
> +		}
> +	}
> +
>  	return 0;
>  }
> 
> @@ -377,6 +433,7 @@ static void dpaa_eth_dev_close(struct rte_eth_dev
> *dev)
>  	struct rte_device *rdev = dev->device;
>  	struct rte_dpaa_device *dpaa_dev;
>  	struct rte_intr_handle *intr_handle;
> +	struct rte_eth_link *link = &dev->data->dev_link;
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> @@ -386,6 +443,10 @@ static void dpaa_eth_dev_close(struct rte_eth_dev
> *dev)
> 
>  	dpaa_eth_dev_stop(dev);
> 
> +	/* Reset link to autoneg */
> +	if (link->link_status && !link->link_autoneg)
> +		dpaa_restart_link_autoneg(__fif->node_name);
> +
>  	if (intr_handle && intr_handle->fd &&
>  	    dev->data->dev_conf.intr_conf.lsc != 0) {
>  		dpaa_intr_disable(__fif->node_name);
> @@ -446,12 +507,24 @@ static int dpaa_eth_dev_info(struct rte_eth_dev
> *dev,
>  	dev_info->flow_type_rss_offloads = DPAA_RSS_OFFLOAD_ALL;
> 
>  	if (fif->mac_type == fman_mac_1g) {
> -		dev_info->speed_capa = ETH_LINK_SPEED_1G;
> +		dev_info->speed_capa = ETH_LINK_SPEED_10M_HD
> +					| ETH_LINK_SPEED_10M
> +					| ETH_LINK_SPEED_100M_HD
> +					| ETH_LINK_SPEED_100M
> +					| ETH_LINK_SPEED_1G;
>  	} else if (fif->mac_type == fman_mac_2_5g) {
> -		dev_info->speed_capa = ETH_LINK_SPEED_1G
> +		dev_info->speed_capa = ETH_LINK_SPEED_10M_HD
> +					| ETH_LINK_SPEED_10M
> +					| ETH_LINK_SPEED_100M_HD
> +					| ETH_LINK_SPEED_100M
> +					| ETH_LINK_SPEED_1G
>  					| ETH_LINK_SPEED_2_5G;
>  	} else if (fif->mac_type == fman_mac_10g) {
> -		dev_info->speed_capa = ETH_LINK_SPEED_1G
> +		dev_info->speed_capa = ETH_LINK_SPEED_10M_HD
> +					| ETH_LINK_SPEED_10M
> +					| ETH_LINK_SPEED_100M_HD
> +					| ETH_LINK_SPEED_100M
> +					| ETH_LINK_SPEED_1G
>  					| ETH_LINK_SPEED_2_5G
>  					| ETH_LINK_SPEED_10G;
>  	} else {
> @@ -548,31 +621,35 @@ static int dpaa_eth_link_update(struct
> rte_eth_dev *dev,
>  	struct rte_eth_link *link = &dev->data->dev_link;
>  	struct fman_if *fif = dev->process_private;
>  	struct __fman_if *__fif = container_of(fif, struct __fman_if, __if);
> -	int ret;
> +	int ret, ioctl_version;
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> -	if (fif->mac_type == fman_mac_1g)
> -		link->link_speed = ETH_SPEED_NUM_1G;
> -	else if (fif->mac_type == fman_mac_2_5g)
> -		link->link_speed = ETH_SPEED_NUM_2_5G;
> -	else if (fif->mac_type == fman_mac_10g)
> -		link->link_speed = ETH_SPEED_NUM_10G;
> -	else
> -		DPAA_PMD_ERR("invalid link_speed: %s, %d",
> -			     dpaa_intf->name, fif->mac_type);
> +	ioctl_version = dpaa_get_ioctl_version_number();
> +
> 
>  	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
> -		ret = dpaa_get_link_status(__fif->node_name);
> -		if (ret < 0)
> +		ret = dpaa_get_link_status(__fif->node_name, link);
> +		if (ret)
>  			return ret;
> -		link->link_status = ret;
>  	} else {
>  		link->link_status = dpaa_intf->valid;
>  	}
> 
> -	link->link_duplex = ETH_LINK_FULL_DUPLEX;
> -	link->link_autoneg = ETH_LINK_AUTONEG;
> +	if (ioctl_version < 2) {
> +		link->link_duplex = ETH_LINK_FULL_DUPLEX;
> +		link->link_autoneg = ETH_LINK_AUTONEG;
> +
> +		if (fif->mac_type == fman_mac_1g)
> +			link->link_speed = ETH_SPEED_NUM_1G;
> +		else if (fif->mac_type == fman_mac_2_5g)
> +			link->link_speed = ETH_SPEED_NUM_2_5G;
> +		else if (fif->mac_type == fman_mac_10g)
> +			link->link_speed = ETH_SPEED_NUM_10G;
> +		else
> +			DPAA_PMD_ERR("invalid link_speed: %s, %d",
> +				     dpaa_intf->name, fif->mac_type);
> +	}
> 
>  	DPAA_PMD_INFO("Port %d Link is %s\n", dev->data->port_id,
>  		      link->link_status ? "Up" : "Down");
> --
> 2.28.0