Re: [dpdk-dev] [PATCH v9 1/5] net/enetfec: introduce NXP ENETFEC driver

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

On 11/10/2021 7:48 AM, Apeksha Gupta wrote:
> ENETFEC (Fast Ethernet Controller) is a network poll mode driver
> for NXP SoC i.MX 8M Mini.
> 
> This patch adds skeleton for enetfec driver with probe function.
> 
> Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>

<...>

> @@ -0,0 +1,133 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright 2021 NXP
> +
> +ENETFEC Poll Mode Driver
> +========================
> +
> +The ENETFEC NIC PMD (**librte_net_enetfec**) provides poll mode driver
> +support for the inbuilt NIC found in the ** NXP i.MX 8M Mini** SoC.
> +
> +More information can be found at NXP Official Website
> +<https://www.nxp.com/products/processors-and-microcontrollers/arm-processors/i-mx-applications-processors/i-mx-8-processors/i-mx-8m-mini-arm-cortex-a53-cortex-m4-audio-voice-video:i.MX8MMINI>
> +
> +ENETFEC
> +-------
> +
> +This section provides an overview of the NXP ENETFEC and how it is
> +integrated into the DPDK. Driver is taken as **experimental** as driver
> +itself detects the uio device, reads address and mmap them within the
> +driver.

What about something like:
"Driver is taken as **experimental** as driver depends on a Linux kernel
module, 'fec-uio', which is not upstreamed yet."

<...>

> +static int
> +pmd_enetfec_probe(struct rte_vdev_device *vdev)
> +{
> +	struct rte_eth_dev *dev = NULL;
> +	struct enetfec_private *fep;
> +	const char *name;
> +	int rc;
> +
> +	name = rte_vdev_device_name(vdev);
> +	if (name == NULL)
> +		return -EINVAL;


I am not really sure if this check is required, although it doesn't hurt.
Can you please share the call stack, how it can be NULL?

<...>

> +struct enetfec_private {
> +	struct rte_eth_dev	*dev;
> +	struct rte_eth_stats	stats;
> +	struct rte_mempool	*pool;
> +	uint16_t		max_rx_queues;
> +	uint16_t		max_tx_queues;
> +	unsigned int		total_tx_ring_size;
> +	unsigned int		total_rx_ring_size;
> +	bool			bufdesc_ex;
> +	int			full_duplex;
> +	uint32_t		quirks;
> +	uint32_t		enetfec_e_cntl;
> +	int			flag_csum;
> +	int			flag_pause;
> +	bool			rgmii_txc_delay;
> +	bool			rgmii_rxc_delay;
> +	int			link;
> +	void			*hw_baseaddr_v;
> +	uint64_t		hw_baseaddr_p;
> +	void			*bd_addr_v;
> +	uint64_t		bd_addr_p;
> +	uint64_t		bd_addr_p_r[ENETFEC_MAX_Q];
> +	uint64_t		bd_addr_p_t[ENETFEC_MAX_Q];
> +	void			*dma_baseaddr_r[ENETFEC_MAX_Q];
> +	void			*dma_baseaddr_t[ENETFEC_MAX_Q];
> +	uint64_t		cbus_size;
> +	unsigned int		reg_size;
> +	unsigned int		bd_size;
> +	struct enetfec_priv_rx_q *rx_queues[ENETFEC_MAX_Q];
> +	struct enetfec_priv_tx_q *tx_queues[ENETFEC_MAX_Q];

What do you think to construct the struct as the fields are used, at this patch
only 'dev' seems needed.

<...>

> diff --git a/drivers/net/enetfec/meson.build b/drivers/net/enetfec/meson.build
> new file mode 100644
> index 0000000000..6d6c64c94b
> --- /dev/null
> +++ b/drivers/net/enetfec/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 NXP
> +
> +if not is_linux
> +	build = false
> +	reason = 'only supported on linux'
> +endif
> +
> +sources = files('enet_ethdev.c')

./devtools/check-meson.py is failing, can you please fix meson syntax accordingly,
not only this patch, in all patches that updates meson file.