From: moyufeng Sent: Friday, June 25, 2021 3:13 AM
On 2021/4/24 22:51, Salil Mehta wrote:
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;
Inverted triangle structure required
yes, we can do that.
- 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) {
If exits directly without uninit here, init will be executed repeatedly, causing memory leakage issue.
No. this is not the case as explained and *agreed* in the espace.
In crux, failure of bringing UP a device does not need to uninit the client again. It does not makes sense as reset was a success.
User can use manual "ifconfig <netdev> up" command to bring it up. It that does not works it’s a bug in the firmware as UP can only fail if queue reset fails.
This case is no different than when device is left when DOWN. It is perfectly in sane state unless firmware has a bug. Ideally, bug should never fail!
Thanks