[dpdk-dev,3/3] net/mlx5: adjust removal error

Message ID 1509637324-13525-4-git-send-email-matan@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Shahaf Shuler
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Matan Azrad Nov. 2, 2017, 3:42 p.m. UTC
  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 <matan@mellanox.com>
---
 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(-)
  

Comments

Adrien Mazarguil Nov. 3, 2017, 1:06 p.m. UTC | #1
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 <matan@mellanox.com>

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
>
  
Matan Azrad Nov. 5, 2017, 6:57 a.m. UTC | #2
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 <matan@mellanox.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>; dev@dpdk.org
> Subject: Re: [PATCH 3/3] net/mlx5: adjust removal error
> 
> 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 <matan@mellanox.com>
> 
> 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.
> 
Will check it, thanks!

> 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.
> 
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 >= 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.
> 

Ok.

> 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
  

Patch

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);
+	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;
 		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;
 		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.
+ *
+ * @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);
+	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;
 }