From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 9E8C51BD9D for ; Thu, 5 Apr 2018 08:51:15 +0200 (CEST) Received: by mail-wm0-f44.google.com with SMTP id f125so3704356wme.4 for ; Wed, 04 Apr 2018 23:51:15 -0700 (PDT) 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=b1IjLRpL8rLo92xZNHgdn9qe/mP9uA9e2z7bbke1iSU=; b=v6J4lk+vDargPouDwkYAltqiGPmjbQV9sp1vQEnioTLD+QMUtoLozfhfw4KLQ9pOvD uv82X+xMNxoAGhChxafnBfLFpBrMqG5rVBQ7JTuG90dkIFh2rfpxM+7FJl6KK0qoqqVD 5fEd4XPH4icEIHQJjkoYr0vcZw1sUjnfDX7t5QrNSOskgPS9A6zAH7USbs/5KuJMem/D TuH3afdjPAq1ZDD+haYWOG1E+JhrnsvfCg64xWc1dZ9oZrzw8oZUrYZJmSl0TrBFLOgk Ts0oCvRhs18ybEtxH0YhJS9NpsqI0tSs+8brKYwPQux+tr+qEA2LgKCDgj9PhKYRD9pX 1OUA== 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=b1IjLRpL8rLo92xZNHgdn9qe/mP9uA9e2z7bbke1iSU=; b=mZzsHtJRpz+h7LIJvGN1Gj+crSEN7MknVkeMF/aRUaQg9ogk19Axm1hCJu2HzoSgCF b6kr+32Hhnh4YW2TymvvA2eSqVARH1fA6FKUwFrVpuVeoWk1SwWjCrrx5aHLqXOhENIz Nt0F5SiFyXX6Ap5NMHXgL2GL0MkFgcuC3r694v6WX+38pUz3FpSVoVQXHtWyguYlH3ZV AwCgPAvaB9HmD2JDKyuFix8iIV5K2bfyvOb70ZX0QAsmgkv6yEO25YucGy5mGvuJvv70 dPK2OGDC+0OMt5mGVWn94wcyzgyD2X03b0hz/HS5hef4A2nGmDXh+w38vRRde09Ykzsr LItQ== X-Gm-Message-State: ALQs6tCrXDaT5xJJnXcuhb9IV3NnrG14RdKwJKmxm/eFBAW0GT1blduD jEbwhr1bISUP+nupS1V/sxIkddrvZw== X-Google-Smtp-Source: AIpwx4/dx9VfqZ3j2gy621F8Nt46u/PYvF12x5sTc8N4ek0u7PUx6F4kQECgmWSuuXrwWXxYdJQLrQ== X-Received: by 10.28.241.18 with SMTP id p18mr10192563wmh.40.1522911075062; Wed, 04 Apr 2018 23:51:15 -0700 (PDT) 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 z10sm7728921wre.58.2018.04.04.23.51.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 04 Apr 2018 23:51:14 -0700 (PDT) Date: Thu, 5 Apr 2018 08:51:20 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Shahaf Shuler Cc: Adrien Mazarguil , Yongseok Koh , "dev@dpdk.org" Message-ID: <20180405065120.jqttkvlhjflpfdbj@laranjeiro-vm.dev.6wind.com> References: <20180403044817.27457-1-shahafs@mellanox.com> <20180404073009.zgqu3yrj26trwdfr@laranjeiro-vm.dev.6wind.com> <20180404121051.ersiyf75gykwfon5@laranjeiro-vm.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization 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: Thu, 05 Apr 2018 06:51:15 -0000 On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote: > Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro: > > Subject: Re: [PATCH] net/mlx5: fix link status initialization > > > > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote: > > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro: > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization > > > > > > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote: > > > > > Following commit 7ba5320baa32 ("net/mlx5: fix link status > > > > > behavior") > > > > > > > > > > The initial link status is no longer set as part of the port start. > > > > > This may cause application to query the link as down while in fact > > > > > it was already up before the DPDK application start. > > > > > > > > There is something wrong in this explanation, the application should > > > > query the link using this same callback, why the PMD should call it? > > > > > > It is how ethdev is implemented. The application is doing nothing > > > wrong, it queries the link status using rte_eth_link_get_nowait > > > > > > When the application works with LSC interrupts the ethdev layer skips > > > the PMD callback and just update according to the link status exists > > > on device data. > > > It is because it assumes the link status on the device data is the > > > correct one since any link change is processed by the application. > > > > > > The issue is with the initial state of the link. If the link is > > > already up when the PMD starts there will be no callback for the > > > application. > > > > > > I think this logic is OK, and it is also a good practice to initialize > > > the link status to the actual state of the link as part of the port > > > probing. > > > > The commit log should be re-worded to include this explanation. > > Will add. > > > > > > > > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior") > > > > > Cc: nelio.laranjeiro@6wind.com > > > > > > > > > > Signed-off-by: Shahaf Shuler > > > > > Acked-by: Yongseok Koh > > > > > --- > > > > > drivers/net/mlx5/mlx5.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c > > > > > index > > > > > 7d58d66bb9..f954ea2862 100644 > > > > > --- a/drivers/net/mlx5/mlx5.c > > > > > +++ b/drivers/net/mlx5/mlx5.c > > > > > @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv > > > > __rte_unused, > > > > > DRV_LOG(DEBUG, "port %u forcing Ethernet interface up", > > > > > eth_dev->data->port_id); > > > > > mlx5_set_link_up(eth_dev); > > > > > + mlx5_link_update(eth_dev, 1); > > > > > /* Store device configuration on private structure. */ > > > > > priv->config = config; > > > > > continue; > > > > > -- > > > > > 2.12.0 > > > > According to your analysis, this is only necessary when the LCS is configured > > in the device. Why not adding this call to > > mlx5_dev_interrupt_handler_install() which is responsible to install the LCS > > callback. > > I think it is good practice whether or not LSC is set. > The link status should be initialized to the correct value after the probe. There is no guarantee the link will be accurate, at probe time the link may be up so internal information has a status up with a speed with this patch. The application probes a second port, in the mean time the link of the first port goes down, the interrupt is still not installed and the internal status becomes wrong (still up whereas the port is down). Finally at start, the device installs the handler, but the link is still down whereas internally it is up, the application will call rte_eth_link_get_nowait() which will directly copy the wrong internal status to the application. There is also another situation, when the application stops the port, the interrupt is also removed, which means during this time, the internal status may be wrong as it won't be updated anymore. This comes to the same possible situation as above. > > Another point, the wait to complete flag is useless, if the link is up, the status > > and speed will be accurate, if not, it will receive an LSC event later. > > Agree. > > So how about keeping the code on the current place, just removing the > wait_to_complete? The current place is not fixing the issue as there is still a possibility to have a wrong value. Regards, -- Nélio Laranjeiro 6WIND