[dpdk-dev] [PATCHv5 13/33] net/dpaa2: introducing NXP dpaa2 pmd driver

[Shreyansh Jain]

Hello Ferruh,

On Friday 20 January 2017 12:45 AM, Ferruh Yigit wrote:
> On 1/19/2017 1:23 PM, Hemant Agrawal wrote:
>> add support for fsl-mc bus based dpaa2 pmd driver.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>
> <...>
>
>> diff --git a/drivers/common/Makefile b/drivers/common/Makefile
>> index e5bfecb..76ec2d1 100644
>> --- a/drivers/common/Makefile
>> +++ b/drivers/common/Makefile
>> @@ -31,6 +31,8 @@
>>
>>  include $(RTE_SDK)/mk/rte.vars.mk
>>
>> +CONFIG_RTE_LIBRTE_DPAA2_COMMON = $(CONFIG_RTE_LIBRTE_DPAA2_PMD)
>
> This logic make sense, is there any reason DPAA2_COMMON to be a config
> option, it doesn't look like something a user would like to change on
> its own.

I am assuming you wanted to say "The logic _doesn't_ make sense, ..." :)

>
> Instead, as done here, if there is a user of common folder it is enabled
> in Makefile. For this DPAA2_COMMON doesn't need to be a config option
> itself I think.

Aim of drivers/common was to introduce libraries which may be used by
more than one sub-system. These can be other than dpaa2 and may even
be external to DPDK framework itself.

But for now, we will remove it and keep it toggleable with rte.app.mk
changes. Maybe in future, if need for configurable option exist, we will
introduce.

>
>> +
>>  DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_COMMON) += dpaa2
>>
>>  include $(RTE_SDK)/mk/rte.subdir.mk
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 40fc333..f716ca0 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -57,7 +57,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += thunderx
>>  DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
>>  DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
>>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
>> -
>> +DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += dpaa2
>
> Add alphabetically please.

Agree.

>
>>  ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
>>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += vhost
>>  endif # $(CONFIG_RTE_LIBRTE_VHOST)
>
> <...>
>
>> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
>> new file mode 100644
>> index 0000000..2295f82
>> --- /dev/null
>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> <...>
>> +/* Name of the DPAA2 Net PMD */
>> +static const char *drivername = "DPAA2 PMD";
>
> Custom names not preferred, please check any other PMD to be consistent.

Yes, this should be changed.

>
> <...>
>
>> +static int
>> +rte_dpaa2_probe(struct rte_dpaa2_driver *dpaa2_drv,
>> +		struct rte_dpaa2_device *dpaa2_dev)
>> +{
>> +	struct eth_driver    *eth_drv;
>
> whitespace error.

Fixed. Thanks.

>
>> +	struct rte_eth_dev *eth_dev;
>> +	char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>> +
>> +	int diag;
>> +
>> +	eth_drv = (struct eth_driver *)dpaa2_drv;
>
> How this suppose to work?

Actually, a not-so-clean-logic coupled with the above line which 
probably got added in one of our rebasing attempts. And you have pointed
out correctly - because of eth_driver<->pci_driver restriction.

(more below)

>
> struct eth_driver {
>
> 	struct rte_pci_driver pci_drv;
> 	eth_dev_init_t eth_dev_init;
> 	eth_dev_uninit_t eth_dev_uninit;
> 	unsigned int dev_private_size;
> };
>
> struct rte_dpaa2_driver {
> 	TAILQ_ENTRY(rte_dpaa2_driver) next;
> 	struct rte_driver driver;
> 	struct rte_fslmc_bus *fslmc_bus;
> 	uint32_t drv_flags;
> 	uint16_t drv_type;
> 	rte_dpaa2_probe_t probe
> 	rte_dpaa2_remove_t remove;
> };

dpaa2 driver is not using eth_drv - it is more of a dummy.

If we had used rte_eth_dev_pci_probe, we would have faced this issue of
delinking eth_driver from pci_probe. So, we have leveraged the
bus->probe and created our own probing routine (rather than use 
rte_eth_dev_pci_probe).

Then, because it is our own probe routine, we simply allocate the
eth_dev (which luckily is free from pci_dev), and call our dev_init,
which rte_eth_dev_pci_probe would have done using eth_drv->init().

(More below)


>
>> +
>> +	sprintf(ethdev_name, "dpni-%d", dpaa2_dev->object_id);
>> +
>> +	eth_dev = rte_eth_dev_allocate(ethdev_name);
>> +	if (eth_dev == NULL)
>> +		return -ENOMEM;
>> +
>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>> +		eth_dev->data->dev_private = rte_zmalloc(
>> +						"ethdev private structure",
>> +						sizeof(struct dpaa2_dev_priv),
>> +						RTE_CACHE_LINE_SIZE);
>> +		if (eth_dev->data->dev_private == NULL) {
>> +			RTE_LOG(CRIT, PMD, "Cannot allocate memzone for"
>> +				" private port data\n");
>> +			return -ENOMEM;
>
> release allocated port?

Yes, I will fix this.

>
>> +		}
>> +	}
>> +	eth_dev->device = &dpaa2_dev->device;
>> +	dpaa2_dev->eth_dev = eth_dev;
>> +	eth_dev->driver = eth_drv;
>> +	eth_dev->data->rx_mbuf_alloc_failed = 0;
>> +
>> +	/* init user callbacks */
>> +	TAILQ_INIT(&eth_dev->link_intr_cbs);
>
> This is no more required, since done by rte_eth_dev_allocate()

