From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0070.outbound.protection.outlook.com [104.47.0.70]) by dpdk.org (Postfix) with ESMTP id D14231B2F5 for ; Sun, 5 Nov 2017 07:57:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=ID1l6YZGAcy5ll+lRANY3qYpqhJMEB2fKtvD8xilTRE=; b=jn37btQzYnrZG7Gyy3MFZkVWpTboWrbPcrNAJgEks/eokbZwq26owy9vJjQ+jDp+ULKbLJ443xF6EgtRdgK1Zjujh98l+ng+NgAPKQ+LXIBFvv6C+Fwp3HVInv+Fyk6bQPtv9cbgFKoGf02hYmnfTOd4LlxhrWIspAFn9yp17b8= Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com (10.167.127.17) by HE1PR0502MB3657.eurprd05.prod.outlook.com (10.167.127.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.197.13; Sun, 5 Nov 2017 06:57:40 +0000 Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com ([fe80::c524:908c:b99c:3f4b]) by HE1PR0502MB3659.eurprd05.prod.outlook.com ([fe80::c524:908c:b99c:3f4b%13]) with mapi id 15.20.0197.017; Sun, 5 Nov 2017 06:57:40 +0000 From: Matan Azrad To: Adrien Mazarguil CC: Gaetan Rivet , "dev@dpdk.org" Thread-Topic: [PATCH 3/3] net/mlx5: adjust removal error Thread-Index: AQHTVKSK03QbaEQ5EkOd9PCEXbR6O6MFXEiQ Date: Sun, 5 Nov 2017 06:57:40 +0000 Message-ID: References: <1509637324-13525-1-git-send-email-matan@mellanox.com> <1509637324-13525-4-git-send-email-matan@mellanox.com> <20171103130605.GO24849@6wind.com> In-Reply-To: <20171103130605.GO24849@6wind.com> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; HE1PR0502MB3657; 6:lMt/z/bF2msrbdl5TJTrz5I93zKrR6YAXIh8mQg9WS63v8MTOlzdM0tH7y4sTYCqwN7wOAROHY8JOUZNP/K6alRt9JOHz7DA9NGItY5CS9/Idc9dd96UQG2J1b5KFmRWQrrmV+51NQlswRpmvzA5TBOn3+JmNAK5sLT8ZY+b96UxRISIaSmlzztsx/Ivd9z355A5yanStiStHGGy4wPZcOVukF7OIWiK1y98lHdkHNF4q1jFa6GOf3gocgDrnzZNPXwDfTehGIZLbuRpr58XETKnXdt5kCYAlz0oYyVfI9gxfsdYweKxXJ5gNbUch/jUoMhEmeFGp/GM8CXMPDqkj/duaVw6nQiwQCGf3Hr52oI=; 5:G5Aq9msYGXRapUqiz9pDT+9SJf9DIFoSvt7SUUF9rn+vghGPpbKYBWLEhn9as0UbeHnL3fJOdbo2O7ymGZvlb5VySJAdpCjHWcahiwY9NaOw+Jz99QrstaLmydLbjvarBOr/gbY+Kx5O3aB1IeXMVHONvVr0Mt8WLWoI6FqkAhE=; 24:zPgnm8ASriWoWspYKGKQzhWA4K9GNbDAG4TppfBenP4iCDwwOpYugzCP/FbSN08VKIrrGbxI91zPQloVY/Ti8eKhsJXYSl56sejxltKt6Y8=; 7:0sEXyAilpepfrsBHxrcdup0eiYonx8VHgla3mlTbKXncYUKaMsuYsCxgIp+N9erJPTlAg4kipd4KU1PRi3nNRtH1KbwYXZdpap2X5YpGK0HNOr9M3m9Am74wT+hpAMzymEZQWiqbf55pIs/vNns4O1VeiJPTYDA51SVHIRxQ9maaODq3pCqk+MSTle2GuNCzXMHHMXV2gr+U9YCE24uLrvExPRjv8F9OTJJ/nfRB7S+46lYvOfDjS3ACtAk9VVNJ x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 027bcbd1-8b82-466b-ee21-08d5241a8198 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(48565401081)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(2017052603199); SRVR:HE1PR0502MB3657; x-ms-traffictypediagnostic: HE1PR0502MB3657: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-exchange-antispam-report-test: UriScan:; x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3002001)(100000703101)(100105400095)(3231021)(6055026)(6041248)(20161123555025)(20161123558100)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123564025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:HE1PR0502MB3657; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:HE1PR0502MB3657; x-forefront-prvs: 04825EA361 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(346002)(376002)(189002)(13464003)(199003)(24454002)(7736002)(2900100001)(2906002)(106356001)(33656002)(478600001)(81166006)(101416001)(81156014)(68736007)(74316002)(14454004)(66066001)(105586002)(9686003)(4326008)(305945005)(55016002)(5660300001)(7696004)(53936002)(6916009)(2950100002)(8936002)(25786009)(8676002)(6506006)(6436002)(102836003)(6116002)(3846002)(6246003)(3280700002)(229853002)(5250100002)(50986999)(53546010)(189998001)(316002)(54356999)(3660700001)(97736004)(54906003)(76176999)(575784001)(86362001)(99286004); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0502MB3657; H:HE1PR0502MB3659.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 027bcbd1-8b82-466b-ee21-08d5241a8198 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Nov 2017 06:57:40.4357 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0502MB3657 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: Sun, 05 Nov 2017 06:57:42 -0000 Hi Adrien, Thanks for this too. > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Friday, November 3, 2017 3:06 PM > To: Matan Azrad > Cc: Gaetan Rivet ; dev@dpdk.org > Subject: Re: [PATCH 3/3] net/mlx5: adjust removal error >=20 > 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 >=20 > 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 use= d > by the public callbacks from struct eth_dev_ops. >=20 > There's an additional difficulty with this PMD, you need to take into acc= ount > 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. >=20 Will check it, thanks! > See below for coding style and other issues. >=20 > > --- > > 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 =3D priv_sysfs_write(priv, name, value_str, (sizeof(value_str) - = 1)); > > if (ret =3D=3D -1) { > > + if (mlx5_removed(priv)) > > + errno =3D 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) =3D=3D -1 || > > sscanf(utsname.release, "%d.%d.%d", > > &ver[0], &ver[1], &ver[2]) !=3D 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 =3D mlx5_link_update_unlocked_gset(dev, > wait_to_complete); > > + ret =3D mlx5_link_update_unlocked_gs(dev, wait_to_complete); >=20 > Besides the extra space after "ret =3D", I think this doesn't work as int= ended. A > "else" statement is necessary. >=20 Will fix it, thanks! > > + 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 >=3D 0); > > + if (mlx5_removed(priv)) > > + return -ENODEV; > > return -ret; > > } > > > > @@ -1029,6 +1037,8 @@ struct priv * > > out: > > priv_unlock(priv); > > assert(ret >=3D 0); > > + if (mlx5_removed(priv)) > > + return -ENODEV; > > return -ret; > > } > > > > @@ -1083,6 +1093,8 @@ struct priv * > > out: > > priv_unlock(priv); > > assert(ret >=3D 0); > > + if (mlx5_removed(priv)) > > + return -ENODEV; > > return -ret; > > } > > > > @@ -1364,13 +1376,13 @@ struct priv * > > if (up) { > > err =3D priv_set_flags(priv, ~IFF_UP, IFF_UP); > > if (err) > > - return err; > > + return errno =3D=3D ENODEV ? -ENODEV : err; >=20 > 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 va= lue > instead and mlx5_set_link_down/up() should return only negative errno > values but are documented as returning positive ones. >=20 > Anyway to keep it short: currently in mlx5, priv_*() =3D> positive errno = and the > public-facing mlx5_*() =3D> negative errno, hence you should return a pos= itive > ENODEV here. >=20 Ok. > You could avoid this mess by patching the public callbacks only and not > internal functions like this one. >=20 > > priv_dev_select_tx_function(priv, dev); > > priv_dev_select_rx_function(priv, dev); > > } else { > > err =3D priv_set_flags(priv, ~IFF_UP, ~IFF_UP); > > if (err) > > - return err; > > + return errno =3D=3D ENODEV ? -ENODEV : err; >=20 > Same here. >=20 > > dev->rx_pkt_burst =3D removed_rx_burst; > > dev->tx_pkt_burst =3D removed_tx_burst; > > } > > @@ -1474,3 +1486,22 @@ struct priv * > > dev->rx_pkt_burst =3D mlx5_rx_burst; > > } > > } > > + > > +/** > > + * Check if mlx5 device was removed. > > + * >=20 > "mlx5" is redundant. >=20 > As with mlx4, a short paragraph should describe where this function is > supposed to be used. >=20 > > + * @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) =3D=3D EIO) > > + return -(rte_errno =3D ENODEV); >=20 > Coding rules prohibit this kind of affectation, see mlx4 comments. >=20 > > + 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 =3D priv_fdir_ctrl_func(priv, filter_op, arg); > > priv_unlock(priv); > > + if (ret && mlx5_removed(priv)) > > + ret =3D 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 =3D 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] =3D &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 =3D priv_ethtool_get_stats_n(priv); > > if (stats_n < 0) { > > priv_unlock(priv); > > - return -1; > > + ret =3D -1; > > + goto error; > > } > > if (xstats_ctrl->stats_n !=3D stats_n) > > priv_xstats_init(priv); > > ret =3D 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] =3D &txq_ctrl->txq; > > out: > > priv_unlock(priv); > > + if (mlx5_removed(priv)) > > + return -ENODEV; > > return -ret; > > } > > > > -- > > 1.8.3.1 > > >=20 > -- > Adrien Mazarguil > 6WIND