[net PATCH V3 0/3] Fix PFC related issues

[net PATCH V3 0/3] Fix PFC related issues

[Suman Ghosh]

This patchset fixes multiple PFC related issues related to Octeon.

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

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

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

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

Suman Ghosh (2):
  octeontx2-pf: Fix PFC TX scheduler free
  cteonxt2-pf: Fix backpressure config for multiple PFC priorities to
    work simultaneously

---
v3 changes:
	Removed patch #1 from v2 patchset due to review comment from Jakub.
	I will find a more accurate fix for that and will push a separate
	patch.

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        | 19 ++++++-------------
 3 files changed, 16 insertions(+), 21 deletions(-)

-- 
2.25.1

[net PATCH V3 1/3] 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 ccaf97bb1ce0..6492749dd7c8 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

Re: [net PATCH V3 1/3] octeontx2-pf: Fix PFC TX scheduler free

[Simon Horman]

On Mon, Aug 21, 2023 at 10:55:14AM +0530, Suman Ghosh wrote:
> 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);

Hi Suman,

Given that the licence of both this file and otx2_dcbnl.c is GPLv2,
I wonder if EXPORT_SYMBOL_GPL would be more appropriate here.

>  
>  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 ccaf97bb1ce0..6492749dd7c8 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
> 
> 

Re: [net PATCH V3 1/3] octeontx2-pf: Fix PFC TX scheduler free

[Paolo Abeni]

On Tue, 2023-08-22 at 09:11 +0200, Simon Horman wrote:
> On Mon, Aug 21, 2023 at 10:55:14AM +0530, Suman Ghosh wrote:
> > 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);
> 
> Hi Suman,
> 
> Given that the licence of both this file and otx2_dcbnl.c is GPLv2,
> I wonder if EXPORT_SYMBOL_GPL would be more appropriate here.

AFAICS all the symbols exported by otx2_common use plain
EXPORT_SYMBOL(). I think we can keep that for consistency in a -net
patch. In the long run it would be nice to move all of them to
EXPORT_SYMBOL_GPL :)

Cheers,

Paolo

Re: [net PATCH V3 1/3] octeontx2-pf: Fix PFC TX scheduler free

[Simon Horman]

On Tue, Aug 22, 2023 at 12:58:04PM +0200, Paolo Abeni wrote:
> On Tue, 2023-08-22 at 09:11 +0200, Simon Horman wrote:
> > On Mon, Aug 21, 2023 at 10:55:14AM +0530, Suman Ghosh wrote:
> > > 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);
> > 
> > Hi Suman,
> > 
> > Given that the licence of both this file and otx2_dcbnl.c is GPLv2,
> > I wonder if EXPORT_SYMBOL_GPL would be more appropriate here.
> 
> AFAICS all the symbols exported by otx2_common use plain
> EXPORT_SYMBOL(). I think we can keep that for consistency in a -net
> patch.

Sure, no objection.

> In the long run it would be nice to move all of them to
> EXPORT_SYMBOL_GPL :)
> 
> Cheers,
> 
> Paolo
> 

RE: [EXT] Re: [net PATCH V3 1/3] octeontx2-pf: Fix PFC TX scheduler free

[Suman Ghosh]

>> >
>> > Hi Suman,
>> >
>> > Given that the licence of both this file and otx2_dcbnl.c is GPLv2,
>> > I wonder if EXPORT_SYMBOL_GPL would be more appropriate here.
>>
>> AFAICS all the symbols exported by otx2_common use plain
>> EXPORT_SYMBOL(). I think we can keep that for consistency in a -net
>> patch.
>
>Sure, no objection.
>
>> In the long run it would be nice to move all of them to
>> EXPORT_SYMBOL_GPL :)
>>
>> Cheers,
>>
>> Paolo
[Suman] Sure Paolo, we can push a separate patch in future.
>>

[net PATCH V3 2/3] 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

Re: [net PATCH V3 2/3] octeontx2-af: CN10KB: fix PFC configuration

[Simon Horman]

On Mon, Aug 21, 2023 at 10:55:15AM +0530, Suman Ghosh wrote:
> 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.

Hi Suman,

I think it would be useful to describe what the bug is - it seems like an
incorrect mask in some cases - and how that might affect users. Better
still would be commands for an example usage where the problem previously
manifested.

