Re: [dpdk-dev] [PATCH] net/mlx5: fix handling link status change event

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Yongseok,

Small nips.

On Mon, Jan 15, 2018 at 03:48:00PM -0800, Yongseok Koh wrote:
> Even though link of a port gets down, device still can receive traffic.
> That is the reason why mlx5_set_link_up/down() switches rx/tx_pkt_burst().
> However, if link gets down by an external command (e.g. ifconfig), it isn't
> effective. It is better to change burst functions when link status change
> is detected.
> 
> Fixes: 62072098b54e ("mlx5: support setting link up or down")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c         |   1 -
>  drivers/net/mlx5/mlx5.h         |   1 +
>  drivers/net/mlx5/mlx5_ethdev.c  | 112 +++++++++++++++++++++++++++++++---------
>  drivers/net/mlx5/mlx5_trigger.c |  23 ++-------
>  4 files changed, 93 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 1c95f3520..fc2d59fee 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -869,7 +869,6 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
>  		/* Bring Ethernet device up. */
>  		DEBUG("forcing Ethernet interface up");
>  		priv_set_flags(priv, ~IFF_UP, IFF_UP);
> -		mlx5_link_update(priv->dev, 1);
>  		/* Store device configuration on private structure. */
>  		priv->config = config;
>  		continue;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index e740a4e77..9a4a59bd4 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -225,6 +225,7 @@ int priv_set_flags(struct priv *, unsigned int, unsigned int);
>  int mlx5_dev_configure(struct rte_eth_dev *);
>  void mlx5_dev_infos_get(struct rte_eth_dev *, struct rte_eth_dev_info *);
>  const uint32_t *mlx5_dev_supported_ptypes_get(struct rte_eth_dev *dev);
> +int priv_link_update(struct priv *, int);
>  int mlx5_link_update(struct rte_eth_dev *, int);
>  int mlx5_dev_set_mtu(struct rte_eth_dev *, uint16_t);
>  int mlx5_dev_get_flow_ctrl(struct rte_eth_dev *, struct rte_eth_fc_conf *);
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 6f78adc4a..16377a8cb 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -885,7 +885,49 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete)
>  }
>  
>  /**
> - * DPDK callback to retrieve physical link information.
> + * Enable receiving and transmitting traffic.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + */
> +static void
> +priv_link_start(struct priv *priv)

Comments says dev but parameter is priv.

> +{
> +	struct rte_eth_dev *dev = priv->dev;
> +	int err;
> +
> +	dev->tx_pkt_burst = priv_select_tx_function(priv, dev);
> +	dev->rx_pkt_burst = priv_select_rx_function(priv, dev);
> +	err = priv_dev_traffic_enable(priv, dev);
> +	if (err)
> +		ERROR("%p: error occurred while configuring control flows: %s",
> +		      (void *)priv, strerror(err));
> +	err = priv_flow_start(priv, &priv->flows);
> +	if (err)
> +		ERROR("%p: error occurred while configuring flows: %s",
> +		      (void *)priv, strerror(err));
> +}
> +
> +/**
> + * Disable receiving and transmitting traffic.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + */
> +static void
> +priv_link_stop(struct priv *priv)

Same here.

> +{
> +	struct rte_eth_dev *dev = priv->dev;
> +
> +	priv_flow_stop(priv, &priv->flows);
> +	priv_dev_traffic_disable(priv, dev);
> +	dev->rx_pkt_burst = removed_rx_burst;
> +	dev->tx_pkt_burst = removed_tx_burst;
> +}
> +
> +/**
> + * Retrieve physical link information and update rx/tx_pkt_burst callbacks
> + * accordingly.
>   *
>   * @param dev
>   *   Pointer to Ethernet device structure.
> @@ -893,17 +935,54 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete)
>   *   Wait for request completion (ignored).
>   */
>  int
> -mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> +priv_link_update(struct priv *priv, int wait_to_complete)
<snip/>

Seems to be also the case here.

Thanks,

-- 
Nélio Laranjeiro
6WIND