Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Jijie Shao <shaojijie@huawei.com>]

on 2024/12/19 18:43, Paolo Abeni wrote:
> On 12/19/24 11:11, Michal Swiatkowski wrote:
>> On Thu, Dec 19, 2024 at 10:41:53AM +0100, Paolo Abeni wrote:
>>> On 12/18/24 10:02, Michal Swiatkowski wrote:
>>>> On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
>>>>> From: Hao Lan <lanhao@huawei.com>
>>>>>
>>>>> When a reset type that is not supported by the driver is input, a reset
>>>>> pending flag bit of the HNAE3_NONE_RESET type is generated in
>>>>> reset_pending. The driver does not have a mechanism to clear this type
>>>>> of error. As a result, the driver considers that the reset is not
>>>>> complete. This patch provides a mechanism to clear the
>>>>> HNAE3_NONE_RESET flag and the parameter of
>>>>> hnae3_ae_ops.set_default_reset_request is verified.
>>>>>
>>>>> The error message:
>>>>> hns3 0000:39:01.0: cmd failed -16
>>>>> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
>>>>> hns3 0000:39:01.0: failed to reset VF stack
>>>>> hns3 0000:39:01.0: failed to reset VF(4)
>>>>> hns3 0000:39:01.0: prepare reset(2) wait done
>>>>> hns3 0000:39:01.0 eth4: already uninitialized
>>>>>
>>>>> Use the crash tool to view struct hclgevf_dev:
>>>>> struct hclgevf_dev {
>>>>> ...
>>>>> 	default_reset_request = 0x20,
>>>>> 	reset_level = HNAE3_NONE_RESET,
>>>>> 	reset_pending = 0x100,
>>>>> 	reset_type = HNAE3_NONE_RESET,
>>>>> ...
>>>>> };
>>>>>
>>>>> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
>>>>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>>>>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> I haven't signed-off this patch.
>>>
>>> Still no need to repost (yet) for this if the following points are
>>> solved rapidly (as I may end-up merging the series and really adding my
>>> SoB), but please avoid this kind of issue in the future.
>>>
>>>>> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
>>>>>   		return false;
>>>>>   	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
>>>>>   		hdev->rst_stats.reset_fail_cnt++;
>>>>> -		set_bit(hdev->reset_type, &hdev->reset_pending);
>>>>> +		hclge_set_reset_pending(hdev, hdev->reset_type);
>>>> Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
>>>> no reset? If yes, why in this case reset_fail_cnt++ is increasing?
>>>>
>>>> Maybe the check for NONE_RESET should be done in this else if check to
>>>> prevent reset_fail_cnt from increasing (and also solve the problem with
>>>> pending bit set)
>>> @Michal: I don't understand your comment above. hclge_reset_err_handle()
>>> handles attempted reset failures. I don't see it triggered when
>>> reset_type == HNAE3_NONE_RESET.
>>>
>> Maybe I missed sth. The hclge_set_reset_pending() is added to check if
>> reset type isn't HNAE3_NONE_RESET. If it is the set_bit isn't called. It
>> is the only place where hclge_set_reset_pending() is called with a
>> variable, so I assumed the fix is for this place.
>>
>> This means that code can be reach here with HNAE3_NONE_RESET which is
>> unclear for me why to increment resets if rest_type in NONE. If it is
>> true that hclge_reset_err_handle() is never called with reset_type
>> HNAE3_NONE_RESET it shouldn't be needed to have the
>> hclge_set_reset_pending() function.
> You are right, I felt off-track.
>
> @Jijie: how can 'reset_type' be set to an unsupported value?!? I don't
> see that in the code, short of a memory corruption on uninit problem.
> Are you sure you are not papering over a different issue here? At least
> some more info (either in the commit description or in a code comment)
> is IMHO needed. Otherwise you should probably catch that before
> hclge_reset_err_handle() time.

Thanks for reviewing the code.

In fact, we used hclge_set_reset_pendin() to check in entire reset path,
not just hclge_reset_err_handle().

But seems like overkill, I'll try to simplify the modification.

Thank you.

Jiejie Shao