[net PATCH V2 0/4] Fix PFC related issues

[net PATCH V2 0/4] Fix PFC related issues

[Suman Ghosh]

This patchset fixes multiple PFC related issues related to Octeon.

Patch #1: octeontx2-pf: Update PFC configuration

Patch #2: octeontx2-pf: Fix PFC TX scheduler free

Patch #3: octeontx2-af: CN10KB: fix PFC configuration

Patch #4: octeonxt2-pf: Fix backpressure config for multiple PFC
priorities to work simultaneously

Hariprasad Kelam (1):
  octeontx2-af: CN10KB: fix PFC configuration

Suman Ghosh (3):
  octeontx2-pf: Update PFC configuration
  octeontx2-pf: Fix PFC TX scheduler free
  octeonxt2-pf: Fix backpressure config for multiple PFC priorities to
    work simultaneously

---
v2 changes:
	1. Fixed compilation error in patch #2
	ERROR: modpost: "otx2_txschq_free_one"
	[drivers/net/ethernet/marvell/octeontx2/nic/rvu_nicvf.ko] undefined!
	2. Added new patch #4 to the patch set. This patch fixes another PFC
	related issue.

 .../net/ethernet/marvell/octeontx2/af/rpm.c   | 17 ++++-----
 .../marvell/octeontx2/nic/otx2_common.c       |  1 +
 .../marvell/octeontx2/nic/otx2_dcbnl.c        | 35 ++++++++-----------
 3 files changed, 24 insertions(+), 29 deletions(-)

-- 
2.25.1

[net PATCH V2 1/4] octeontx2-pf: Update PFC configuration

[Suman Ghosh]

As of now we are creating/deleting Tx schedulers when user is
setting PFC on/off. The problem is if we have a running traffic on
the interface and as we are updating the sq->smq mapping on the fly,
we might loose completion interrupt for some packets. As a result of
that a watchdog reset is hit from BQL.
This patch solves the issue by simply calling interface off/on APIs
which will reconfigure all the queues. We might loss the running traffic
momentarily but that should be fine.

Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
Signed-off-by: Suman Ghosh <sumang@marvell.com>
---
 .../net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c  | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
