Re: [dpdk-dev] [PATCH v3 1/8] net/mvneta: add neta PMD skeleton

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

On 8/31/2018 1:59 PM, Andrzej Ostruszka wrote:
> From: Zyta Szpak <zr@semihalf.com>
> 
> Add neta pmd driver skeleton providing base for the further
> development.
> 
> Signed-off-by: Natalie Samsonov <nsamsono@marvell.com>
> Signed-off-by: Yelena Krivosheev <yelena@marvell.com>
> Signed-off-by: Dmitri Epshtein <dima@marvell.com>
> Signed-off-by: Zyta Szpak <zr@semihalf.com>
> Signed-off-by: Andrzej Ostruszka <amo@semihalf.com>
<...>

> --- /dev/null
> +++ b/doc/guides/nics/features/mvneta.ini
> @@ -0,0 +1,18 @@
> +;
> +; Supported features of the 'mvneta' network poll mode driver.
> +;
> +; Refer to default.ini for the full list of available PMD features.
> +;
> +[Features]
> +Speed capabilities   = Y
> +Link status          = Y
For link status feature "link_update" eth_dev_ops needs to be implemented,
please either implement it or remove feature. (I saw this done later,  please
add this on relevant patch)

> +MTU update           = Y

For mtu update feature "mtu_set" eth_dev_ops needs to be implemented,
please either implement it or remove feature.  (I saw this done later,  please
add this on relevant patch)

> +Jumbo frame          = Y
> +Promiscuous mode     = Y

Promiscuous mode support is not enabled. (I saw this done later,  please add
this on relevant patch)

> +CRC offload          = Y
> +L3 checksum offload  = Y
> +L4 checksum offload  = Y

Claiming L3 & L4 cksum offload also requires data path implementation to take
care mbuf.ol_flags flags. (I saw this is done in next patch, please add this
line in next patch)
Also I can see these are presented as capabilities but shouldn't this
information passed to musdk somehow? Are they always enabled?

> +Packet type parsing  = Y

Similar to above, dev_supported_ptypes_get() is implemented but this also
requires data path implementation that fills mbuf->packet_type.  (I saw this is
done in next patch, please add this line in next patch)

> +Basic stats          = Y

Basic stats collection is not implemented in this patch.  (I saw this done
later,  please add this on relevant patch)

<...>

> +Usage example
> +^^^^^^^^^^^^^
> +
> +.. code-block:: console
> +
> +   ./testpmd --vdev=eth_mvneta,iface=eth0,iface=eth1 \

There is a limit to number of "iface" in code, does it make sense to document it?

<...>

> +#define MVNETA_RX_OFFLOADS (DEV_RX_OFFLOAD_JUMBO_FRAME | \
> +			  DEV_RX_OFFLOAD_CRC_STRIP | \

CRC_STRIP flag removed in next-net

<...>

> +
> +	if (!(dev->data->dev_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
> +		MVNETA_LOG(INFO,
> +			"L2 CRC stripping is always enabled in hw");
> +		dev->data->dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> +	}

DEV_RX_OFFLOAD_CRC_STRIP is removed from next-net, you can remove it from
capabilities and remove above block.

<...>

> +
> +	eth_dev->data->kdrv = RTE_KDRV_NONE;
> +	eth_dev->data->dev_private = priv;
> +	eth_dev->device = &vdev->device;
> +;	eth_dev->dev_ops = &mvneta_ops;

extra ";" at the beginning of the line

<...>

> +RTE_PMD_REGISTER_VDEV(net_mvneta, pmd_mvneta_drv);
> +RTE_PMD_REGISTER_ALIAS(net_mvneta, eth_mvneta);

No need to provide alias for new PMDs