Hi Stephen,
Please see few comments bellow,
On Tue, Jan 02, 2018 at 12:53:10PM -0800, Stephen Hemminger wrote:
> This driver uses ethtool to get link status. The ethtool API has new
> and old deprecated API. Rather than checking kernel version, use the
> same algorithm that the ethtool command does; check the new API first
> and if that fails, try the old one.
>
> Also, use common code for getting link state up/down and comparing
> for changes.
>
> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 110
> ++++++++++++++++++-----------------------
> 1 file changed, 47 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 388507f109f7..2dc32cdf58b9 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -49,7 +49,6 @@
> #include <netinet/in.h>
> #include <linux/ethtool.h>
> #include <linux/sockios.h>
> -#include <linux/version.h>
> #include <fcntl.h>
> #include <stdalign.h>
> #include <sys/un.h>
> @@ -757,36 +756,25 @@ mlx5_dev_supported_ptypes_get(struct rte_eth_dev *dev)
> * Pointer to Ethernet device structure.
> */
> static int
> -mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
> +mlx5_link_update_unlocked_gset(struct priv *priv, struct ifreq *ifr,
> + struct rte_eth_link *dev_link)
Function documentation should also describe the new parameter.
> {
> - struct priv *priv = mlx5_get_priv(dev);
> struct ethtool_cmd edata = {
> .cmd = ETHTOOL_GSET /* Deprecated since Linux v4.5. */
> };
> - struct ifreq ifr;
> - struct rte_eth_link dev_link;
> int link_speed = 0;
>
> - /* priv_lock() is not taken to allow concurrent calls. */
> -
> - if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
> - WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
> - return -1;
> - }
> - memset(&dev_link, 0, sizeof(dev_link));
> - dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> - (ifr.ifr_flags & IFF_RUNNING));
> - ifr.ifr_data = (void *)&edata;
> - if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) {
> + ifr->ifr_data = (void *)&edata;
> + if (priv_ifreq(priv, SIOCETHTOOL, ifr)) {
> WARN("ioctl(SIOCETHTOOL, ETHTOOL_GSET) failed: %s",
> strerror(errno));
> return -1;
> }
> link_speed = ethtool_cmd_speed(&edata);
> if (link_speed == -1)
> - dev_link.link_speed = 0;
> + dev_link->link_speed = 0;
> else
> - dev_link.link_speed = link_speed;
> + dev_link->link_speed = link_speed;
> priv->link_speed_capa = 0;
> if (edata.supported & SUPPORTED_Autoneg)
> priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;
> @@ -800,17 +788,9 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
> SUPPORTED_40000baseSR4_Full |
> SUPPORTED_40000baseLR4_Full))
> priv->link_speed_capa |= ETH_LINK_SPEED_40G;
> - dev_link.link_duplex = ((edata.duplex == DUPLEX_HALF) ?
> + dev_link->link_duplex = ((edata.duplex == DUPLEX_HALF) ?
> ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> - dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> - ETH_LINK_SPEED_FIXED);
> - if (memcmp(&dev_link, &dev->data->dev_link, sizeof(dev_link))) {
> - /* Link status changed. */
> - dev->data->dev_link = dev_link;
> - return 0;
> - }
> - /* Link status is still the same. */
> - return -1;
> + return 0;
> }
>
> /**
> @@ -820,23 +800,14 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
> * Pointer to Ethernet device structure.
> */
> static int
> -mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
> +mlx5_link_update_unlocked_gs(struct priv *priv, struct ifreq *ifr,
> + struct rte_eth_link *dev_link)
Same here.
> {
> - struct priv *priv = mlx5_get_priv(dev);
> struct ethtool_link_settings gcmd = { .cmd = ETHTOOL_GLINKSETTINGS };
> - struct ifreq ifr;
> - struct rte_eth_link dev_link;
> uint64_t sc;
>
> - if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
> - WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
> - return -1;
> - }
> - memset(&dev_link, 0, sizeof(dev_link));
> - dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> - (ifr.ifr_flags & IFF_RUNNING));
> - ifr.ifr_data = (void *)&gcmd;
> - if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) {
> + ifr->ifr_data = (void *)&gcmd;
> + if (priv_ifreq(priv, SIOCETHTOOL, ifr)) {
> DEBUG("ioctl(SIOCETHTOOL, ETHTOOL_GLINKSETTINGS) failed: %s",
> strerror(errno));
> return -1;
> @@ -849,13 +820,13 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
> struct ethtool_link_settings *ecmd = (void *)data;
>
> *ecmd = gcmd;
> - ifr.ifr_data = (void *)ecmd;
> - if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) {
> + ifr->ifr_data = (void *)ecmd;
> + if (priv_ifreq(priv, SIOCETHTOOL, ifr)) {
> DEBUG("ioctl(SIOCETHTOOL, ETHTOOL_GLINKSETTINGS) failed: %s",
> strerror(errno));
> return -1;
> }
> - dev_link.link_speed = ecmd->speed;
> + dev_link->link_speed = ecmd->speed;
> sc = ecmd->link_mode_masks[0] |
> ((uint64_t)ecmd->link_mode_masks[1] << 32);
> priv->link_speed_capa = 0;
> @@ -893,17 +864,9 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
> MLX5_BITSHIFT(ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT) |
> MLX5_BITSHIFT(ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT)))
> priv->link_speed_capa |= ETH_LINK_SPEED_100G;
> - dev_link.link_duplex = ((ecmd->duplex == DUPLEX_HALF) ?
> + dev_link->link_duplex = ((ecmd->duplex == DUPLEX_HALF) ?
> ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> - dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> - ETH_LINK_SPEED_FIXED);
> - if (memcmp(&dev_link, &dev->data->dev_link, sizeof(dev_link))) {
> - /* Link status changed. */
> - dev->data->dev_link = dev_link;
> - return 0;
> - }
> - /* Link status is still the same. */
> - return -1;
> + return 0;
> }
>
> /**
> @@ -917,15 +880,36 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
> int
> mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
> {
> - struct utsname utsname;
> - int ver[3];
> -
> - if (uname(&utsname) == -1 ||
> - sscanf(utsname.release, "%d.%d.%d",
> - &ver[0], &ver[1], &ver[2]) != 3 ||
> - KERNEL_VERSION(ver[0], ver[1], ver[2]) < KERNEL_VERSION(4, 9, 0))
> - return mlx5_link_update_unlocked_gset(dev);
> - return mlx5_link_update_unlocked_gs(dev);
> + struct priv *priv = mlx5_get_priv(dev);
> + struct rte_eth_link dev_link;
> + struct ifreq ifr;
> + int ret;
> +
> + if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
> + WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
> + return -1;
> + }
> + memset(&dev_link, 0, sizeof(dev_link));
> + dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> + (ifr.ifr_flags & IFF_RUNNING));
> +
> + dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> + ETH_LINK_SPEED_FIXED);
> +
> + ret = mlx5_link_update_unlocked_gs(priv, &ifr, &dev_link);
> + if (ret)
> + ret = mlx5_link_update_unlocked_gset(priv, &ifr, &dev_link);
> +
> + if (ret)
> + return ret;
> +
> + if (memcmp(&dev_link, &dev->data->dev_link, sizeof(dev_link))) {
> + /* Link status changed. */
> + dev->data->dev_link = dev_link;
> + return 0;
> + }
> + /* Link status is still the same. */
> + return -1;
This function does not respect the PMD coding style, please remove the
blank lines between the code.
Thanks,
--
Nélio Laranjeiro
6WIND
