From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f180.google.com (mail-wr0-f180.google.com [209.85.128.180]) by dpdk.org (Postfix) with ESMTP id D97687D3A for ; Thu, 24 Aug 2017 09:38:46 +0200 (CEST) Received: by mail-wr0-f180.google.com with SMTP id p8so7201858wrf.5 for ; Thu, 24 Aug 2017 00:38:46 -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=c/B05ZnZXQEmp9GwY5Dc5zMUuc+yzRlnIRuaUdoe7SM=; b=RoXMNhPsx6U0k8QHy0p33vBZy/HMA0HVPIrKdhaFIirZZwx/K/p5O4y9E+sgcTeq0d KtOBpi9TlTe3X7LKBheRfZFO62F8EqLGLFR2H/rQ2TY81dpFYQpkWahXLGh6IdDqRm7D rfZ9uNhzC/QKv6huAmJT5fBAFfhBgppc7G13u3YDtm/Lcw2aVr45Dtyhq6R1DvNGorpP E27ASyyMkNvek6g08bwDqYCr3zuq6dCA8ysFtNJYl5TVNOS8pVw4GLiM3IJkSvarcRm0 hTYV+J0vBb8yvTuszcSoIxNEOHfe4DeOiIp1WGHSBcDeyPo6mocfK1wBfB/KUWSqeYeV rgzw== 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=c/B05ZnZXQEmp9GwY5Dc5zMUuc+yzRlnIRuaUdoe7SM=; b=D3It9PzkMbMmjo/niod7GeGsIp5HTSurtXdo0/Vl8APbotYXqdTAqLoHGHIhqOK3Nq DGy7MFpdCXMXjBY0CxgZLWJoIZFfCS8XqatNi/2s+xqOQT0487tbPxRtrm15FI4Sn+XA 66W24sSDemI8dceoK1U+0virdNWAGaOWSfvS1zVtyxbQ9OpnpHF6n7zYjITC4pKaYBPs mCo41rDWzUvrUQuSN5CbR1cIH4umqEUcRU22UEADXBdpf/0J1P585+UAgkztPHGygE+0 urzj9nZj0242tIIiDvhsnUmsqcazHy6ePeIsWJIngZrCCkQwBXPKJLOwhgj6ojAmSxFE EP4g== X-Gm-Message-State: AHYfb5iFvEXcsBOW3fLl/xhyY2ADOdUT6y8TiJc+dc03WvHHuNwzXt5C l82DIYuPpurCQNbHK+cLpw== X-Received: by 10.223.188.16 with SMTP id s16mr3029273wrg.311.1503560326593; Thu, 24 Aug 2017 00:38:46 -0700 (PDT) Received: from autoinstall.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id l13sm5306172wmd.47.2017.08.24.00.38.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 24 Aug 2017 00:38:46 -0700 (PDT) Date: Thu, 24 Aug 2017 09:38:24 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Matan Azrad Cc: Adrien Mazarguil , "dev@dpdk.org" Message-ID: <20170824073824.GA4544@autoinstall.dev.6wind.com> References: <1502627112-53405-1-git-send-email-matan@mellanox.com> <20170823094037.GS12995@autoinstall.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: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH 1/2] net/mlx5: support device removal event 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, 24 Aug 2017 07:38:47 -0000 On Wed, Aug 23, 2017 at 07:44:45PM +0000, Matan Azrad wrote: > Hi Nelio > > > -----Original Message----- > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com] > > Sent: Wednesday, August 23, 2017 12:41 PM > > To: Matan Azrad > > Cc: Adrien Mazarguil ; dev@dpdk.org > > Subject: Re: [PATCH 1/2] net/mlx5: support device removal event > > > > Hi Matan, > > > > On Sun, Aug 13, 2017 at 03:25:11PM +0300, Matan Azrad wrote: > > > Extend the LSC event handling to support the device removal as well. > > > The Verbs library may send several related events, which are different > > > from LSC event. > > > > > > The mlx5 event handling has been made capable of receiving and > > > signaling several event types at once. > > > > > > This support includes next: > > > 1. Removal event detection according to the user configuration. > > > 2. Calling to all registered mlx5 removal callbacks. > > > 3. Capabilities extension to include removal interrupt handling. > > > > > > Signed-off-by: Matan Azrad > > > --- > > > drivers/net/mlx5/mlx5.c | 2 +- > > > drivers/net/mlx5/mlx5_ethdev.c | 100 > > > +++++++++++++++++++++++++++-------------- > > > 2 files changed, 68 insertions(+), 34 deletions(-) > > > > > > Hi > > > This patch based on top of last Nelio mlx5 cleanup patches. > > > > > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index > > > bd66a7c..1a3d7f1 100644 > > > --- a/drivers/net/mlx5/mlx5.c > > > +++ b/drivers/net/mlx5/mlx5.c > > > @@ -865,7 +865,7 @@ static struct rte_pci_driver mlx5_driver = { > > > }, > > > .id_table = mlx5_pci_id_map, > > > .probe = mlx5_pci_probe, > > > - .drv_flags = RTE_PCI_DRV_INTR_LSC, > > > + .drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV, > > > }; > > > > > > /** > > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c > > > b/drivers/net/mlx5/mlx5_ethdev.c index 57f6237..404d8f4 100644 > > > --- a/drivers/net/mlx5/mlx5_ethdev.c > > > +++ b/drivers/net/mlx5/mlx5_ethdev.c > > > @@ -1112,47 +1112,75 @@ mlx5_ibv_device_to_pci_addr(const struct > > > ibv_device *device, } > > > > > > /** > > > - * Link status handler. > > > + * Update the link status. > > > + * Set alarm if the device link status is inconsistent. > > > > Adding such comment should also comment about the issue this alarm is > > solving i.e. why the link is inconsistent and why the alarm help to fix the > > issue. > > > I didn't see any comments about that in the old code , Hence I didn't write it. Normal as the alarm is a work around specifically necessary to Mellanox PMD. Now you explicitly announce that this function program an alarm, the question is why is it necessary? > I think you right and this could be added.(even before this patch). No, in the current code, it update the link, if it inconsistent it tries to have a link correct ASAP. There is no need to inform this function will program an alarm, it is internal cooking. > > > * > > > * @param priv > > > * Pointer to private structure. > > > - * @param dev > > > - * Pointer to the rte_eth_dev structure. > > > * > > > * @return > > > - * Nonzero if the callback process can be called immediately. > > > + * Zero if alarm is not set and the link status is consistent. > > > */ > > > static int > > > -priv_dev_link_status_handler(struct priv *priv, struct rte_eth_dev > > > *dev) > > > +priv_link_status_alarm_update(struct priv *priv) > > > > The old name is more accurate, the fact we need to program an alarm is a > > work around to get the correct status from ethtool. If it was possible to avoid > > it, this alarm would not exists. > > > Probably because of the git +- format and this specific patch you got confuse here. No I applied your patch and read your code. You did not understand my comment. >[...] When I read: > void > mlx5_dev_link_status_handler(void *arg) > { > struct rte_eth_dev *dev = arg; > struct priv *priv = dev->data->dev_private; > int ret; > > priv_lock(priv); > assert(priv->pending_alarm == 1); > priv->pending_alarm = 0; > - ret = priv_dev_link_status_handler(priv, dev); > + ret = priv_link_status_alarm_update(priv); > priv_unlock(priv); > - if (ret) > + if (!ret) > _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL, > - NULL); > + NULL); > } I am expecting to find something related to a link update, what I see is an alarm update. I don't expect to update an alarm but a link. The names and action are inconsistent i.e. mlx5_dev_link_status_handler() should handle a link not an alarm. I understand there is a need to add more function levels, but the priv_link_status_alarm_update() should be renamed to something like priv_link_status_update(). Regards, -- Nélio Laranjeiro 6WIND