Hello Ferruh,
I was almost wondering if this series has been forgotten. Thanks for
the comprehensive review.
My comments inline (and in some other mails):
On Wednesday 28 June 2017 09:11 PM, Ferruh Yigit wrote:
On 6/16/2017 6:40 AM, Shreyansh Jain wrote:
A skeleton which would be called after bus device scan. It currently
fails to identify the device>
Signed-off-by: Hemant Agrawal <hemant.agra...@nxp.com>
Signed-off-by: Shreyansh Jain <shreyansh.j...@nxp.com>
<...>
+
+/* Initialise a network interface */
+static int dpaa_eth_dev_init(struct rte_eth_dev *eth_dev __rte_unused)
__rte_unused can be removed
I will correct this.
<...>
+
+static int
+rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv __rte_unused,
+ struct rte_dpaa_device *dpaa_dev)
+{
+ int diag;
+ int ret;
+ struct rte_eth_dev *eth_dev;
+ char ethdev_name[RTE_ETH_NAME_MAX_LEN];
+
+ PMD_INIT_FUNC_TRACE();
+
+ if (!is_global_init) {
+ /* One time load of Qman/Bman drivers */
+ ret = qman_global_init();
+ if (ret) {
+ PMD_DRV_LOG(ERR, "QMAN initialization failed: %d",
+ ret);
+ return ret;
+ }
+ ret = bman_global_init();
+ if (ret) {
+ PMD_DRV_LOG(ERR, "BMAN initialization failed: %d",
+ ret);
+ return ret;
+ }
+
+ is_global_init = 1;
+ }
+
+ sprintf(ethdev_name, "%s", dpaa_dev->name);
snprintf can be preferred
Ok. Will fix this.
+
+ ret = rte_dpaa_portal_init((void *)1);
+ if (ret) {
+ PMD_DRV_LOG(ERR, "Unable to initialize portal");
+ return ret;
+ }
+
+ eth_dev = rte_eth_dev_allocate(ethdev_name);
If this is done without RTE_PROC_PRIMARY check, this will cause
secondary to memset all device data.
Agree. I will correct this.
I am adding this because of below check, it multi process support is
intended, this also be protected.
+ 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 dpaa_if),
+ RTE_CACHE_LINE_SIZE);
+ if (!eth_dev->data->dev_private) {
+ PMD_INIT_LOG(CRIT, "Cannot allocate memzone for"
+ " private port data\n");
+ rte_eth_dev_release_port(eth_dev);
+ return -ENOMEM;
+ }
+ }
+
+ eth_dev->device = &dpaa_dev->device;
+ dpaa_dev->eth_dev = eth_dev;
I thought "struct rte_dpaa_device" is bus device, like "struct
rte_pci_device", if so why it has link to the eth_dev?
Yes, rte_dpaa_device ~ rte_pci_device.
This is used to extract the eth_dev back while de-initializing
the device.
driver->remove = rte_dpaa_remove(rte_dpaa_device)
// fetch rte_eth_dev from rte_dpaa_device
`-> .eth_dev_stop(eth_dev)
So, essentially reusing the internal eth_ops for cleaning up the
device.
+ eth_dev->data->rx_mbuf_alloc_failed = 0;
not required, data already memset via rte_eth_dev_allocate()
Ok. I will remove this.
+
+ /* Invoke PMD device initialization function */
+ diag = dpaa_eth_dev_init(eth_dev);
+ if (diag) {
+ PMD_DRV_LOG(ERR, "Eth dev initialization failed: %d", ret);
+ return diag;
+ }
+
+ PMD_DRV_LOG(DEBUG, "Eth dev initialized: %d\n", diag);
+
+ return 0;
+}
+
+static int
+rte_dpaa_remove(struct rte_dpaa_device *dpaa_dev)
+{
+ struct rte_eth_dev *eth_dev;
+
+ PMD_INIT_FUNC_TRACE();
+
+ eth_dev = dpaa_dev->eth_dev;
can be:
eth_dev = rte_eth_dev_allocated(dpaa_dev->device.name);
Hmm, ok, now I understand why you are inquiring about
eth_dev being assigned in rte_dpaa_device. I will have a
relook at this and fix if required.
+
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ rte_free(eth_dev->data->dev_private);
+
no pmd uninit() ?
I will fix this. There is an internal commit that we had very
recently (a miss in previous series).
+ rte_eth_dev_release_port(eth_dev);
+
+ return 0;
+}
<...>
