[PATCH 01/17] bus/dpaa: fix PFDRs leaks due to FQRNIs

[PATCH 01/17] bus/dpaa: fix PFDRs leaks due to FQRNIs

[Hemant Agrawal]

From: Gagandeep Singh <g.singh@nxp.com>

When a Retire FQ command is executed on a FQ in the
Tentatively Scheduled or Parked states, in that case FQ
is retired immediately and a FQRNI (Frame Queue Retirement
Notification Immediate) message is generated. Software
must read this message from MR and consume it to free
the memory used by it.

Although it is not mentioned about which memory to be used
by FQRNIs in the RM but through experiments it is proven
that it can use PFDRs. So if these messages are allowed to
build up indefinitely then PFDR resources can become exhausted
and cause enqueues to stall. Therefore software must consume
these MR messages on a regular basis to avoid depleting
the available PFDR resources.

This is the PFDRs leak issue which user can experienace while
using the DPDK crypto driver and creating and destroying the
sessions multiple times. On a session destroy, DPDK calls the
qman_retire_fq() for each FQ used by the session, but it does
not handle the FQRNIs generated and allowed them to build up
indefinitely in MR.

This patch fixes this issue by consuming the FQRNIs received
from MR immediately after FQ retire by calling drain_mr_fqrni().

Please note that this drain_mr_fqrni() only look for
FQRNI type messages to consume. If there are other type of messages
like FQRN, FQRL, FQPN, ERN etc. also coming on MR then those
messages need to be handled separately.

Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
Cc: stable@dpdk.org

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 drivers/bus/dpaa/base/qbman/qman.c | 46 ++++++++++++++++--------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index 301057723e..9c90ee25a6 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -292,10 +292,32 @@ static inline void qman_stop_dequeues_ex(struct qman_portal *p)
 		qm_dqrr_set_maxfill(&p->p, 0);
 }
 
+static inline void qm_mr_pvb_update(struct qm_portal *portal)
+{
+	register struct qm_mr *mr = &portal->mr;
+	const struct qm_mr_entry *res = qm_cl(mr->ring, mr->pi);
+
+#ifdef RTE_LIBRTE_DPAA_HWDEBUG
+	DPAA_ASSERT(mr->pmode == qm_mr_pvb);
+#endif
+	/* when accessing 'verb', use __raw_readb() to ensure that compiler
+	 * inlining doesn't try to optimise out "excess reads".
+	 */
+	if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) {
+		mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1);
+		if (!mr->pi)
+			mr->vbit ^= QM_MR_VERB_VBIT;
+		mr->fill++;
+		res = MR_INC(res);
+	}
+	dcbit_ro(res);
+}
+
 static int drain_mr_fqrni(struct qm_portal *p)
 {
 	const struct qm_mr_entry *msg;
 loop:
+	qm_mr_pvb_update(p);
 	msg = qm_mr_current(p);
 	if (!msg) {
 		/*
@@ -317,6 +339,7 @@ static int drain_mr_fqrni(struct qm_portal *p)
 		do {
 			now = mfatb();
 		} while ((then + 10000) > now);
+		qm_mr_pvb_update(p);
 		msg = qm_mr_current(p);
 		if (!msg)
 			return 0;
@@ -479,27 +502,6 @@ static inline int qm_mr_init(struct qm_portal *portal,
 	return 0;
 }
 
-static inline void qm_mr_pvb_update(struct qm_portal *portal)
-{
-	register struct qm_mr *mr = &portal->mr;
-	const struct qm_mr_entry *res = qm_cl(mr->ring, mr->pi);
-
-#ifdef RTE_LIBRTE_DPAA_HWDEBUG
-	DPAA_ASSERT(mr->pmode == qm_mr_pvb);
-#endif
-	/* when accessing 'verb', use __raw_readb() to ensure that compiler
-	 * inlining doesn't try to optimise out "excess reads".
-	 */
-	if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) {
-		mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1);
-		if (!mr->pi)
-			mr->vbit ^= QM_MR_VERB_VBIT;
-		mr->fill++;
-		res = MR_INC(res);
-	}
-	dcbit_ro(res);
-}
-
 struct qman_portal *
 qman_init_portal(struct qman_portal *portal,
 		   const struct qm_portal_config *c,
@@ -1794,6 +1796,8 @@ int qman_retire_fq(struct qman_fq *fq, u32 *flags)
 	}
 out:
 	FQUNLOCK(fq);
