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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> ---
>  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
> 
>