From: lipeng (Y) Sent: Friday, June 25, 2021 2:03 AM
在 2021/4/24 22:51, Salil Mehta 写道:
The fallback logic of the set channels when set_channel() fails to configure TX Sched/RSS H/W configuration or for the code which brings down/restores client before/therafter is not handled properly.
Fix and refactor the code for handling the errors properly and to improve the readibility.
Fixes: 3a5a5f06d4d2 ("net: hns3: revert to old channel when setting new channel num fail") Signed-off-by: Salil Mehta salil.mehta@huawei.com Signed-off-by: Peng Li lipeng321@huawei.com
.../net/ethernet/hisilicon/hns3/hns3_enet.c | 77 +++++++++++-------- 1 file changed, 47 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index bf4302a5cf95..fbb0f4c9b98e 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c @@ -4692,24 +4692,60 @@ static int hns3_reset_notify(struct hnae3_handle *handle, static int hns3_change_channels(struct hnae3_handle *handle, u32 new_tqp_num, bool rxfh_configured) {
- int ret;
- const struct hnae3_ae_ops *ops = handle->ae_algo->ops;
- u32 org_tqp_num = handle->kinfo.num_tqps;
- struct device *dev = &handle->pdev->dev;
- u32 req_tqp_num = new_tqp_num;
- bool revert_old_config = false;
- int ret, retval = 0;
- /* bring down the client */
- ret = hns3_reset_notify(handle, HNAE3_DOWN_CLIENT);
- if (ret) {
dev_err(dev, "client down fail, this should'nt have happened!\n");return ret;- }
- ret = handle->ae_algo->ops->set_channels(handle, new_tqp_num,
rxfh_configured);
- ret = hns3_reset_notify(handle, HNAE3_UNINIT_CLIENT); if (ret) {
dev_err(&handle->pdev->dev,"Change tqp num(%u) fail.\n", new_tqp_num);
dev_err(dev, "client uninit fail, this should'nt have happened!\n");+revert_old_tpqs_config:
- /* update the TX Sched and RSS config in the H/W */
- ret = ops->set_channels(handle, req_tqp_num, rxfh_configured);
- if (ret) {
dev_err(dev, "TX Sched/RSS H/W cfg fail(=%d) for %s TPQs\n",ret, revert_old_config ? "old" : "new");goto err_set_channel;- }
- /* restore the client */ ret = hns3_reset_notify(handle, HNAE3_INIT_CLIENT);
- if (ret)
return ret;
if (ret) {
dev_err(dev, "failed to initialize the client again\n");goto err_set_channel;}
ret = hns3_reset_notify(handle, HNAE3_UP_CLIENT);
- if (ret)
hns3_reset_notify(handle, HNAE3_UNINIT_CLIENT);
- if (ret) {
dev_err(dev, "Client up fail, this should'nt have happened!\n");return ret;- }
- return retval;
+err_set_channel:
- if (!revert_old_config) {
dev_warn(dev, "Revert TX Sched/RSS H/W config with old TPQs\n");req_tqp_num = org_tqp_num;revert_old_config = true;retval = ret;goto revert_old_tpqs_config;Hi,salil,
Huawei C programming specification requires goto statement cannot jump up.
Hi Lipeng,
New code is: 1. Making existing code *more structured* by correcting the overspill of the change channel logic beyond function hns3_change_channels(). Right now, its half inside and half outside. 2. Ensuring the gotos are within the same scope of the function and it is bounded jump. 3. Goto is part of the exception leg of the code rather the normal code. In our case it only happens once in case when failure occurs and we need to attempt to restore older config. This is clearly an exception leg and not random jumps. 4. Fixing existing problems with the code. 5. Lastly, use of the goto is also within the visible boundary of the function.
With respect to the section in you guidelines: Link: http://openx.huawei.com/communityWikiDetails?communityId=41&ibid=SPE1000...
There is difference in what example function listed in above link is trying to do with gotos and what the goto in this patch is doing. 1. Above function is using goto to jump out of the loop in normal execution flow. Goto in the patch is being used only in the exceptional condition i.e. when normal flow encounters a failure in some leg. 2. goto statement in above example looks unbounded. Goto statement in the .patch is bounded and in fact executed only once even in exceptional circumstance of restoring older config. 3. Goto in above example looks avoidable but goto in the patch being submitted is the last resort we are using as a tradeoff to braking the structured paradigm. Later is more important.
In simple terms, goto which generally is termed as the one which breaks structured programming, here it is helping to restore structured programing by very limited and controlled use of just *one* jump in the exceptional failure case. It is ironical but I think it is a perfectly fine tradeoff.
Thanks
}
dev_err(dev, "Bad, we could'nt revert to old TPQ H/W config\n");
dev_warn(dev, "Device maybe insane. Reload driver/Reset required!\n"); return ret; }
@@ -4720,7 +4756,6 @@ int hns3_set_channels(struct net_device *netdev, struct hnae3_knic_private_info *kinfo = &h->kinfo; bool rxfh_configured = netif_is_rxfh_configured(netdev); u32 new_tqp_num = ch->combined_count;
u16 org_tqp_num; int ret;
if (hns3_nic_resetting(netdev))
@@ -4750,28 +4785,10 @@ int hns3_set_channels(struct net_device *netdev, "set channels: tqp_num=%u, rxfh=%d\n", new_tqp_num, rxfh_configured);
- ret = hns3_reset_notify(h, HNAE3_DOWN_CLIENT);
- if (ret)
return ret;- ret = hns3_reset_notify(h, HNAE3_UNINIT_CLIENT);
- if (ret)
return ret;- org_tqp_num = h->kinfo.num_tqps; ret = hns3_change_channels(h, new_tqp_num, rxfh_configured); if (ret) {
int ret1;netdev_warn(netdev,"Change channels fail, revert to old value\n");ret1 = hns3_change_channels(h, org_tqp_num, rxfh_configured);if (ret1) {netdev_err(netdev,"revert to old channel fail\n");return ret1;}
netdev_err(netdev, "fail(=%d) to set number of channels to %u\n", ret,new_tqp_num);