Re: [dpdk-dev] [PATCH v2 10/17] net/ionic: add basic port operations

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/15/2019 9:22 AM, Alfredo Cardigliano wrote:
> Add support for port start/stop and handle basic features
> including mtu and link up/down.
> 
> Signed-off-by: Alfredo Cardigliano <cardigliano@ntop.org>
> Reviewed-by: Shannon Nelson <snelson@pensando.io>
> ---
>  doc/guides/nics/features/ionic.ini |   4 +
>  drivers/net/ionic/ionic.h          |   1 +
>  drivers/net/ionic/ionic_dev.h      |   3 +
>  drivers/net/ionic/ionic_ethdev.c   | 316 +++++++++++++++++++++++++++++
>  drivers/net/ionic/ionic_lif.c      | 268 ++++++++++++++++++++++++
>  drivers/net/ionic/ionic_lif.h      |   9 +
>  drivers/net/ionic/ionic_osdep.h    |   2 +
>  7 files changed, 603 insertions(+)

<...>

> @@ -18,6 +18,17 @@
>  
>  static int  eth_ionic_dev_init(struct rte_eth_dev *eth_dev, void *init_params);
>  static int  eth_ionic_dev_uninit(struct rte_eth_dev *eth_dev);
> +static int  ionic_dev_info_get(struct rte_eth_dev *eth_dev,
> +		struct rte_eth_dev_info *dev_info);
> +static int  ionic_dev_configure(struct rte_eth_dev *dev);
> +static int  ionic_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
> +static int  ionic_dev_start(struct rte_eth_dev *dev);
> +static void ionic_dev_stop(struct rte_eth_dev *dev);

Start and stop dev_ops should not return 'void' but 'int', I think you will see
this when rebase to latest code. Same for a few more dev_ops.

<...>

> +static int
> +ionic_dev_mtu_set(struct rte_eth_dev *eth_dev, uint16_t mtu)
> +{
> +	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
> +	/*
> +	 * Size = MTU + Ethernet header + VLAN + QinQ
> +	 * Also add ETHER_CRC_LEN if the adapter is able to keep CRC
> +	 */
> +	uint32_t frame_size = mtu + RTE_ETHER_HDR_LEN + 4 + 4;
> +	int err;
> +
> +	ionic_init_print_call();
> +
> +	/* Check that mtu is within the allowed range */
> +	if (mtu < IONIC_MIN_MTU || mtu > IONIC_MAX_MTU)
> +		return -EINVAL;

If you feed these values to dev_info min_mtu/max_mtu, this check will be done by
API level already.

> +
> +	err = ionic_lif_change_mtu(lif, mtu);
> +
> +	if (err)
> +		return err;
> +
> +	/* Update max frame size */
> +	eth_dev->data->dev_conf.rxmode.max_rx_pkt_len = frame_size;

'max_rx_pkt_len' is a user provided configuration value to limit the packet size
in jumbo frame case, and it shouldn't just updated to reflect the MTU.

<...>

> +static int
> +ionic_dev_start(struct rte_eth_dev *eth_dev)
> +{
> +	struct rte_eth_conf *dev_conf = &eth_dev->data->dev_conf;
> +	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
> +	struct ionic_adapter *adapter = lif->adapter;
> +	struct ionic_dev *idev = &adapter->idev;
> +	uint32_t allowed_speeds;
> +	int err;
> +
> +	ionic_init_print_call();
> +
> +	err = ionic_lif_start(lif);
> +
> +	if (err) {
> +		ionic_drv_print(ERR, "Cannot start LIF: %d", err);
> +		return err;
> +	}
> +
> +	if (eth_dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED) {
> +		uint32_t speed = ionic_parse_link_speeds(dev_conf->link_speeds);
> +
> +		if (speed)
> +			ionic_dev_cmd_port_speed(idev, speed);
> +	}
> +
> +	allowed_speeds =
> +		ETH_LINK_SPEED_FIXED |
> +		ETH_LINK_SPEED_10G |
> +		ETH_LINK_SPEED_25G |
> +		ETH_LINK_SPEED_40G |
> +		ETH_LINK_SPEED_50G |
> +		ETH_LINK_SPEED_100G;
> +
> +	if (dev_conf->link_speeds & ~allowed_speeds) {
> +		ionic_init_print(ERR, "Invalid link setting");

Should this check be done before 'ionic_lif_start()' call?

<...>

> +static void
> +ionic_dev_close(struct rte_eth_dev *eth_dev)
> +{
> +	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
> +	int err;
> +
> +	ionic_init_print_call();
> +
> +	err = ionic_lif_stop(lif);
> +
> +	if (err) {
> +		ionic_drv_print(ERR, "Cannot stop LIF: %d", err);
> +		return;
> +	}

'close()' dev_ops should free all device resources. A closed eth_dev can't be
opened back. Can you please add additional cleanup.

<...>

> diff --git a/drivers/net/ionic/ionic_osdep.h b/drivers/net/ionic/ionic_osdep.h
> index 5b8baa67b..39a9cefa9 100644
> --- a/drivers/net/ionic/ionic_osdep.h
> +++ b/drivers/net/ionic/ionic_osdep.h
> @@ -28,6 +28,8 @@
>  #define BIT_ULL(nr)        (1ULL << (nr))
>  #define BITS_TO_LONGS(nr)  div_round_up(nr, 8 * sizeof(long))
>  
> +#define ETH_ALEN        6

You can use 'RTE_ETHER_ADDR_LEN' instead in the net libary