[net PATCH] octeontx2-pf: Fix graceful exit during PFC configuration failure

[net PATCH] octeontx2-pf: Fix graceful exit during PFC configuration failure

[Suman Ghosh]

During PFC configuration failure the code was not handling a graceful
exit. This patch fixes the same and add proper code for a graceful exit.

Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
Signed-off-by: Suman Ghosh <sumang@marvell.com>
---
 .../ethernet/marvell/octeontx2/nic/otx2_dcbnl.c | 17 ++++++++++++++---
 1 file changed, 14 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 bfddbff7bcdf..28fb643d2917 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
@@ -399,9 +399,10 @@ 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);
+	u8 old_pfc_en;
 	int err;
 
-	/* Save PFC configuration to interface */
+	old_pfc_en = pfvf->pfc_en;
 	pfvf->pfc_en = pfc->pfc_en;
 
 	if (pfvf->hw.tx_queues >= NIX_PF_PFC_PRIO_MAX)
@@ -411,13 +412,17 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
 	 * supported by the tx queue configuration
 	 */
 	err = otx2_check_pfc_config(pfvf);
-	if (err)
+	if (err) {
+		pfvf->pfc_en = old_pfc_en;
 		return err;
+	}
 
 process_pfc:
 	err = otx2_config_priority_flow_ctrl(pfvf);
-	if (err)
+	if (err) {
+		pfvf->pfc_en = old_pfc_en;
 		return err;
+	}
 
 	/* Request Per channel Bpids */
 	if (pfc->pfc_en)
@@ -425,6 +430,12 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
 
 	err = otx2_pfc_txschq_update(pfvf);
 	if (err) {
+		if (pfc->pfc_en)
+			otx2_nix_config_bp(pfvf, false);
+
+		otx2_pfc_txschq_stop(pfvf);
+		pfvf->pfc_en = old_pfc_en;
+		otx2_config_priority_flow_ctrl(pfvf);
 		dev_err(pfvf->dev, "%s failed to update TX schedulers\n", __func__);
 		return err;
 	}
-- 
2.25.1

Re: [net PATCH] octeontx2-pf: Fix graceful exit during PFC configuration failure

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 13 Dec 2023 23:40:44 +0530 you wrote:
> During PFC configuration failure the code was not handling a graceful
> exit. This patch fixes the same and add proper code for a graceful exit.
> 
> Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
> Signed-off-by: Suman Ghosh <sumang@marvell.com>
> ---
>  .../ethernet/marvell/octeontx2/nic/otx2_dcbnl.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Here is the summary with links:
  - [net] octeontx2-pf: Fix graceful exit during PFC configuration failure
    https://git.kernel.org/netdev/net/c/8c97ab5448f2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [net PATCH] octeontx2-pf: Fix graceful exit during PFC configuration failure

[Simon Horman]

On Wed, Dec 13, 2023 at 11:40:44PM +0530, Suman Ghosh wrote:
> During PFC configuration failure the code was not handling a graceful
> exit. This patch fixes the same and add proper code for a graceful exit.
> 
> Fixes: 99c969a83d82 ("octeontx2-pf: Add egress PFC support")
> Signed-off-by: Suman Ghosh <sumang@marvell.com>
> ---
>  .../ethernet/marvell/octeontx2/nic/otx2_dcbnl.c | 17 ++++++++++++++---
>  1 file changed, 14 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 bfddbff7bcdf..28fb643d2917 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c
> @@ -399,9 +399,10 @@ 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);
> +	u8 old_pfc_en;
>  	int err;
>  
> -	/* Save PFC configuration to interface */
> +	old_pfc_en = pfvf->pfc_en;
>  	pfvf->pfc_en = pfc->pfc_en;
>  
>  	if (pfvf->hw.tx_queues >= NIX_PF_PFC_PRIO_MAX)
> @@ -411,13 +412,17 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
>  	 * supported by the tx queue configuration
>  	 */
>  	err = otx2_check_pfc_config(pfvf);
> -	if (err)
> +	if (err) {
> +		pfvf->pfc_en = old_pfc_en;
>  		return err;

Hi Suman,

I think that rather than duplicating this logic,
it would be appropriate to use a goto label.

(OTOH, while not related to this patch, removing the process_pfc
 label would be a win for readability, IMHO.)

> +	}
>  
>  process_pfc:
>  	err = otx2_config_priority_flow_ctrl(pfvf);
> -	if (err)
> +	if (err) {
> +		pfvf->pfc_en = old_pfc_en;
>  		return err;
> +	}
>  
>  	/* Request Per channel Bpids */
>  	if (pfc->pfc_en)
> @@ -425,6 +430,12 @@ static int otx2_dcbnl_ieee_setpfc(struct net_device *dev, struct ieee_pfc *pfc)
>  
>  	err = otx2_pfc_txschq_update(pfvf);
>  	if (err) {
> +		if (pfc->pfc_en)
> +			otx2_nix_config_bp(pfvf, false);
> +
> +		otx2_pfc_txschq_stop(pfvf);
> +		pfvf->pfc_en = old_pfc_en;
> +		otx2_config_priority_flow_ctrl(pfvf);
>  		dev_err(pfvf->dev, "%s failed to update TX schedulers\n", __func__);
>  		return err;
>  	}
> -- 
> 2.25.1
> 

Perhaps I am on the wrong track here, but if
1. otx2_pfc_txschq_stop() was called by otx2_pfc_txschq_update()
   (or otx2_pfc_txschq_config()) for relevant error cases; and
