Hi Shachar, On Wed, Jul 26, 2017 at 09:21:27AM +0000, Shachar Beiser wrote: > Hi , > > When I say close I mean : " mlx5_dev_close" . This function set the > priv->ctx to NULL. > We think this patch is required because we have an open bug of seg > fault while accessing priv->ctx == NULL. > > -Shachar Beiser. >
This patch does not fix the root cause of the issue. It is a bug in the ether layer, and missing flags within MLX5 PMD. So NACK on this patch, I will send shortly the proper fix. Best regards, > > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > Sent: Wednesday, July 26, 2017 12:06 PM > To: Shachar Beiser <shacha...@mellanox.com> > Cc: dev@dpdk.org; Nélio Laranjeiro <nelio.laranje...@6wind.com>; > sta...@dpdk.org > Subject: Re: [PATCH] net/mlx5: fix verification of device context > > Hi Shachar, > > On Wed, Jul 26, 2017 at 05:43:24AM +0000, Shachar Beiser wrote: > > Get interface name function lacks verification of device context. > > It might lead to segmentation fault when trying to query the name > > after the device is closed.fixing it by adding the missing > > verification > > > > Thanks, however if by "close" you mean it may occur when applications use > ethdev callbacks after a call to rte_eth_dev_close(), I do not think PMDs > have to protect themselves against bad application behavior, otherwise there > is no end to such fixes. > > The reverse of rte_eth_dev_close() is not rte_eth_dev_configure() nor any > other ethdev callback (see documentation), but a bus probe operation. > > Perhaps I've missed something, so in case a crash occurs *while* calling > rte_eth_dev_close() I guess this patch is fine, but then please describe the > reason. > > > Fixes: cd89f22a1e9770 ("net/mlx5: remove unused interface name query") > > This commit doesn't look like the root cause of that issue? > > > Cc: sta...@dpdk.org > > > > Signed-off-by: Shachar Beiser <shacha...@mellanox.com> > > --- > > drivers/net/mlx5/mlx5_ethdev.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c > > b/drivers/net/mlx5/mlx5_ethdev.c index b70b7b9..6e67461 100644 > > --- a/drivers/net/mlx5/mlx5_ethdev.c > > +++ b/drivers/net/mlx5/mlx5_ethdev.c > > @@ -173,6 +173,10 @@ struct priv * > > char match[IF_NAMESIZE] = ""; > > > > { > > + if (priv->ctx == NULL) { > > + DEBUG("The device is closed, cannot query interface > > name "); > > + return -1; > > + } > > MKSTR() is at the beginning of this block because it defines a new variable > (path). For coding style consistency you should not put any code before > variable declarations, or at least insert an empty line between them. > Otherwise you could move this check to the parent block. > > > MKSTR(path, "%s/device/net", priv->ctx->device->ibdev_path); > > > > dir = opendir(path); > > -- > > 1.8.3.1 > > > > I think this patch is not necessary unless proved otherwise, have you > actually observed a crash addressed by it? > > -- > Adrien Mazarguil > 6WIND -- Gaëtan Rivet 6WIND
