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 BEEA91B293 for ; Fri, 16 Feb 2018 11:49:13 +0100 (CET) Received: by mail-wr0-f195.google.com with SMTP id 34so2418662wre.13 for ; Fri, 16 Feb 2018 02:49:13 -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:in-reply-to; bh=c1sxs21t0ay1F5MGcBO4M7Zff1i7lsTGCgxshXlQj54=; b=gNr3Q6gTF6vCwhIaG3xyPhqf8mkDkNPlnebkW0h8C4dbzDvRMZTieQkweBQm9sfnSm iGS6WHuq5jR8/bdctaRcN6IVdrSz7M4HnvszURXksuiftxML5a5DrRbTbX85rUeFBUMD LH3GTDkMTcRK01fgj9LkYs4U2rWXeUTtCa/5hyn17cziTOo8N8Q/ebZ6h6NXggSQE6/F iIe5s0OnVfWSTghyv3r5o44doil/VbyeAiIvM112XBTydsKtO1Ly/POROlhggSFHe4Bh ShnQlGHaHlxo6W7UdGDHkEgcvPSCSejslBGaeCvwgqrMzla2gMwzRHYJvVxJVw4qOyG0 5Q/w== 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:in-reply-to; bh=c1sxs21t0ay1F5MGcBO4M7Zff1i7lsTGCgxshXlQj54=; b=hEbyXUycZOTc15GgnLPa0XB1sYSxijwaFy65Y2IrSt1E4OzfRY6lI/5jUAbSTI5tWa jexEVDJlotctVTmMiLFpjYMOkPgQ8AOEYVwuAOmVQLGsS6D4A2oGwxmttN3MLvhxPnoV 4zSpnM3GOL/0tzZGBj5b2dn2kFaXcMRqt2+47bLtw7a9y2Q3vCU80zCFD0f0mu46zQkp FvtpTlXSlLPe4oFYehD+92z2vArpVrxsHhysW8oPtTZjA2UloI8stoIrLbX56Ddo0Sjc BbpjmMfQ+z4ajfV1ISO1zMXV28sTNxNPq8cWWo/rtuSyqkL7vsplwb/owazNpsxbQ/N5 e6rw== X-Gm-Message-State: APf1xPBD0IleXX6hP62xyAO2sV7VQefgztUm9mn1IXpqnowY17Mlt8Rr 6BkdR6vGQWRrXaOLeM3EGr3hlO9M X-Google-Smtp-Source: AH8x2252ji3VhFvS83s+o0hDxUCcxdEVrSoFM/CZGACFfsDAR+WL8Vf7ER7Gore5A2ssTvYFt4c8vg== X-Received: by 10.223.133.235 with SMTP id 40mr5016744wru.275.1518778153454; Fri, 16 Feb 2018 02:49:13 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 63sm13291060wmd.17.2018.02.16.02.49.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Feb 2018 02:49:12 -0800 (PST) Date: Fri, 16 Feb 2018 11:49:00 +0100 From: Adrien Mazarguil To: Nelio Laranjeiro Cc: dev@dpdk.org, Yongseok Koh Message-ID: <20180216104900.GC4256@6wind.com> References: <21fb91002768a627d9c7f3d81e0c8a12fbf6811f.1518684427.git.nelio.laranjeiro@6wind.com> <16c653c55e2144291ce92c9696ffaa829a70a028.1518684427.git.nelio.laranjeiro@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <16c653c55e2144291ce92c9696ffaa829a70a028.1518684427.git.nelio.laranjeiro@6wind.com> Subject: Re: [dpdk-dev] [PATCH 2/2] net/mlx5: fix link status to use wait to complete 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: Fri, 16 Feb 2018 10:49:13 -0000 On Thu, Feb 15, 2018 at 09:47:28AM +0100, Nelio Laranjeiro wrote: > Wait to complete is present to let the application get a correct status > when it requires it, it should not be ignored. > > Fixes: cb8faed7dde8 ("mlx5: support link status update") > Cc: adrien.mazarguil@6wind.com > > Signed-off-by: Nelio Laranjeiro Several comments, please see below. > --- > drivers/net/mlx5/mlx5_ethdev.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c > index 0ce9f438a..112cc2a40 100644 > --- a/drivers/net/mlx5/mlx5_ethdev.c > +++ b/drivers/net/mlx5/mlx5_ethdev.c > @@ -502,11 +502,12 @@ mlx5_dev_supported_ptypes_get(struct rte_eth_dev *dev) > * > * @param dev > * Pointer to Ethernet device structure. > - * @param wait_to_complete > - * Wait for request completion (ignored). > + * > + * @return > + * 0 on success, -1 otherwise. See below regarding the return value. > */ > static int > -mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev, int wait_to_complete) > +mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev) > { > struct priv *priv = dev->data->dev_private; > struct ethtool_cmd edata = { > @@ -518,7 +519,6 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev, int wait_to_complete) > > /* priv_lock() is not taken to allow concurrent calls. */ > > - (void)wait_to_complete; > if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) { > WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno)); > return -1; > @@ -568,11 +568,9 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev, int wait_to_complete) > * > * @param dev > * Pointer to Ethernet device structure. > - * @param wait_to_complete > - * Wait for request completion (ignored). You should use this opportunity to document its return value as well. > */ > static int > -mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete) > +mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev) > { > struct priv *priv = dev->data->dev_private; > struct ethtool_link_settings gcmd = { .cmd = ETHTOOL_GLINKSETTINGS }; > @@ -580,7 +578,6 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete) > struct rte_eth_link dev_link; > uint64_t sc; > > - (void)wait_to_complete; > if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) { > WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno)); > return -1; > @@ -708,6 +705,9 @@ priv_link_stop(struct priv *priv) > * Pointer to private structure. > * @param wait_to_complete > * Wait for request completion (ignored). > + * > + * @return > + * 0 on success, negative value otherwise. How about "0 on success, negative errno value otherwise" according to subsequent comments. > */ > int > priv_link_update(struct priv *priv, int wait_to_complete) > @@ -720,10 +720,16 @@ priv_link_update(struct priv *priv, int wait_to_complete) > current_version >= KERNEL_VERSION(4, 9, 0) : > 0; > > - if (use_new_api) > - ret = mlx5_link_update_unlocked_gset(dev, wait_to_complete); > - else > - ret = mlx5_link_update_unlocked_gs(dev, wait_to_complete); > + do { > + if (use_new_api) > + ret = mlx5_link_update_unlocked_gset(dev); > + else > + ret = mlx5_link_update_unlocked_gs(dev); > + if (ret == -1) > + return -EAGAIN; Like for system calls, EAGAIN isn't an acceptable error in case wait_to_complete (blocking mode) is requested. It must be some other reason. This check should be replaced with my next suggestion. > + } while(wait_to_complete && !ret); Missing space after "while". One issue is when wait_to_complete is enabled and link status never settles down due to bad cabling or buggy SFP. I think this function should give up and return an error after a while (not -EAGAIN in this case but -EIO, -EBUSY or even -EINTR to reflect the call had to be interrupted due to HW trouble). You could use MLX5_MAX_LINK_QUERY_ATTEMPTS for that, e.g.: int attempt = MLX5_MAX_LINK_QUERY_ATTEMPTS; [...] while (1) { [...] if (!wait_to_complete || ret != -EAGAIN || !attempt--) break; sleep(1); } > + if (ret == -EAGAIN) > + return ret; Since neither function may return anything other than -1 in case of error at the moment and since their wait_to_complete argument is being removed, I suggest to make them properly non-blocking by default (i.e. O_NONBLOCK on their ioctl() FD), then return -errno in case of error on intermediate system calls, then the above check will make sense. > /* If lsc interrupt is disabled, should always be ready for traffic. */ > if (!dev->data->dev_conf.intr_conf.lsc) { > priv_link_start(priv); > @@ -755,10 +761,11 @@ int > priv_force_link_status_change(struct priv *priv, int status) > { > int try = 0; > + int ret = 0; > > while (try < MLX5_MAX_LINK_QUERY_ATTEMPTS) { > - priv_link_update(priv, 0); > - if (priv->dev->data->dev_link.link_status == status) > + ret = priv_link_update(priv, 0); > + if (!ret && priv->dev->data->dev_link.link_status == status) > return 0; > try++; > sleep(1); I think this function could be removed in the same patch (with e313ef4c2fe8 "net/mlx5: fix link state on device start" partially reverted) since a call to priv_link_update(priv, 1) would now result in the same behavior. -- Adrien Mazarguil 6WIND