Hi,
NXP Confidential > -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Wednesday, October 28, 2026 8:11 PM > To: Gagandeep Singh <[email protected]> > Cc: [email protected]; Hemant Agrawal <[email protected]> > Subject: Re: [PATCH v1 0/3] DPAA driver changes > > On Wed, 28 Oct 2026 14:25:14 +0530 > Gagandeep Singh <[email protected]> wrote: > > > - Adding OH port support. > > - Updates in shared ethernet code. > > - Congestion related code cleanup. > > > > Gagandeep Singh (1): > > drivers: add offline (O/H) port device support > > > > Hemant Agrawal (1): > > net/dpaa: support non fmX-macY type of shared Ethernet name > > > > Jun Yang (1): > > drivers: add DPAA cgrid cleanup support > > > > doc/api/doxy-api-index.md | 1 + > > doc/guides/nics/dpaa.rst | 32 + > > drivers/bus/dpaa/base/fman/fman.c | 19 + > > drivers/bus/dpaa/base/qbman/qman.c | 61 ++ > > drivers/bus/dpaa/bus_dpaa_driver.h | 4 +- > > drivers/bus/dpaa/dpaa_bus.c | 100 ++- > > drivers/bus/dpaa/dpaa_bus_base_symbols.c | 2 + > > drivers/bus/dpaa/include/fman.h | 6 +- > > drivers/bus/dpaa/include/fsl_qman.h | 28 + > > drivers/net/dpaa/dpaa_ethdev.c | 176 ++++- > > drivers/net/dpaa/dpaa_ethdev.h | 3 + > > drivers/net/dpaa/dpaa_flow.c | 7 +- > > drivers/net/dpaa/dpaa_oldev.c | 875 +++++++++++++++++++++++ > > drivers/net/dpaa/meson.build | 11 +- > > drivers/net/dpaa/rte_pmd_dpaa_oldev.h | 158 ++++ > > 15 files changed, 1454 insertions(+), 29 deletions(-) create mode > > 100644 drivers/net/dpaa/dpaa_oldev.c create > > AI review found issues. The biggest one is carrying vendor private ioctl data > structures in DPDK. > > Review of [PATCH v1 0/3] DPAA offline port, shared MAC, cgrid cleanup > > Series > ------ > > The rte_pmd_dpaa_ol_* functions do not operate on a DPDK port. They take no > port_id and only forward a caller-filled struct to an out-of-tree kernel > driver > through ioctl() on /dev/cdx_ctrl. That puts a vendor kernel ioctl ABI (GTP > classification, "telecom application" > flags) into an installed DPDK header under DPDK API/ABI rules. An application > can > issue these ioctls itself; this belongs in the vendor userspace library for > that kernel > module, not in DPDK. Please drop rte_pmd_dpaa_oldev.h and the four exported > functions. To clarify the design intent: the DPAA offline (OL) port is one half of a split-driver model. The other half is the ASK/CDX kernel module, which is part of the NXP SDK and is released together with this PMD. The OL port's Rx/Tx path runs entirely in DPDK via QMan FQs, but the classification policy deciding which GTP/IP traffic is steered to the DPDK fast path versus handled by the kernel module is configured through ASK/CDX, since that module owns the control-plane side of this split. `rte_pmd_dpaa_ol_set_classif_info()`, `_reset_classif_info()`, `_set_lgw_info()`, and `_reset_lgw_info()` are the configuration contract for this specific, co-developed kernel module — not a generic ioctl-wrapping convenience API. The structs mirror the ASK/CDX kernel ABI by necessity, since that is the data being configured for this shared interface. So We'd like to keep these APIs in-tree as they are required for the OL port feature. > > The drv_oldev bus argument is redundant. The offline port is only probed if > "-a > dpaa_bus:oldev1" is given anyway, so create the oldev1 device when its devargs > exist and drop drv_oldev, the bus_str parsing, and the allowlist caveats in > dpaa.rst: > > if (rte_bus_find_devargs(&rte_dpaa_bus, "oldev1") != NULL) { > /* create oldev1 */ > } > > No release notes for the new port type. dpaa.rst does not say the offline port > needs the /dev/cdx_ctrl kernel driver or where to get it. > > > Patch 1/3 drivers: add offline (O/H) port device support > -------------------------------------------------------- > > Error > > dpaa_oldev_init() never sets tx_queues[0].dpaa_intf, and the queue array comes > from rte_zmalloc(), so it is NULL. dpaa_eth_queue_tx() starts with: > > struct dpaa_if *dpaa_intf = fq->dpaa_intf; > ... > if (unlikely(dpaa_intf->ts_enable)) { > > so the first Tx burst after dev_start dereferences NULL. Set it after the Tx > FQ is > initialised: > > ret = dpaa_ol_tx_queue_init(&dpaa_intf->tx_queues[0], 0); > if (ret) > goto free_rx; > dpaa_intf->tx_queues[0].dpaa_intf = dpaa_intf; > > Warning > > rte_kvargs_count() only tests for presence, so "drv_oldev=0" enables the > offline > port. Moot if drv_oldev is removed as above; otherwise parse the value with > rte_kvargs_process(). > > dev_close() shuts down the Rx/Tx FQs and returns their FQIDs to the > allocator, but > the kernel was given those FQIDs and the bpid through > ASK_CTRL_SET_DPDK_INFO and is never told they are gone. The next > qman_alloc_fqid() user can receive frames the kernel enqueues to a reused > FQID. > The kernel side needs a reset ioctl issued before the FQIDs are released. > > The cell-index range checks in fman_if_init() and fman_if_init_onic() fix an > out-of- > bounds read of mac_idx[] and have nothing to do with the offline port. The > commit message does not mention them, nor the MEMMAC_REG_OFFSET > removal. Split the bounds checks into their own patch > with: > > Fixes: a0edbb8a8e52 ("bus/dpaa: add OH port mode") > Fixes: 7e5f49ae767d ("bus/dpaa: add ONIC port mode") > Cc: [email protected] > > drv_bh_port is required, but it is parsed on every dev_start() with a NULL > key list, > so a misspelled key is accepted at probe and the port only fails at start. > Parse it > once in dpaa_ol_probe() with a valid key list, store it in the private data, > and fail > probe if it is missing. > > If rte_pmd_dpaa_oldev.h stays, the installed header defines DPA_ISC_*, > DPDK_OLDEV_*, DPDK_CLASSIF_*, DPDK_TELECOM_* and struct dpa_* without > an RTE_/rte_ prefix, and none of the structs or fields have Doxygen. > > Info > > dpaa_ol_rx_queue_setup() checks queue_idx against nb_rx_queues; ethdev > already does that before calling the PMD. > > open(ASK_PATH, O_RDWR) should use O_RDWR | O_CLOEXEC. > > In dpaa_create_device_list(), use snprintf() rather than sprintf(), and drop > the > memset() after calloc() and the "int i = 0" initialiser. > > rte_dpaa_bus_dev_build() gates the num_ethports messages on oldev_enabled; > the condition that matters is dpaa_netcfg != NULL. > > The dpaa_bus_parse_bus_args() comment refers to a previous environment- > variable configuration that never existed upstream. The bh_port_name field > comment says "for testing purpose" but the field is mandatory. > > > Patch 2/3 net/dpaa: support non fmX-macY type of shared Ethernet name > --------------------------------------------------------------------- > > Warning > > Platform devices created from device-tree nodes without a reg property are > named after the whole parent chain, which is why dpaa_bus.c uses > "/sys/devices/platform/soc/soc:fsl,dpaa". A dpa-ethernet child of that node > is not > "ethernet@N" under /sys/bus/platform/devices, so > > node = strrchr(node_path, '/'); > ... > "/sys/bus/platform/devices/%s/net", node > > misses and silently falls back to the port name, which is the case the patch > is > meant to fix. Match on the netdev's of_node instead: > > /* for each /sys/class/net/<if> */ > snprintf(path, sizeof(path), "/sys/class/net/%s/device/of_node", > ifname); > if (realpath(path, resolved) != NULL && > strcmp(resolved + strlen("/sys/firmware/devicetree/base"), > node_path) == 0) > /* found */ > > Info > > The new dpaa.rst bullet lands at the end of the bus devargs block; it belongs > with > the shared MAC description. > > > Patch 3/3 drivers: add DPAA cgrid cleanup support > ------------------------------------------------- > > Warning > > This fixes a CGRID leak on close and a stale portal list entry on probe > failure, but > has no Fixes: tag or stable Cc: > > Fixes: 62f53995caaf ("net/dpaa: add frame count based tail drop with > CGR") > Fixes: 9124e65dd3eb ("net/dpaa: enable Tx queue taildrop") > Cc: [email protected] > > On the probe error path the initialised Rx FQs (and Tx FQs on free_tx) were > scheduled with QM_FQCTRL_CGE and their cgid, and are never shut down before > > for (loop = 0; loop < nb_rx_cgr; loop++) > qman_delete_cgr(&dpaa_intf->cgr_rx[loop]); > if (rx_cgrid_allocated) > qman_release_cgrid_range(cgrid[0], num_rx_fqs); > > That deletes CGRs that still have members, which the commit message says must > not happen, and hands the CGRIDs to the next probe while live FQs reference > them. Shut down the FQs initialised so far before deleting the CGRs. > > dev_close() releases the CGRID range unconditionally, even when > dpaa_cgr_stale_fq_cleanup() hit an FQ that would not shut down or > qman_delete_cgr() failed. 1/3 deliberately leaks FQIDs in that case; do the > same > here and skip qman_release_cgrid_range() if any delete or shutdown failed. > > dpaa_eth_dev_stop() and dpaa_eth_dev_close() never shut down the port's own > Rx/Tx FQs, so the FQs dpaa_cgr_stale_fq_cleanup() finds at close are this > port's > live queues, not ones "not cleaned in previous program". The driver already > holds > their FQIDs. Shut them down directly before deleting the CGRs, drop the FQID > space scan, and with it the new > qman_pending_fq_by_cgrid_range() export: > > for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++) > qman_shutdown_fq(&dpaa_intf->rx_queues[loop]); > for (loop = 0; loop < MAX_DPAA_CORES; loop++) > qman_shutdown_fq(&dpaa_intf->tx_queues[loop]); > > The existing "Release congestion Groups after releasing FQIDs" comment is then > true. > > Info > > cgrid is u32; use %u, not %d, in the DPAA_PMD_DEBUG/WARN calls. All AI comments are handled in V2.