+	/* Draining FQRNIs, if any */
+	drain_mr_fqrni(&p->p);
 	return rval;
 }
 
-- 
2.25.1

[PATCH 02/17] net/dpaa: fix typecasting ch ID to u32

[Hemant Agrawal]

From: Rohit Raj <rohit.raj@nxp.com>

Avoid typecasting ch_id to u32 and passing it to another API since it
can corrupt other data. Instead, create new u32 variable and typecase
it back to u16 after it gets updated by the API.
NXP CID: 27996293

Fixes: 0c504f6950b6 ("net/dpaa: support push mode")
Cc: hemant.agrawal@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
---
 drivers/net/dpaa/dpaa_ethdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 060b8c678f..1a2de5240f 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -972,7 +972,7 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 	struct fman_if *fif = dev->process_private;
 	struct qman_fq *rxq = &dpaa_intf->rx_queues[queue_idx];
 	struct qm_mcc_initfq opts = {0};
-	u32 flags = 0;
+	u32 ch_id, flags = 0;
 	int ret;
 	u32 buffsz = rte_pktmbuf_data_room_size(mp) - RTE_PKTMBUF_HEADROOM;
 	uint32_t max_rx_pktlen;
@@ -1096,7 +1096,9 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 						DPAA_IF_RX_CONTEXT_STASH;
 
 		/*Create a channel and associate given queue with the channel*/
-		qman_alloc_pool_range((u32 *)&rxq->ch_id, 1, 1, 0);
+		qman_alloc_pool_range(&ch_id, 1, 1, 0);
+		rxq->ch_id = (u16)ch_id;
+
 		opts.we_mask = opts.we_mask | QM_INITFQ_WE_DESTWQ;
 		opts.fqd.dest.channel = rxq->ch_id;
 		opts.fqd.dest.wq = DPAA_IF_RX_PRIORITY;
-- 
2.25.1

[PATCH 03/17] bus/dpaa: fix VSP for 1G fm1-mac9 and 10

[Hemant Agrawal]

No need to classify interface separately for 1G and 10G

Fixes: e0718bb2ca95 ("bus/dpaa: add virtual storage profile port init")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman.c b/drivers/bus/dpaa/base/fman/fman.c
index 41195eb0a7..beeb03dbf2 100644
--- a/drivers/bus/dpaa/base/fman/fman.c
+++ b/drivers/bus/dpaa/base/fman/fman.c
@@ -153,7 +153,7 @@ static void fman_if_vsp_init(struct __fman_if *__if)
 	size_t lenp;
 	const uint8_t mac_idx[] = {-1, 0, 1, 2, 3, 4, 5, 6, 7, 0, 1};
 
-	if (__if->__if.mac_type == fman_mac_1g) {
+	if (__if->__if.mac_idx <= 8) {
 		for_each_compatible_node(dev, NULL,
 			"fsl,fman-port-1g-rx-extended-args") {
 			prop = of_get_property(dev, "cell-index", &lenp);
@@ -176,7 +176,32 @@ static void fman_if_vsp_init(struct __fman_if *__if)
 				}
 			}
 		}
