[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<SJ0PR18MB52169B706EB4F8F8D8FAB20CDB91A@SJ0PR18MB5216.namprd18.prod.outlook.com>
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!!
Powered by blists - more mailing lists