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