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

[tanhuazhong]

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

> .