Re: [dpdk-dev] [PATCH v11 2/8] ethdev: use constants for link duplex

["Zhang, Helin" <helin.zhang@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, March 18, 2016 2:09 AM
> To: marcdevel@gmail.com; Richardson, Bruce; Doherty, Declan; Ananyev,
> Konstantin; Lu, Wenzhuo; Zhang, Helin; Chen, Jing D;
> harish.patil@qlogic.com; rahul.lakkireddy@chelsio.com;
> johndale@cisco.com; vido@cesnet.cz; adrien.mazarguil@6wind.com;
> alejandro.lucero@netronome.com
> Cc: dev@dpdk.org
> Subject: [PATCH v11 2/8] ethdev: use constants for link duplex
> 
> From: Marc Sune <marcdevel@gmail.com>
> 
> Some duplex values are replaced from 0 to half-duplex when link is down.
> 
> Some drivers are still using their own constants for duplex modes.
> 
> Signed-off-by: Marc Sune <marcdevel@gmail.com>
> ---
>  drivers/net/e1000/em_ethdev.c      | 2 +-
>  drivers/net/e1000/igb_ethdev.c     | 2 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c   | 2 +-
>  drivers/net/virtio/virtio_ethdev.c | 2 +-  drivers/net/virtio/virtio_ethdev.h |
> 2 --
>  lib/librte_ether/rte_ethdev.h      | 2 +-
>  6 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/e1000/em_ethdev.c
> b/drivers/net/e1000/em_ethdev.c index 58093c6..943a270 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -1108,7 +1108,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int
> wait_to_complete)
>  		link.link_status = ETH_LINK_UP;
>  	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
>  		link.link_speed = 0;
> -		link.link_duplex = 0;
> +		link.link_duplex = ETH_LINK_HALF_DUPLEX;
>  		link.link_status = ETH_LINK_DOWN;
>  	}
>  	rte_em_dev_atomic_write_link_status(dev, &link); diff --git
> a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index
> 311f866..ea156ce 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -2033,7 +2033,7 @@ eth_igb_link_update(struct rte_eth_dev *dev, int
> wait_to_complete)
>  		link.link_status = ETH_LINK_UP;
>  	} else if (!link_check) {
>  		link.link_speed = 0;
> -		link.link_duplex = 0;
> +		link.link_duplex = ETH_LINK_HALF_DUPLEX;
>  		link.link_status = ETH_LINK_DOWN;
>  	}
>  	rte_igb_dev_atomic_write_link_status(dev, &link); diff --git
> a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index be28f7e..35dac49 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2997,7 +2997,7 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev,
> int wait_to_complete)
> 
>  	link.link_status = ETH_LINK_DOWN;
>  	link.link_speed = 0;
> -	link.link_duplex = 0;
> +	link.link_duplex = ETH_LINK_HALF_DUPLEX;
>  	memset(&old, 0, sizeof(old));
>  	rte_ixgbe_dev_atomic_read_link_status(dev, &old);
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 3ebc221..63a368a 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1401,7 +1401,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev,
> __rte_unused int wait_to_complet
>  	memset(&link, 0, sizeof(link));
>  	virtio_dev_atomic_read_link_status(dev, &link);
>  	old = link;
> -	link.link_duplex = FULL_DUPLEX;
> +	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>  	link.link_speed  = SPEED_10G;
> 
>  	if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) { diff --git
> a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index fed9571..66423a0 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -42,8 +42,6 @@
>  #define SPEED_100	100
>  #define SPEED_1000	1000
>  #define SPEED_10G	10000
> -#define HALF_DUPLEX	1
> -#define FULL_DUPLEX	2
> 
>  #ifndef PAGE_SIZE
>  #define PAGE_SIZE 4096
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index ec8d6b1..5379bee 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -246,7 +246,7 @@ struct rte_eth_stats {
>   */
>  struct rte_eth_link {
>  	uint16_t link_speed;      /**< ETH_LINK_SPEED_[10, 100, 1000, 10000]
> */
> -	uint16_t link_duplex;     /**< ETH_LINK_[HALF_DUPLEX,
> FULL_DUPLEX] */
> +	uint16_t link_duplex;     /**< ETH_LINK_[HALF/FULL]_DUPLEX */
>  	uint8_t  link_status : 1; /**< ETH_LINK_[DOWN/UP] */
>  }__attribute__((aligned(8)));     /**< aligned for atomic64 read/write */
For link speed and link duplex, I'd suggest to add one more status of 'UNKNOWN'.
Because, sometimes it cannot get all the information from hardware.
For link stauts, assume it in DOWN state is acceptable, while for other two, I don't think so.

Currently it can be seen that a default link speed and duplex will be set if it cannot
get the accurate info from hardware. That's not good, and I think UNKNOWN could be better.

What do you think?

Thanks,
Helin

> 
> --
> 2.7.0