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