-	} else if (__if->__if.mac_type == fman_mac_10g) {
+
+		for_each_compatible_node(dev, NULL,
+					 "fsl,fman-port-op-extended-args") {
+			prop = of_get_property(dev, "cell-index", &lenp);
+
+			if (prop) {
+				cell_index = of_read_number(&prop[0],
+						lenp / sizeof(phandle));
+
+				if (cell_index == __if->__if.mac_idx) {
+					prop = of_get_property(dev,
+							       "vsp-window",
+							       &lenp);
+
+					if (prop) {
+						__if->__if.num_profiles =
+							of_read_number(&prop[0],
+								       1);
+						__if->__if.base_profile_id =
+							of_read_number(&prop[1],
+								       1);
+					}
+				}
+			}
+		}
+	} else {
 		for_each_compatible_node(dev, NULL,
 			"fsl,fman-port-10g-rx-extended-args") {
 			prop = of_get_property(dev, "cell-index", &lenp);
-- 
2.25.1

Re: [PATCH 02/17] net/dpaa: fix typecasting ch ID to u32

[Ferruh Yigit]

On 8/1/2024 11:52 AM, Hemant Agrawal wrote:
> From: Rohit Raj <rohit.raj@nxp.com>
> 
> Avoid typecasting ch_id to u32 and passing it to another API since it
> can corrupt other data. Instead, create new u32 variable and typecase
> it back to u16 after it gets updated by the API.
>

Ack on the issue and the solution.

> NXP CID: 27996293
> 

internal tracking id can be dropped

> Fixes: 0c504f6950b6 ("net/dpaa: support push mode")
> Cc: hemant.agrawal@nxp.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
>

<...>

Re: [PATCH 03/17] bus/dpaa: fix VSP for 1G fm1-mac9 and 10

[Ferruh Yigit]

On 8/1/2024 11:52 AM, Hemant Agrawal wrote:
> No need to classify interface separately for 1G and 10G
> 
> Fixes: e0718bb2ca95 ("bus/dpaa: add virtual storage profile port init")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>

Can you please briefly explain what is VSP in the commit log, at worst
provide what the acronym stands for.

[PATCH v2 01/18] bus/dpaa: fix PFDRs leaks due to FQRNIs

[Hemant Agrawal]

From: Gagandeep Singh <g.singh@nxp.com>

When a Retire FQ command is executed on a FQ in the
Tentatively Scheduled or Parked states, in that case FQ
is retired immediately and a FQRNI (Frame Queue Retirement
Notification Immediate) message is generated. Software
must read this message from MR and consume it to free
the memory used by it.

Although it is not mentioned about which memory to be used
by FQRNIs in the RM but through experiments it is proven
that it can use PFDRs. So if these messages are allowed to
build up indefinitely then PFDR resources can become exhausted
and cause enqueues to stall. Therefore software must consume
these MR messages on a regular basis to avoid depleting
the available PFDR resources.

This is the PFDRs leak issue which user can experienace while
using the DPDK crypto driver and creating and destroying the
sessions multiple times. On a session destroy, DPDK calls the
qman_retire_fq() for each FQ used by the session, but it does
not handle the FQRNIs generated and allowed them to build up
indefinitely in MR.

This patch fixes this issue by consuming the FQRNIs received
from MR immediately after FQ retire by calling drain_mr_fqrni().

Please note that this drain_mr_fqrni() only look for
FQRNI type messages to consume. If there are other type of messages
like FQRN, FQRL, FQPN, ERN etc. also coming on MR then those
messages need to be handled separately.

Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
Cc: stable@dpdk.org

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 drivers/bus/dpaa/base/qbman/qman.c | 46 ++++++++++++++++--------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index 301057723e..9c90ee25a6 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -292,10 +292,32 @@ static inline void qman_stop_dequeues_ex(struct qman_portal *p)
 		qm_dqrr_set_maxfill(&p->p, 0);
 }
 
+static inline void qm_mr_pvb_update(struct qm_portal *portal)
+{
+	register struct qm_mr *mr = &portal->mr;
+	const struct qm_mr_entry *res = qm_cl(mr->ring, mr->pi);
+
+#ifdef RTE_LIBRTE_DPAA_HWDEBUG
+	DPAA_ASSERT(mr->pmode == qm_mr_pvb);
+#endif
+	/* when accessing 'verb', use __raw_readb() to ensure that compiler
+	 * inlining doesn't try to optimise out "excess reads".
+	 */
+	if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) {
+		mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1);
+		if (!mr->pi)
+			mr->vbit ^= QM_MR_VERB_VBIT;
+		mr->fill++;
+		res = MR_INC(res);
+	}
+	dcbit_ro(res);
+}
+
 static int drain_mr_fqrni(struct qm_portal *p)
 {
 	const struct qm_mr_entry *msg;
 loop:
+	qm_mr_pvb_update(p);
 	msg = qm_mr_current(p);
 	if (!msg) {
 		/*
@@ -317,6 +339,7 @@ static int drain_mr_fqrni(struct qm_portal *p)
 		do {
 			now = mfatb();
 		} while ((then + 10000) > now);
+		qm_mr_pvb_update(p);
 		msg = qm_mr_current(p);
 		if (!msg)
 			return 0;
@@ -479,27 +502,6 @@ static inline int qm_mr_init(struct qm_portal *portal,
 	return 0;
 }
 
-static inline void qm_mr_pvb_update(struct qm_portal *portal)
-{
-	register struct qm_mr *mr = &portal->mr;
-	const struct qm_mr_entry *res = qm_cl(mr->ring, mr->pi);
-
-#ifdef RTE_LIBRTE_DPAA_HWDEBUG
-	DPAA_ASSERT(mr->pmode == qm_mr_pvb);
-#endif
-	/* when accessing 'verb', use __raw_readb() to ensure that compiler
-	 * inlining doesn't try to optimise out "excess reads".
-	 */
-	if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) {
-		mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1);
-		if (!mr->pi)
-			mr->vbit ^= QM_MR_VERB_VBIT;
-		mr->fill++;
-		res = MR_INC(res);
-	}
-	dcbit_ro(res);
-}
-
 struct qman_portal *
 qman_init_portal(struct qman_portal *portal,
 		   const struct qm_portal_config *c,
@@ -1794,6 +1796,8 @@ int qman_retire_fq(struct qman_fq *fq, u32 *flags)
 	}
 out:
 	FQUNLOCK(fq);
