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

[Paolo Abeni <pabeni@redhat.com>]

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!

Paolo