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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 17 Dec 2023 06:26:24 +0000
From: Suman Ghosh <sumang@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com"
	<edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        Sunil Kovvuri Goutham
	<sgoutham@...vell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        Jerin
 Jacob Kollanukkaran <jerinj@...vell.com>,
        Geethasowjanya Akula
	<gakula@...vell.com>,
        Hariprasad Kelam <hkelam@...vell.com>,
        Linu Cherian
	<lcherian@...vell.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] Re: [net PATCH] octeontx2-pf: Fix graceful exit during PFC
 configuration failure

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