+	/* Draining FQRNIs, if any */
+	drain_mr_fqrni(&p->p);
 	return rval;
 }
 
-- 
2.25.1

[PATCH v2 02/18] net/dpaa: fix typecasting ch ID to u32

[Hemant Agrawal]

From: Rohit Raj <rohit.raj@nxp.com>

Avoid typecasting ch_id to u32 and passing it to another API since it
can corrupt other data. Instead, create new u32 variable and typecase
it back to u16 after it gets updated by the API.

Fixes: 0c504f6950b6 ("net/dpaa: support push mode")
Cc: hemant.agrawal@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Rohit Raj <rohit.raj@nxp.com>
---
 drivers/net/dpaa/dpaa_ethdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 060b8c678f..1a2de5240f 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -972,7 +972,7 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 	struct fman_if *fif = dev->process_private;
 	struct qman_fq *rxq = &dpaa_intf->rx_queues[queue_idx];
 	struct qm_mcc_initfq opts = {0};
-	u32 flags = 0;
+	u32 ch_id, flags = 0;
 	int ret;
 	u32 buffsz = rte_pktmbuf_data_room_size(mp) - RTE_PKTMBUF_HEADROOM;
 	uint32_t max_rx_pktlen;
@@ -1096,7 +1096,9 @@ int dpaa_eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 						DPAA_IF_RX_CONTEXT_STASH;
 
 		/*Create a channel and associate given queue with the channel*/
-		qman_alloc_pool_range((u32 *)&rxq->ch_id, 1, 1, 0);
+		qman_alloc_pool_range(&ch_id, 1, 1, 0);
+		rxq->ch_id = (u16)ch_id;
+
 		opts.we_mask = opts.we_mask | QM_INITFQ_WE_DESTWQ;
 		opts.fqd.dest.channel = rxq->ch_id;
 		opts.fqd.dest.wq = DPAA_IF_RX_PRIORITY;
-- 
2.25.1

[PATCH v2 03/18] bus/dpaa: fix VSP for 1G fm1-mac9 and 10

[Hemant Agrawal]

No need to classify interface separately for 1G and 10G
Note that VSP or Virtual storage profile are DPAA equivalent for SRIOV
config to logically divide a physical ports in virtual ports.

