diff mbox series

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

Message ID 20230821052516.398572-3-sumang@marvell.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Fix PFC related issues | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 1351 this patch: 1351
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1351 this patch: 1351
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Suman Ghosh Aug. 21, 2023, 5:25 a.m. UTC 
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(-)

Comments

Simon Horman Aug. 22, 2023, 7:16 a.m. UTC | #1 
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
> 
>
Paolo Abeni Aug. 22, 2023, 11:12 a.m. UTC | #2 
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
Simon Horman Aug. 22, 2023, 8:06 p.m. UTC | #3 
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.
Suman Ghosh Aug. 23, 2023, 1:16 p.m. UTC | #4 
>----------------------------------------------------------------------
>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.
diff mbox series

Patch

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;