> 
> 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;

Maybe it is overkill, but as this appears at least twice,
perhaps a helper would be appropriate.

> +	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
> 
> 

Re: [net PATCH V3 2/3] octeontx2-af: CN10KB: fix PFC configuration

[Paolo Abeni]

On Tue, 2023-08-22 at 09:16 +0200, Simon Horman wrote:
> On Mon, Aug 21, 2023 at 10:55:15AM +0530, Suman Ghosh wrote:
> > 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.
> 
> Hi Suman,
> 
> I think it would be useful to describe what the bug is - it seems like an
> incorrect mask in some cases - and how that might affect users. Better
> still would be commands for an example usage where the problem previously
> manifested.

Suman, please address Simon's feedback above in the new iteration.

> > 
> > 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;
> 
> Maybe it is overkill, but as this appears at least twice,
> perhaps a helper would be appropriate.

I think this is a matter of personal preferences (there is another
similar chunk with will not fit an helper, short of implementing it
with a somewhat ugly macro. So the overall code would be asymmetric), 

I'm fine either way.

Cheers,

Paolo

Re: [net PATCH V3 2/3] octeontx2-af: CN10KB: fix PFC configuration

[Simon Horman]

On Tue, Aug 22, 2023 at 01:12:26PM +0200, Paolo Abeni wrote:
> On Tue, 2023-08-22 at 09:16 +0200, Simon Horman wrote:
> > On Mon, Aug 21, 2023 at 10:55:15AM +0530, Suman Ghosh wrote:
> > > 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.
> > 
> > Hi Suman,
> > 
> > I think it would be useful to describe what the bug is - it seems like an
> > incorrect mask in some cases - and how that might affect users. Better
> > still would be commands for an example usage where the problem previously
> > manifested.
> 
> Suman, please address Simon's feedback above in the new iteration.
> 
> > > 
> > > 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;
> > 
> > Maybe it is overkill, but as this appears at least twice,
> > perhaps a helper would be appropriate.
> 
> I think this is a matter of personal preferences (there is another
> similar chunk with will not fit an helper, short of implementing it
> with a somewhat ugly macro. So the overall code would be asymmetric), 
> 
> I'm fine either way.

Likewise, I don't feel strongly either way.

RE: [EXT] Re: [net PATCH V3 2/3] octeontx2-af: CN10KB: fix PFC configuration

[Suman Ghosh]

>----------------------------------------------------------------------
>On Tue, Aug 22, 2023 at 01:12:26PM +0200, Paolo Abeni wrote:
>> On Tue, 2023-08-22 at 09:16 +0200, Simon Horman wrote:
>> > On Mon, Aug 21, 2023 at 10:55:15AM +0530, Suman Ghosh wrote:
>> > > 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.
>> >
>> > Hi Suman,
>> >
>> > I think it would be useful to describe what the bug is - it seems
>> > like an incorrect mask in some cases - and how that might affect
>> > users. Better still would be commands for an example usage where the
>> > problem previously manifested.
>>
>> Suman, please address Simon's feedback above in the new iteration.
[Suman] Sure. I will update in V4
>>
>> > >
>> > > 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;
>> >
>> > Maybe it is overkill, but as this appears at least twice, perhaps a
>> > helper would be appropriate.
>>
>> I think this is a matter of personal preferences (there is another
>> similar chunk with will not fit an helper, short of implementing it
>> with a somewhat ugly macro. So the overall code would be asymmetric),
>>
>> I'm fine either way.
>
>Likewise, I don't feel strongly either way.
[Suman] Then, I would like to keep it as is. I can consider macro/function if we are using more of it in future.

[net PATCH V3 3/3] cteonxt2-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>
---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c | 6 +++---
 1 file changed, 3 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 6492749dd7c8..bfddbff7bcdf 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]);
 
-- 
2.25.1

Re: [net PATCH V3 3/3] cteonxt2-pf: Fix backpressure config for multiple PFC priorities to work simultaneously

[Simon Horman]

On Mon, Aug 21, 2023 at 10:55:16AM +0530, Suman Ghosh wrote:
> 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>

Reviewed-by: Simon Horman <horms@kernel.org>