From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 551301B6A7 for ; Fri, 3 Nov 2017 14:06:17 +0100 (CET) Received: by mail-wm0-f68.google.com with SMTP id r68so1409700wmr.1 for ; Fri, 03 Nov 2017 06:06:17 -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:in-reply-to; bh=LIma2ccQvAtfhqBZbJew7gQBrNnE8WANOEDj/cdznDo=; b=SYFoUHz/y15U3Hj6fkcwVYx0VkJzXTbP+EIlwkpRNm9m+GtKmMeV79l1cGErrev631 6WBy74YaXMvzfvxVvlw6tdXH9OWXDp4UjgxoTCz9pXFyhCxsl5p9pI12jNqOyh5UuNwK mkHM7oumX9vZYrntQWU03h9Rql8kPui9/KbwFGoRqmCg8SY4qYLeV2jnwDKVRloZZjmD B12nbVyLX5EFJM19fwx1Mdv2aOn0PHldVK+lxLwsdS19yHOkyQcWdQSOY96JZRY7bZ1l hMNCGnwpo8rvaB51bhZoxcD2xkJrJTwUhDhCpLETDRa95ivRWZ6ajeYrtDYAR2tTsQar rDhQ== 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=LIma2ccQvAtfhqBZbJew7gQBrNnE8WANOEDj/cdznDo=; b=O+y8+ppRjBLQTJkb/rt7679YFP91rNdRUkwPgx6U5lHFUsZdxieMKUakCEQuX17dLS syEy2SnNne1iwO3ihQBr+Z8VeYo2oMlBEQsAN8UseLFzsykNji0Jo4qND9XKkm9tLC3H M935IHiVVamximtABWFuJ5jg58UGHo+5ClWHdEhRo5fOXG7Se3uZ7hTzEtk7YF0IcNmm t9hj/r8J+OCPOuL7i3edqqDCssRJmJPqTc7xdtvuWAmbIsrTxVjOmWINqQFivZ6I8+Rr ReiGz2tm1DLZtDkdpVLDz/CQzH+mNSFni4DiWmuboocyFanAialZRCg+tNXOZuQEquxR mdRg== X-Gm-Message-State: AMCzsaXJGKqV5inY6v3BlAeItBXwkXno+L0wBfhUnsBsLpVQjFDsLvbB SpoTaCAE1oCJOvehC2HFJt9rcw== X-Google-Smtp-Source: ABhQp+Tj7xhNAnq6M+1qbX9tdpbCrvtmymOgKVDBOVcfp6SNWCgAXha7+pEQZn0T8QPuXezF/y97UA== X-Received: by 10.80.148.44 with SMTP id p41mr8795689eda.171.1509714376947; Fri, 03 Nov 2017 06:06:16 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id h2sm5166296edc.89.2017.11.03.06.06.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 03 Nov 2017 06:06:16 -0700 (PDT) Date: Fri, 3 Nov 2017 14:06:05 +0100 From: Adrien Mazarguil To: Matan Azrad Cc: Gaetan Rivet , dev@dpdk.org Message-ID: <20171103130605.GO24849@6wind.com> References: <1509637324-13525-1-git-send-email-matan@mellanox.com> <1509637324-13525-4-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1509637324-13525-4-git-send-email-matan@mellanox.com> Subject: Re: [dpdk-dev] [PATCH 3/3] net/mlx5: adjust removal error 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, 03 Nov 2017 13:06:17 -0000 On Thu, Nov 02, 2017 at 03:42:04PM +0000, Matan Azrad wrote: > Fail-safe PMD expects to get -ENODEV error value if sub PMD control > command fails because of device removal. > > Make control callbacks return with -ENODEV when the device has > disappeared. > > Signed-off-by: Matan Azrad In short I have the same comments as on the mlx4 patch about usage consistency, this also applies to mlx5; mlx5_removed() should be only used by the public callbacks from struct eth_dev_ops. There's an additional difficulty with this PMD, you need to take into account the fact it provides secondary process support (mlx5_dev_sec_ops). I think secondary processes do not have any IBV context available for mlx5_removed() to query, which should resolve to a no-op in this case. Make sure secondary processes do not crash whatever happens. See below for coding style and other issues. > --- > drivers/net/mlx5/mlx5.h | 1 + > drivers/net/mlx5/mlx5_ethdev.c | 39 +++++++++++++++++++++++++++++++++++---- > drivers/net/mlx5/mlx5_flow.c | 2 ++ > drivers/net/mlx5/mlx5_rss.c | 4 ++++ > drivers/net/mlx5/mlx5_rxq.c | 12 ++++++++++-- > drivers/net/mlx5/mlx5_stats.c | 6 +++++- > drivers/net/mlx5/mlx5_txq.c | 2 ++ > 7 files changed, 59 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h > index e6a69b8..0dd104a 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -208,6 +208,7 @@ int mlx5_ibv_device_to_pci_addr(const struct ibv_device *, > int mlx5_set_link_up(struct rte_eth_dev *dev); > void priv_dev_select_tx_function(struct priv *priv, struct rte_eth_dev *dev); > void priv_dev_select_rx_function(struct priv *priv, struct rte_eth_dev *dev); > +int mlx5_removed(const struct priv *priv); > > /* mlx5_mac.c */ > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c > index c31ea4b..bf61cd6 100644 > --- a/drivers/net/mlx5/mlx5_ethdev.c > +++ b/drivers/net/mlx5/mlx5_ethdev.c > @@ -394,6 +394,8 @@ struct priv * > > ret = priv_sysfs_write(priv, name, value_str, (sizeof(value_str) - 1)); > if (ret == -1) { > + if (mlx5_removed(priv)) > + errno = ENODEV; > DEBUG("cannot write %s `%s' (%lu) to sysfs: %s", > name, value_str, value, strerror(errno)); > return -1; > @@ -925,13 +927,17 @@ struct priv * > { > struct utsname utsname; > int ver[3]; > + int ret; > > 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, wait_to_complete); > - return mlx5_link_update_unlocked_gs(dev, wait_to_complete); > + ret = mlx5_link_update_unlocked_gset(dev, wait_to_complete); > + ret = mlx5_link_update_unlocked_gs(dev, wait_to_complete); Besides the extra space after "ret =", I think this doesn't work as intended. A "else" statement is necessary. > + if (ret && mlx5_removed(mlx5_get_priv(dev))) > + return -ENODEV; > + return ret; > } > > /** > @@ -978,6 +984,8 @@ struct priv * > strerror(ret)); > priv_unlock(priv); > assert(ret >= 0); > + if (mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > > @@ -1029,6 +1037,8 @@ struct priv * > out: > priv_unlock(priv); > assert(ret >= 0); > + if (mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > > @@ -1083,6 +1093,8 @@ struct priv * > out: > priv_unlock(priv); > assert(ret >= 0); > + if (mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > > @@ -1364,13 +1376,13 @@ struct priv * > if (up) { > err = priv_set_flags(priv, ~IFF_UP, IFF_UP); > if (err) > - return err; > + return errno == ENODEV ? -ENODEV : err; There is a documentation issue here since the mlx5 PMD didn't get all the errno consistency fixes that mlx4 got, however err is documented as being -1 in case of error, whereas priv_dev_set_link() returns a positive errno value instead and mlx5_set_link_down/up() should return only negative errno values but are documented as returning positive ones. Anyway to keep it short: currently in mlx5, priv_*() => positive errno and the public-facing mlx5_*() => negative errno, hence you should return a positive ENODEV here. You could avoid this mess by patching the public callbacks only and not internal functions like this one. > priv_dev_select_tx_function(priv, dev); > priv_dev_select_rx_function(priv, dev); > } else { > err = priv_set_flags(priv, ~IFF_UP, ~IFF_UP); > if (err) > - return err; > + return errno == ENODEV ? -ENODEV : err; Same here. > dev->rx_pkt_burst = removed_rx_burst; > dev->tx_pkt_burst = removed_tx_burst; > } > @@ -1474,3 +1486,22 @@ struct priv * > dev->rx_pkt_burst = mlx5_rx_burst; > } > } > + > +/** > + * Check if mlx5 device was removed. > + * "mlx5" is redundant. As with mlx4, a short paragraph should describe where this function is supposed to be used. > + * @param priv > + * Pointer to private structure. > + * > + * @return > + * -ENODEV when device is removed and rte_errno is set, otherwise 0. > + */ > +int > +mlx5_removed(const struct priv *priv) > +{ > + struct ibv_device_attr device_attr; > + > + if (ibv_query_device(priv->ctx, &device_attr) == EIO) > + return -(rte_errno = ENODEV); Coding rules prohibit this kind of affectation, see mlx4 comments. > + return 0; > +} > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > index 5f49bf5..448c0a3 100644 > --- a/drivers/net/mlx5/mlx5_flow.c > +++ b/drivers/net/mlx5/mlx5_flow.c > @@ -3068,6 +3068,8 @@ struct rte_flow * > priv_lock(priv); > ret = priv_fdir_ctrl_func(priv, filter_op, arg); > priv_unlock(priv); > + if (ret && mlx5_removed(priv)) > + ret = ENODEV; > break; > default: > ERROR("%p: filter type (%d) not supported", > diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c > index f3de46d..1ad9269 100644 > --- a/drivers/net/mlx5/mlx5_rss.c > +++ b/drivers/net/mlx5/mlx5_rss.c > @@ -250,6 +250,8 @@ > priv_lock(priv); > ret = priv_dev_rss_reta_query(priv, reta_conf, reta_size); > priv_unlock(priv); > + if (ret && mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > > @@ -282,5 +284,7 @@ > mlx5_dev_stop(dev); > mlx5_dev_start(dev); > } > + if (ret && mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c > index a1f382b..c9a549d 100644 > --- a/drivers/net/mlx5/mlx5_rxq.c > +++ b/drivers/net/mlx5/mlx5_rxq.c > @@ -278,6 +278,8 @@ > (*priv->rxqs)[idx] = &rxq_ctrl->rxq; > out: > priv_unlock(priv); > + if (mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > > @@ -485,8 +487,11 @@ > } > exit: > priv_unlock(priv); > - if (ret) > + if (ret) { > WARN("unable to arm interrupt on rx queue %d", rx_queue_id); > + if (mlx5_removed(priv)) > + return -ENODEV; > + } > return -ret; > } > > @@ -537,9 +542,12 @@ > if (rxq_ibv) > mlx5_priv_rxq_ibv_release(priv, rxq_ibv); > priv_unlock(priv); > - if (ret) > + if (ret) { > WARN("unable to disable interrupt on rx queue %d", > rx_queue_id); > + if (mlx5_removed(priv)) > + return -ENODEV; > + } > return -ret; > } > > diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c > index 5e225d3..33b2a60 100644 > --- a/drivers/net/mlx5/mlx5_stats.c > +++ b/drivers/net/mlx5/mlx5_stats.c > @@ -438,13 +438,17 @@ struct mlx5_counter_ctrl { > stats_n = priv_ethtool_get_stats_n(priv); > if (stats_n < 0) { > priv_unlock(priv); > - return -1; > + ret = -1; > + goto error; > } > if (xstats_ctrl->stats_n != stats_n) > priv_xstats_init(priv); > ret = priv_xstats_get(priv, stats); > priv_unlock(priv); > } > +error: > + if (ret < 0 && mlx5_removed(priv)) > + return -ENODEV; > return ret; > } > > diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c > index fbb2630..a0101cb 100644 > --- a/drivers/net/mlx5/mlx5_txq.c > +++ b/drivers/net/mlx5/mlx5_txq.c > @@ -186,6 +186,8 @@ > (*priv->txqs)[idx] = &txq_ctrl->txq; > out: > priv_unlock(priv); > + if (mlx5_removed(priv)) > + return -ENODEV; > return -ret; > } > > -- > 1.8.3.1 > -- Adrien Mazarguil 6WIND