2. pfc_en was passed as a parameter to otx2_config_priority_flow_ctrl()

Then I think that the unwind logic in the if condition above could
be expressed as unwind ladder with goto labels whereby the order
of unwinding is strictly the reverse of configuration.

This is a subjective opinion, but the advantage of this approach is that it
tends to lead to more maintainable code and fewer errors in... error paths.

(Completely untested!)

	...
	if (err)
		goto err_pfc_en;
	...
	err = otx2_pfc_txschq_update(pfvf);
	if (err)
		goto err_config;

	return 0;

err_config:
	if (pfc->pfc_en)
		otx2_nix_config_bp(pfvf, false);
	otx2_config_priority_flow_ctrl(pfvf, old_pfc_en);
err_pfc_en:
	pfvf->pfc_en = old_pfc_en;

	return err;

RE: [EXT] Re: [net PATCH] octeontx2-pf: Fix graceful exit during PFC configuration failure

[Suman Ghosh]

>>  	err = otx2_check_pfc_config(pfvf);
>> -	if (err)
>> +	if (err) {
>> +		pfvf->pfc_en = old_pfc_en;
>>  		return err;
>
>Hi Suman,
>
>I think that rather than duplicating this logic, it would be appropriate
>to use a goto label.
>
>(OTOH, while not related to this patch, removing the process_pfc  label
>would be a win for readability, IMHO.)
[Suman] Sure, I can update that.
>
>> +	}
>>
>>  process_pfc:
>>  	err = otx2_config_priority_flow_ctrl(pfvf);
>> -	if (err)
>> +	if (err) {
>> +		pfvf->pfc_en = old_pfc_en;
>>  		return err;
>> +	}
>>
>>  	/* Request Per channel Bpids */
>>  	if (pfc->pfc_en)
>> @@ -425,6 +430,12 @@ static int otx2_dcbnl_ieee_setpfc(struct
>> net_device *dev, struct ieee_pfc *pfc)
>>
>>  	err = otx2_pfc_txschq_update(pfvf);
>>  	if (err) {
>> +		if (pfc->pfc_en)
>> +			otx2_nix_config_bp(pfvf, false);
>> +
>> +		otx2_pfc_txschq_stop(pfvf);
>> +		pfvf->pfc_en = old_pfc_en;
>> +		otx2_config_priority_flow_ctrl(pfvf);
>>  		dev_err(pfvf->dev, "%s failed to update TX schedulers\n",
>__func__);
>>  		return err;
>>  	}
>> --
>> 2.25.1
>>
>
>Perhaps I am on the wrong track here, but if
>1. otx2_pfc_txschq_stop() was called by otx2_pfc_txschq_update()
>   (or otx2_pfc_txschq_config()) for relevant error cases; and
>2. pfc_en was passed as a parameter to otx2_config_priority_flow_ctrl()
>
>Then I think that the unwind logic in the if condition above could
>be expressed as unwind ladder with goto labels whereby the order
>of unwinding is strictly the reverse of configuration.
>
>This is a subjective opinion, but the advantage of this approach is that
>it
>tends to lead to more maintainable code and fewer errors in... error
>paths.
>
>(Completely untested!)
>
>	...
>	if (err)
>		goto err_pfc_en;
>	...
>	err = otx2_pfc_txschq_update(pfvf);
>	if (err)
>		goto err_config;
>
>	return 0;
>
>err_config:
>	if (pfc->pfc_en)
>		otx2_nix_config_bp(pfvf, false);
>	otx2_config_priority_flow_ctrl(pfvf, old_pfc_en);
>err_pfc_en:
>	pfvf->pfc_en = old_pfc_en;
>
>	return err;
[Suman] Let me think through it. I need to check if some cases will be missed, will update, and push a new patch in that case. Thanks for your feedback, Simon!!

Re: [EXT] Re: [net PATCH] octeontx2-pf: Fix graceful exit during PFC configuration failure

[Simon Horman]

On Sun, Dec 17, 2023 at 06:26:24AM +0000, Suman Ghosh wrote:

...

> >Perhaps I am on the wrong track here, but if
> >1. otx2_pfc_txschq_stop() was called by otx2_pfc_txschq_update()
> >   (or otx2_pfc_txschq_config()) for relevant error cases; and
> >2. pfc_en was passed as a parameter to otx2_config_priority_flow_ctrl()
> >
> >Then I think that the unwind logic in the if condition above could
> >be expressed as unwind ladder with goto labels whereby the order
> >of unwinding is strictly the reverse of configuration.
> >
> >This is a subjective opinion, but the advantage of this approach is that
> >it
> >tends to lead to more maintainable code and fewer errors in... error
> >paths.
> >
> >(Completely untested!)
> >
> >	...
> >	if (err)
> >		goto err_pfc_en;
> >	...
> >	err = otx2_pfc_txschq_update(pfvf);
> >	if (err)
> >		goto err_config;
> >
> >	return 0;
> >
> >err_config:
> >	if (pfc->pfc_en)
> >		otx2_nix_config_bp(pfvf, false);
> >	otx2_config_priority_flow_ctrl(pfvf, old_pfc_en);
> >err_pfc_en:
> >	pfvf->pfc_en = old_pfc_en;
> >
> >	return err;
> [Suman] Let me think through it. I need to check if some cases will be missed, will update, and push a new patch in that case. Thanks for your feedback, Simon!!

Thanks, much appreciated.