Fixes: e0718bb2ca95 ("bus/dpaa: add virtual storage profile port init")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman.c b/drivers/bus/dpaa/base/fman/fman.c
index 41195eb0a7..beeb03dbf2 100644
--- a/drivers/bus/dpaa/base/fman/fman.c
+++ b/drivers/bus/dpaa/base/fman/fman.c
@@ -153,7 +153,7 @@ static void fman_if_vsp_init(struct __fman_if *__if)
 	size_t lenp;
 	const uint8_t mac_idx[] = {-1, 0, 1, 2, 3, 4, 5, 6, 7, 0, 1};
 
-	if (__if->__if.mac_type == fman_mac_1g) {
+	if (__if->__if.mac_idx <= 8) {
 		for_each_compatible_node(dev, NULL,
 			"fsl,fman-port-1g-rx-extended-args") {
 			prop = of_get_property(dev, "cell-index", &lenp);
@@ -176,7 +176,32 @@ static void fman_if_vsp_init(struct __fman_if *__if)
 				}
 			}
 		}
-	} else if (__if->__if.mac_type == fman_mac_10g) {
+
+		for_each_compatible_node(dev, NULL,
+					 "fsl,fman-port-op-extended-args") {
+			prop = of_get_property(dev, "cell-index", &lenp);
+
+			if (prop) {
+				cell_index = of_read_number(&prop[0],
+						lenp / sizeof(phandle));
+
+				if (cell_index == __if->__if.mac_idx) {
+					prop = of_get_property(dev,
+							       "vsp-window",
+							       &lenp);
+
+					if (prop) {
+						__if->__if.num_profiles =
+							of_read_number(&prop[0],
+								       1);
+						__if->__if.base_profile_id =
+							of_read_number(&prop[1],
+								       1);
+					}
+				}
+			}
+		}
+	} else {
 		for_each_compatible_node(dev, NULL,
 			"fsl,fman-port-10g-rx-extended-args") {
 			prop = of_get_property(dev, "cell-index", &lenp);
-- 
2.25.1

[PATCH v2 04/18] bus/dpaa: fix the fman details status

[Hemant Agrawal]

Fix the incorrect placing of brackets to calculate stats.

This corrects the "(a | b) << 32"  to  "a | (b << 32)"

Fixes: e62a3f4183f1 ("bus/dpaa: fix statistics reading")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/fman/fman_hw.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
index 24a99f7235..97e792806f 100644
--- a/drivers/bus/dpaa/base/fman/fman_hw.c
+++ b/drivers/bus/dpaa/base/fman/fman_hw.c
@@ -243,10 +243,11 @@ fman_if_stats_get_all(struct fman_if *p, uint64_t *value, int n)
 	int i;
 	uint64_t base_offset = offsetof(struct memac_regs, reoct_l);
 
-	for (i = 0; i < n; i++)
-		value[i] = (((u64)in_be32((char *)regs + base_offset + 8 * i) |
-				(u64)in_be32((char *)regs + base_offset +
-				8 * i + 4)) << 32);
+	for (i = 0; i < n; i++) {
+		uint64_t a = in_be32((char *)regs + base_offset + 8 * i);
+		uint64_t b = in_be32((char *)regs + base_offset + 8 * i + 4);
+		value[i] = a | b << 32;
+	}
 }
 
 void
-- 
2.25.1

Re: [PATCH 03/17] bus/dpaa: fix VSP for 1G fm1-mac9 and 10

[Hemant Agrawal]

On 07-08-2024 21:08, Ferruh Yigit wrote:
> On 8/1/2024 11:52 AM, Hemant Agrawal wrote:
>> No need to classify interface separately for 1G and 10G
>>
>> Fixes: e0718bb2ca95 ("bus/dpaa: add virtual storage profile port init")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>
> Can you please briefly explain what is VSP in the commit log, at worst
> provide what the acronym stands for.
Fixed the commit logs. VSP is virtual storage profile. It is a SRIOV 
equivalent in DPAA for creating VF interfaces with VSP.