yes, through eth_dev_get. Thanks - next version will contain the fix.

>
>> +
>> +	/*
>> +	 * Set the default MTU.
>> +	 */
>> +	eth_dev->data->mtu = ETHER_MTU;
>
> Same, no more required to set in pmd.

Yes, thanks for highlighting. Implementing our own rte_eth_dev_pci_probe
meant writing all these ourselves which we couldn't match to the changes
being done upstream.

>
>> +
>> +	/* Invoke PMD device initialization function */
>> +	diag = dpaa2_dev_init(eth_dev);
>> +	if (diag == 0)
>> +		return 0;
>> +
>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>> +		rte_free(eth_dev->data->dev_private);
>> +	rte_eth_dev_release_port(eth_dev);
>> +	return diag;
>> +}
>
> <...>
>
>> +static struct rte_dpaa2_driver rte_dpaa2_pmd = {
>> +	.drv_type = DPAA2_MC_DPNI_DEVID,
>> +	.driver = {
>> +		.name = "DPAA2 PMD",
>
> This is not required, RTE_PMD_REGISTER_DPAA2 will set it.

Yes. Will be removed.

>
>> +	},
>> +	.probe = rte_dpaa2_probe,
>> +	.remove = rte_dpaa2_remove,
>
> These are PMD specific APIs, PCI equivalent of these are eth_dev_init()
> and eth_dev_uninit() functions. Instead of rte_eth_dev_pci_probe() like
> function.
> Here it seems used as how it is used for virtual devices. But even for
> virtual devices, it is desired to have more proper virtual bus structure.
>
> I understand this part is a little problematic now, because of the
> eth_driver <-> pci_driver relation, this is not generic.
> What do you think first solve eth_driver <-> pci_driver problem, out of
> this patchset, and later add this PMD?

This is a long pending problem. I agree that it would be cleaner to do 
this unlink. But, I think that the current way dpaa2 driver handles it
is not truly incorrect.

bus->probe() can be linked to a driver specific probe (which dpaa2 
does). A PCI PMD would have called rte_eth_dev_pci_probe() as a helper
for doing all eth_dev/eth_drv initialization. dpaa2 does it manually.

I agree we need to match up to various changes done in 
rte_eth_dev_pci_probe(), but after that our code is as legal as any 
other PMD :D.

>
> <...>
>
>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
>> index a5daa84..c793dd2 100644
>> --- a/mk/rte.app.mk
>> +++ b/mk/rte.app.mk
>> @@ -110,6 +110,11 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_AF_PACKET)  += -lrte_pmd_af_packet
>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_BNX2X_PMD)      += -lrte_pmd_bnx2x -lz
>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_BNXT_PMD)       += -lrte_pmd_bnxt
>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_CXGBE_PMD)      += -lrte_pmd_cxgbe
>> +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_COMMON),y)
>
> If DPAA2_COMMON removed it can work here too, because if DPAA2_PMD
> enabled, DPAA2_COMMON should be already enabled.

Agreed.

>
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD)      += -lrte_pmd_dpaa2
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD)      += -lrte_pmd_dpaa2_qbman
>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD)      += -lrte_pmd_fslmcbus
>> +endif
>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_E1000_PMD)      += -lrte_pmd_e1000
>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_ENA_PMD)        += -lrte_pmd_ena
>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_ENIC_PMD)       += -lrte_pmd_enic
>>
>
>