index ccaf97bb1ce0..d54edfa8fcc9 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
@@ -406,6 +406,7 @@ static int otx2_dcbnl_ieee_getpfc(struct net_device *dev, struct ieee_pfc *pfc)
 static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
 {
 	struct otx2_nic *pfvf = netdev_priv(dev);
+	bool if_up = netif_running(dev);
 	int err;
 
 	/* Save PFC configuration to interface */
@@ -426,14 +427,9 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
 	if (err)
 		return err;
 
-	/* Request Per channel Bpids */
-	if (pfc->pfc_en)
-		otx2_nix_config_bp(pfvf, true);
-
-	err = otx2_pfc_txschq_update(pfvf);
-	if (err) {
-		dev_err(pfvf->dev, "%s failed to update TX schedulers\n", __func__);
-		return err;
+	if (if_up) {
+		otx2_stop(dev);
+		otx2_open(dev);
 	}
 
 	return 0;
-- 
2.25.1

[net PATCH V2 2/4] octeontx2-pf: Fix PFC TX scheduler free

[Suman Ghosh]

During PFC TX schedulers free, flag TXSCHQ_FREE_ALL was being set
which caused free up all schedulers other than the PFC schedulers.
This patch fixes that to free only the PFC Tx schedulers.

Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
Signed-off-by: Suman Ghosh <sumang@marvell.com>
---
 .../ethernet/marvell/octeontx2/nic/otx2_common.c  |  1 +
 .../ethernet/marvell/octeontx2/nic/otx2_dcbnl.c   | 15 ++++-----------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 77c8f650f7ac..289371b8ce4f 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -804,6 +804,7 @@ void otx2_txschq_free_one(struct otx2_nic *pfvf, u16 lvl, u16 schq)
 
 	mutex_unlock(&pfvf->mbox.lock);
 }
+EXPORT_SYMBOL(otx2_txschq_free_one);
 
 void otx2_txschq_stop(struct otx2_nic *pfvf)
 {
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
index d54edfa8fcc9..c75435bab411 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
@@ -125,19 +125,12 @@ int otx2_pfc_txschq_alloc(struct otx2_nic *pfvf)
 
 static int otx2_pfc_txschq_stop_one(struct otx2_nic *pfvf, u8 prio)
 {
-	struct nix_txsch_free_req *free_req;
+	int lvl;
 
-	mutex_lock(&pfvf->mbox.lock);
 	/* free PFC TLx nodes */
-	free_req = otx2_mbox_alloc_msg_nix_txsch_free(&pfvf->mbox);
-	if (!free_req) {
-		mutex_unlock(&pfvf->mbox.lock);
-		return -ENOMEM;
-	}
-
-	free_req->flags = TXSCHQ_FREE_ALL;
-	otx2_sync_mbox_msg(&pfvf->mbox);
-	mutex_unlock(&pfvf->mbox.lock);
+	for (lvl = 0; lvl < pfvf->hw.txschq_link_cfg_lvl; lvl++)
+		otx2_txschq_free_one(pfvf, lvl,
+				     pfvf->pfc_schq_list[lvl][prio]);
 
 	pfvf->pfc_alloc_status[prio] = false;
 	return 0;
-- 
2.25.1

[net PATCH V2 3/4] octeontx2-af: CN10KB: fix PFC configuration

[Suman Ghosh]

From: Hariprasad Kelam <hkelam@marvell.com>

The previous patch which added new CN10KB RPM block support,
has a bug due to which PFC is not getting configured properly.
This patch fixes the same.

Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/af/rpm.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rpm.c b/drivers/net/ethernet/marvell/octeontx2/af/rpm.c
index b4fcb20c3f4f..af21e2030cff 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rpm.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rpm.c
@@ -355,8 +355,8 @@ int rpm_lmac_enadis_pause_frm(void *rpmd, int lmac_id, u8 tx_pause,
 
 void rpm_lmac_pause_frm_config(void *rpmd, int lmac_id, bool enable)
 {
+	u64 cfg, pfc_class_mask_cfg;
 	rpm_t *rpm = rpmd;
-	u64 cfg;
 
 	/* ALL pause frames received are completely ignored */
 	cfg = rpm_read(rpm, lmac_id, RPMX_MTI_MAC100X_COMMAND_CONFIG);
@@ -380,9 +380,11 @@ void rpm_lmac_pause_frm_config(void *rpmd, int lmac_id, bool enable)
 		rpm_write(rpm, 0, RPMX_CMR_CHAN_MSK_OR, ~0ULL);
 
 	/* Disable all PFC classes */
-	cfg = rpm_read(rpm, lmac_id, RPMX_CMRX_PRT_CBFC_CTL);
+	pfc_class_mask_cfg = is_dev_rpm2(rpm) ? RPM2_CMRX_PRT_CBFC_CTL :
+						RPMX_CMRX_PRT_CBFC_CTL;
+	cfg = rpm_read(rpm, lmac_id, pfc_class_mask_cfg);
 	cfg = FIELD_SET(RPM_PFC_CLASS_MASK, 0, cfg);
-	rpm_write(rpm, lmac_id, RPMX_CMRX_PRT_CBFC_CTL, cfg);
+	rpm_write(rpm, lmac_id, pfc_class_mask_cfg, cfg);
 }
 
 int rpm_get_rx_stats(void *rpmd, int lmac_id, int idx, u64 *rx_stat)
@@ -605,8 +607,11 @@ int rpm_lmac_pfc_config(void *rpmd, int lmac_id, u8 tx_pause, u8 rx_pause, u16 p
 	if (!is_lmac_valid(rpm, lmac_id))
 		return -ENODEV;
 
+	pfc_class_mask_cfg = is_dev_rpm2(rpm) ? RPM2_CMRX_PRT_CBFC_CTL :
+						RPMX_CMRX_PRT_CBFC_CTL;
+
 	cfg = rpm_read(rpm, lmac_id, RPMX_MTI_MAC100X_COMMAND_CONFIG);
-	class_en = rpm_read(rpm, lmac_id, RPMX_CMRX_PRT_CBFC_CTL);
+	class_en = rpm_read(rpm, lmac_id, pfc_class_mask_cfg);
 	pfc_en |= FIELD_GET(RPM_PFC_CLASS_MASK, class_en);
 
 	if (rx_pause) {
@@ -635,10 +640,6 @@ int rpm_lmac_pfc_config(void *rpmd, int lmac_id, u8 tx_pause, u8 rx_pause, u16 p
 		cfg |= RPMX_MTI_MAC100X_COMMAND_CONFIG_PFC_MODE;
 
 	rpm_write(rpm, lmac_id, RPMX_MTI_MAC100X_COMMAND_CONFIG, cfg);
-
-	pfc_class_mask_cfg = is_dev_rpm2(rpm) ? RPM2_CMRX_PRT_CBFC_CTL :
-						RPMX_CMRX_PRT_CBFC_CTL;
-
 	rpm_write(rpm, lmac_id, pfc_class_mask_cfg, class_en);
 
 	return 0;
-- 
2.25.1

[net PATCH V2 4/4] octeonxt2-pf: Fix backpressure config for multiple PFC priorities to work simultaneously

[Suman Ghosh]

MAC (CGX or RPM) asserts backpressure at TL3 or TL2 node of the egress
hierarchical scheduler tree depending on link level config done. If
there are multiple PFC priorities enabled at a time and for all such
flows to backoff, each priority will have to assert backpressure at
different TL3/TL2 scheduler nodes and these flows will need to submit
egress pkts to these nodes.

Current PFC configuration has an issue where in only one backpressure
scheduler node is being allocated which is resulting in only one PFC
priority to work. This patch fixes this issue.

Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
Signed-off-by: Suman Ghosh <sumang@marvell.com>
---
 .../net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c    | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
index c75435bab411..048ee015c085 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
@@ -70,7 +70,7 @@ static int otx2_pfc_txschq_alloc_one(struct otx2_nic *pfvf, u8 prio)
 	 * link config level. These rest of the scheduler can be
 	 * same as hw.txschq_list.
 	 */
-	for (lvl = 0; lvl < pfvf->hw.txschq_link_cfg_lvl; lvl++)
+	for (lvl = 0; lvl <= pfvf->hw.txschq_link_cfg_lvl; lvl++)
 		req->schq[lvl] = 1;
 
 	rc = otx2_sync_mbox_msg(&pfvf->mbox);
@@ -83,7 +83,7 @@ static int otx2_pfc_txschq_alloc_one(struct otx2_nic *pfvf, u8 prio)
 		return PTR_ERR(rsp);
 
 	/* Setup transmit scheduler list */
-	for (lvl = 0; lvl < pfvf->hw.txschq_link_cfg_lvl; lvl++) {
+	for (lvl = 0; lvl <= pfvf->hw.txschq_link_cfg_lvl; lvl++) {
 		if (!rsp->schq[lvl])
 			return -ENOSPC;
 
@@ -128,7 +128,7 @@ static int otx2_pfc_txschq_stop_one(struct otx2_nic *pfvf, u8 prio)
 	int lvl;
 
 	/* free PFC TLx nodes */
-	for (lvl = 0; lvl < pfvf->hw.txschq_link_cfg_lvl; lvl++)
+	for (lvl = 0; lvl <= pfvf->hw.txschq_link_cfg_lvl; lvl++)
 		otx2_txschq_free_one(pfvf, lvl,
 				     pfvf->pfc_schq_list[lvl][prio]);
 
@@ -400,9 +400,11 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
 {
 	struct otx2_nic *pfvf = netdev_priv(dev);
 	bool if_up = netif_running(dev);
+	u8 prev_pfc_en;
 	int err;
 
 	/* Save PFC configuration to interface */
+	prev_pfc_en = pfvf->pfc_en;
 	pfvf->pfc_en = pfc->pfc_en;
 
 	if (pfvf->hw.tx_queues >= NIX_PF_PFC_PRIO_MAX)
@@ -421,7 +423,9 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
 		return err;
 
 	if (if_up) {
+		pfvf->pfc_en = prev_pfc_en;
 		otx2_stop(dev);
+		pfvf->pfc_en = pfc->pfc_en;
 		otx2_open(dev);
 	}
 
-- 
2.25.1

Re: [net PATCH V2 1/4] octeontx2-pf: Update PFC configuration

[Jakub Kicinski]

On Wed, 9 Aug 2023 12:35:29 +0530 Suman Ghosh wrote:
> +		otx2_stop(dev);
> +		otx2_open(dev);

If there is any error in open() this will silently take the interface
down. Can't you force a NAPI poll or some such, if the concern is a
missed IRQ?
-- 
pw-bot: cr

RE: [EXT] Re: [net PATCH V2 1/4] octeontx2-pf: Update PFC configuration

[Suman Ghosh]

>----------------------------------------------------------------------
>On Wed, 9 Aug 2023 12:35:29 +0530 Suman Ghosh wrote:
>> +		otx2_stop(dev);
>> +		otx2_open(dev);
>
>If there is any error in open() this will silently take the interface
>down. Can't you force a NAPI poll or some such, if the concern is a
>missed IRQ?
[Suman] I can check the return type of open() and report in case of error. But even if we force NAPI poll we might not be able to control the watchdog reset. If we have a running traffic and interface is up, when we force NAPI poll, then the new packets will have updated scheduler queue and we will still loose the completion interrupts of the previous packets. Do you see any issue if I handle the error situation during open() call?
>--
>pw-bot: cr

Re: [EXT] Re: [net PATCH V2 1/4] octeontx2-pf: Update PFC configuration

[Jakub Kicinski]

Thanks for replying a week late, always a good use of maintainers time
to swap back all the context from random conversations!

On Fri, 18 Aug 2023 06:54:52 +0000 Suman Ghosh wrote:
> >If there is any error in open() this will silently take the interface
> >down. Can't you force a NAPI poll or some such, if the concern is a
> >missed IRQ?  
> [Suman] I can check the return type of open() and report in case of
> error. But even if we force NAPI poll we might not be able to control
> the watchdog reset. If we have a running traffic and interface is up,
> when we force NAPI poll, then the new packets will have updated
> scheduler queue and we will still loose the completion interrupts of
> the previous packets.

Why does it matter that you lost an interrupt if the poll has happened.
Can you describe the problem in more detail?

> Do you see any issue if I handle the error situation during open() call?

No, for years we have been rejecting code which does this.
If the machine is under memory pressure allocating all the buffers 
for rings can easily fail and make the machine drop off the network.
You either have to refuse to change this setting at runtime or
implement prepare/commit reconfiguration model like other modern
drivers, where allocations are done before the stop().

RE: [EXT] Re: [net PATCH V2 1/4] octeontx2-pf: Update PFC configuration

[Suman Ghosh]

>Thanks for replying a week late, always a good use of maintainers time
>to swap back all the context from random conversations!
[Suman] Sorry for being late this time Jakub. I will remove this patch from the patch set and will push a new version with the other three patches. I will analyze the issue in more detail and will produce a proper fix.
>
>On Fri, 18 Aug 2023 06:54:52 +0000 Suman Ghosh wrote:
>> >If there is any error in open() this will silently take the interface
>> >down. Can't you force a NAPI poll or some such, if the concern is a
>> >missed IRQ?
>> [Suman] I can check the return type of open() and report in case of
>> error. But even if we force NAPI poll we might not be able to control
>> the watchdog reset. If we have a running traffic and interface is up,
>> when we force NAPI poll, then the new packets will have updated
>> scheduler queue and we will still loose the completion interrupts of
>> the previous packets.
>
>Why does it matter that you lost an interrupt if the poll has happened.
>Can you describe the problem in more detail?
>
>> Do you see any issue if I handle the error situation during open()
>call?
>
>No, for years we have been rejecting code which does this.
>If the machine is under memory pressure allocating all the buffers for
>rings can easily fail and make the machine drop off the network.
>You either have to refuse to change this setting at runtime or implement
>prepare/commit reconfiguration model like other modern drivers, where
>allocations are done before the stop().