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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <707be979-7716-5669-ad91-ff0fdbcc101d@huawei.com>
Date:   Wed, 20 Nov 2019 09:36:44 +0800
From:   tanhuazhong <tanhuazhong@...wei.com>
To:     David Miller <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <salil.mehta@...wei.com>, <yisen.zhuang@...wei.com>,
        <linuxarm@...wei.com>, <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH net] net: hns3: fix a wrong reset interrupt status mask



On 2019/11/20 7:36, David Miller wrote:
> From: Huazhong Tan <tanhuazhong@...wei.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@...wei.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.

> 
> .
>