From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id A101A1B022 for ; Wed, 3 Jan 2018 08:34:07 +0100 (CET) Received: by mail-wr0-f195.google.com with SMTP id u19so667815wrc.3 for ; Tue, 02 Jan 2018 23:34:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=SHvTeA77ICFn2IYsgtO8arEDZSGLRx7hquG4w+3bWUM=; b=JI5OcDXZUN1cJT/ClEKml4yEcWomiABCSa5hPRT2M8tBKd96Wbmj3Y9l4wxQs5McO4 nh+Th8zbVL+kDkilXFd2KxVPf723AW2N5aRUuRxlF6q7mbHZtgx1jBdlRGsqWY+KGNWu drlgGRELiNH6ZczUOOHLHZBp5aWxeeWsv2srVg8T40jTN4/fPcyobUJV7sTqXjqYDao5 3AuJ9Kr0e9n6wd88MYtqpjq+cx1wnd97+NuhezzxyruPq44c/O33kqwuJShdV2f5VKv/ 7yJSyTQYiCShg8vPk+eTenRDYDvZIjc+WK81ot1aviGbDyXdMyzqv7BBY01/XG7/qOJI MYlg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=SHvTeA77ICFn2IYsgtO8arEDZSGLRx7hquG4w+3bWUM=; b=pYsY+18UsrdTaZwDvye+ldzAgPc5D2bzm9teIj2X9opGb70enrZwcL7dCeegB8jJqK P0T2Y45vdXyadH0AVUTZnsLFsZr9bT+rOYfuFIS+GLp4+Hum39h3UG/NTzV0ANFCmhv+ iYYfh8/kPQ2JWF1BMZTUoEoFvZatKVZi0zS7h7BU4PFUwN28Fxtae4Gaw6HZCM6QAbE3 H1vrEmKRfPEX6MW3ajxB23xkrcR9PX4N/tx0+JUVqX9aufGSXYRUl9tLjYKeqWy4TyJO nh+cexRaW4UO0nZ2woThQzcR+Sdjwp932D98Pts7YslADYoIkvIBHZKZ2vW54vSr1Mj9 rowQ== X-Gm-Message-State: AKGB3mK3tzjH8xu6DH7XO5RfWTlhxkiyNeLntPa/+PvBwxi11aqCaWrL h3WFjLqdDksE32tNxZ8A3Llg X-Google-Smtp-Source: ACJfBouoW1svGl9Ek17ncfrci1UvT+6FZfPrUYy1+leQzq+yuqQvCVSY0tN3iQ25Py81Cvbzfvy5BA== X-Received: by 10.223.195.99 with SMTP id e32mr619267wrg.10.1514964847115; Tue, 02 Jan 2018 23:34:07 -0800 (PST) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id q74sm1151483wmg.22.2018.01.02.23.34.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 02 Jan 2018 23:34:06 -0800 (PST) Date: Wed, 3 Jan 2018 08:33:19 +0100 From: Nelio Laranjeiro To: Stephen Hemminger Cc: adrien.mazarguil@6wind.com, yskoh@mellanox.com, dev@dpdk.org Message-ID: <20180103073319.suvlicfbqa6llq5e@laranjeiro-vm.dev.6wind.com> References: <20180102205310.3586-1-stephen@networkplumber.org> <20180102205310.3586-3-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180102205310.3586-3-stephen@networkplumber.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 2/2] mlx5: don't depend on kernel version X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Jan 2018 07:34:07 -0000 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 > --- > 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 > #include > #include > -#include > #include > #include > #include > @@ -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