Re: [PATCH net] net: hns3: fix a wrong reset interrupt status mask

[tanhuazhong <tanhuazhong@huawei.com>]

On 2019/11/20 7:36, David Miller wrote:
> From: Huazhong Tan <tanhuazhong@huawei.com>
> Date: Tue, 19 Nov 2019 10:31:48 +0800
> 
>> According to hardware user manual, bits5~7 in register
>> HCLGE_MISC_VECTOR_INT_STS means reset interrupts status,
>> but HCLGE_RESET_INT_M is defined as bits0~2 now. So it
>> will make hclge_reset_err_handle() read the wrong reset
>> interrupt status.
>>
>> This patch fixes it and prints out the register value.
>>
>> Fixes: 2336f19d7892 ("net: hns3: check reset interrupt status when reset fails")
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> 
> Fix exactly _one_ thing or else you make your patch hard to review.
> 
> The bug is that the bits are wrong, just fix the bits!
> 

ok, thanks

>>   
>> +	u32 msix_sts_reg;
>> +
>> +	msix_sts_reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_VECTOR_INT_STS);
>> +
>>   	if (hdev->reset_pending) {
> 
> Now you are reading a register, and potentially clearing status bits and
> causing other side effects, that would not happen in this code path
> where hdev->reset_pending is true.
> 
> Don't do stuff like this!
> 
> If you want to add code to print out the register value, that is a
> separate patch, for net-next, and it must be done properly.  In that
> you should only read the register in the same code paths you do
> previously.   Otherwise you must _clearly_ explain why reading the
> register value in new code paths is OK, and the side effects will
> not potentially cause problems for the pending reset operation.  It
> is still going to be a net-next improvement only.
> 
> Thank you.

I will separate it, and resend the fix to -net.
Thanks.

> 
> .
>