On Wednesday 28 June 2017 09:15 PM, Ferruh Yigit wrote:
On 6/16/2017 6:40 AM, Shreyansh Jain wrote:
Signed-off-by: Hemant Agrawal <hemant.agra...@nxp.com>
Signed-off-by: Shreyansh Jain <shreyansh.j...@nxp.com>
---
doc/guides/nics/features/dpaa.ini | 1 +
drivers/net/dpaa/Makefile | 4 +
drivers/net/dpaa/dpaa_ethdev.c | 279 ++++++++++++++++++++++++++++++++-
drivers/net/dpaa/dpaa_ethdev.h | 6 +
drivers/net/dpaa/dpaa_rxtx.c | 313 ++++++++++++++++++++++++++++++++++++++
drivers/net/dpaa/dpaa_rxtx.h | 61 ++++++++
This patch adds initial rx/tx support, as well as rx/tx queue support as
mentioned in patch subject.
I would be for splitting patch, but even if patch not splitted, I would
suggest updating patch suject and commit log to cover patch content.
Ok. I will fix this (splitting if possible, else update to commit).
<...>
--- a/doc/guides/nics/features/dpaa.ini
+++ b/doc/guides/nics/features/dpaa.ini
@@ -4,5 +4,6 @@
; Refer to default.ini for the full list of available PMD features.
;
[Features]
+Queue start/stop = Y
This requires following dev_ops implemented:
rx_queue_start, rx_queue_stop, tx_queue_start, tx_queue_stop
Ok. My understanding here was wrong - I incorrectly matched this
to queue setup/teardown. I will remove this feature listing. (and
a couple more as per your review comment on other patches).
ARMv8 = Y
Usage doc = Y
<...>
+
+ /* Initialize Rx FQ's */
+ if (getenv("DPAA_NUM_RX_QUEUES"))
I think this was disscussed before, should a PMD get config options from
enviroment variable? Altough this works, I am for a more explicit
method, like dev_args.
Well, I do remember that discussion and still continued with it because
1) I am not done with that dev_args changes and 2) I think this is more
non-intrusive as this is specific to DPAA without need for expanding it
towards dev_args (and impacting application arg list).
You think this is no-go? If so, I will fix this.
<...>
+
+ dpaa_intf->rx_queues = rte_zmalloc(NULL,
+ sizeof(struct qman_fq) * num_rx_fqs, MAX_CACHELINE);
A NULL check perhaps?
And if multi-process support desired, this should be done only for
primary process.
I will fix both the above.
<...>
+ /* Allocate memory for storing MAC addresses */
+ eth_dev->data->mac_addrs = rte_zmalloc("mac_addr",
+ ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER, 0);
+ if (eth_dev->data->mac_addrs == NULL) {
+ PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to "
+ "store MAC addresses",
+ ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER);
Anything to cleanup before exit?
bad miss from my side. *_queues should be released - I will fix this. (I
will run some static analyzer and fix
any other similar before next version)
+ return -ENOMEM;
+ }
<...>
+uint16_t dpaa_eth_queue_rx(void *q,
+ struct rte_mbuf **bufs,
+ uint16_t nb_bufs)
+{
+ struct qman_fq *fq = q;
+ struct qm_dqrr_entry *dq;
+ uint32_t num_rx = 0, ifid = ((struct dpaa_if *)fq->dpaa_intf)->ifid;
+ int ret;
+
+ ret = rte_dpaa_portal_init((void *)0);
+ if (ret) {
+ PMD_DRV_LOG(ERR, "Failure in affining portal");
+ return 0;
+ }
This is rx_pkt_burst function, right? Is it Ok to call
rte_dpaa_portal_init() in Rx data path?
Yes, actually, a portal needs to be initialized if not already - for all
I/O operations to succeed.
rte_dpaa_portal_init segragates calls if multiple entries are made for
initialization.
rte_dpaa_portal_init
`-> _dpaa_portal_init() if not already initialized
<...>
+ buf = (uint64_t)rte_dpaa_mem_ptov(bufs.addr) - bp_info->meta_data_size;
+ if (!buf)
+ goto out;
goto is not required here.
:) yes, I will remove this stupid miss.
+
+out:
+ return (void *)buf;
+}
+
<...>
+uint16_t dpaa_eth_tx_drop_all(void *q __rte_unused,
+ struct rte_mbuf **bufs __rte_unused,
+ uint16_t nb_bufs __rte_unused)
+{
+ PMD_TX_LOG(DEBUG, "Drop all packets");
Should mbufs freed here?
+
+ /* Drop all incoming packets. No need to free packets here
+ * because the rte_eth f/w frees up the packets through tx_buffer
+ * callback in case this functions returns count less than nb_bufs
+ */
Ah, actually I was banking on logic that in case a driver doesn't
release memory, the API caller (on getting less than nb_bufs) would do
that. This is case for stopped interface.
But, I agree, this is dirty fix. I will change this.
+ return 0;
+}
<...>
